Update low-level backup documentation to match actual behavior

-     the next WAL segment.  The reason for the switch is to arrange for
+     the next WAL segment when run on a primary.  On a standby you can call
+     <function>pg_switch_wal</function> on the primary to perform a manual
+     switch.
+     The reason for the switch is to arrange for

Tacking on "when run on a primary" onto the end of the existing
sentence is a little ambiguous: does that clause apply only to the
last part, or to the whole sentence? I suggest something like: This
terminates the backup mode. On a primary, it also performs an
automatic switch to the next WAL segment. On a standby, it is not
possible to automatically switch WAL segments, so you may wish to
consider running <function>pg_switch_wal</function> on the primary to
perform a manual switch.

-     backup files.  If <varname>archive_mode</> is enabled,
+     backup files.  On a primary, if <varname>archive_mode</> is
enabled and the
+     <literal>wait_for_archive</> parameter is <literal>true</>,
      <function>pg_stop_backup</> does not return until the last segment has
      been archived.
+     On a standby, <varname>archive_mode</> must be <literal>always</> in order
+     for <function>pg_stop_backup</> to wait.

Looks good.

-    <title>Making an exclusive low level backup</title>
+    <title>Making an exclusive low level backup on a primary</title>

I'd omit this hunk.

-     more than one concurrent backup to run, and there can be some issues on
+     more than one concurrent backup to run, must be run on a
primary, and there
+     can be some issues on

Maybe this would be clearer: This type of backup can only be taken on
a primary, does not allow more than one ...

-     This function, when called on a primary, terminates the backup mode and
+     This function terminates the backup mode and
      performs an automatic switch to the next WAL segment. The reason for the
      switch is to arrange for the last WAL segment written during the backup
-     interval to be ready to archive.  When called on a standby, this function
-     only terminates backup mode.  A subsequent WAL segment switch will be
-     needed in order to ensure that all WAL files needed to restore the backup
-     can be archived; if the primary does not have sufficient write activity
-     to trigger one, <function>pg_switch_wal</function> should be executed on
-     the primary.
+     interval to be ready to archive.

Why do you want to delete all that text? It seems like good text to me.

-    The function also creates a backup history file in the write-ahead log
+    When executed on a primary, the function also creates a backup history file
+    in the write-ahead log

Looks good.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert,

Thanks for reviewing!

On 8/18/17 2:45 PM, Robert Haas wrote:

-     the next WAL segment.  The reason for the switch is to arrange for
+     the next WAL segment when run on a primary.  On a standby you can call
+     <function>pg_switch_wal</function> on the primary to perform a manual
+     switch.
+     The reason for the switch is to arrange for

Tacking on "when run on a primary" onto the end of the existing
sentence is a little ambiguous: does that clause apply only to the
last part, or to the whole sentence? I suggest something like: This
terminates the backup mode. On a primary, it also performs an
automatic switch to the next WAL segment. On a standby, it is not
possible to automatically switch WAL segments, so you may wish to
consider running <function>pg_switch_wal</function> on the primary to
perform a manual switch.

Looks good.

-    <title>Making an exclusive low level backup</title>
+    <title>Making an exclusive low level backup on a primary</title>

I'd omit this hunk.

OK, but I was trying to make it very clear that this backup method only
works on a primary. If you think the text is in the first paragraph is
enough then I'm willing to go with that, though.

-     more than one concurrent backup to run, and there can be some issues on
+     more than one concurrent backup to run, must be run on a
primary, and there
+     can be some issues on

Maybe this would be clearer: This type of backup can only be taken on
a primary, does not allow more than one ...

Looks good.

-     This function, when called on a primary, terminates the backup mode and
+     This function terminates the backup mode and
performs an automatic switch to the next WAL segment. The reason for the
switch is to arrange for the last WAL segment written during the backup
-     interval to be ready to archive.  When called on a standby, this function
-     only terminates backup mode.  A subsequent WAL segment switch will be
-     needed in order to ensure that all WAL files needed to restore the backup
-     can be archived; if the primary does not have sufficient write activity
-     to trigger one, <function>pg_switch_wal</function> should be executed on
-     the primary.
+     interval to be ready to archive.

Why do you want to delete all that text? It seems like good text to me.

Since the exclusive method only works on a primary...

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Aug 18, 2017 at 2:58 PM, David Steele <david@pgmasters.net> wrote:

OK, but I was trying to make it very clear that this backup method only
works on a primary. If you think the text is in the first paragraph is
enough then I'm willing to go with that, though.

Yeah, I think the text is enough.

Since the exclusive method only works on a primary...

Oh, right. Duh.

If you update the patch I'll apply it to 11 and 10.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/18/17 3:00 PM, Robert Haas wrote:

If you update the patch I'll apply it to 11 and 10.

Attached is the updated patch.

I didn't like the vague "there can be some issues on the server if it
crashes during the backup" so I added a new paragraph at the appropriate
step to give a more detailed explanation of the problem.

Thanks,
--
-David
david@pgmasters.net

On Fri, Aug 18, 2017 at 3:35 AM, David Steele <david@pgmasters.net> wrote:

This patch should be sufficient for 10/11 but will need some minor
changes for 9.6 to remove the reference to wait_for_archive. Note that
this patch ignores Michael's patch [2] to create WAL history files on a
standby as this will likely only be applied to master.

I'll just rebase as needed the other patch, this documentation update
is very important in itself, so let's not worry about that.

On Sat, Aug 19, 2017 at 7:46 AM, David Steele <david@pgmasters.net> wrote:

On 8/18/17 3:00 PM, Robert Haas wrote:

If you update the patch I'll apply it to 11 and 10.

Attached is the updated patch.

I didn't like the vague "there can be some issues on the server if it
crashes during the backup" so I added a new paragraph at the appropriate
step to give a more detailed explanation of the problem.

Thanks for the patch.

-     This terminates the backup mode and performs an automatic switch to
-     the next WAL segment.  The reason for the switch is to arrange for
+     This terminates backup mode. On a primary, it also performs an automatic
+     switch to the next WAL segment.  On a standby, it is not possible to
+     automatically switch WAL segments, so you may wish to run
+     <function>pg_switch_wal</function> on the primary to perform a manual
+     switch. The reason for the switch is to arrange for
      the last WAL segment file written during the backup interval to be
      ready to archive.
[...]
-     backup files.  If <varname>archive_mode</> is enabled,
+     backup files.  On a primary, if <varname>archive_mode</> is
enabled and the
+     <literal>wait_for_archive</> parameter is <literal>true</>,
      <function>pg_stop_backup</> does not return until the last segment has
      been archived.
+     On a standby, <varname>archive_mode</> must be <literal>always</> in order
+     for <function>pg_stop_backup</> to wait.

This level of details is good to have. Thanks.

+ Prior to PostgreSQL 9.6, this
Markup <productname>?

+      Note well that if the server crashes during the backup it may not be
+      possible to restart until the <literal>backup_label</> file has been
+      manually deleted from the PGDATA directory.
Missing a markup <envvar> here for PGDATA.
s/Note well/Note as well/, no?

-     This function, when called on a primary, terminates the backup mode and
+     This function terminates backup mode and
      performs an automatic switch to the next WAL segment. The reason for the
      switch is to arrange for the last WAL segment written during the backup
-     interval to be ready to archive.  When called on a standby, this function
-     only terminates backup mode.  A subsequent WAL segment switch will be
-     needed in order to ensure that all WAL files needed to restore the backup
-     can be archived; if the primary does not have sufficient write activity
-     to trigger one, <function>pg_switch_wal</function> should be executed on
-     the primary.
+     interval to be ready to archive.
pg_stop_backup() still waits for the last WAL segment to be archived
on the primary. Perhaps we'd want to mention that?

Documentation does not state yet that the use of low-level APIs for
exclusive backups are not supported on standbys.

Now in the docs:
If the backup process monitors and ensures that all WAL segment files
required for the backup are successfully archived then the second
parameter (which defaults to true) can be set to false to have
I would recommend adding some details here and mention
"wait_for_archive" instead of "second parameter". I am wondering as
well if this paragraph should be put in red with a warning or
something like that. This is really, really important to ensure
consistent backups!
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Michael,

Thanks for reviewing! Sorry for the late response, those eclipses don't
just chase themselves...

On 8/20/17 10:22 PM, Michael Paquier wrote:

On Fri, Aug 18, 2017 at 3:35 AM, David Steele <david@pgmasters.net> wrote:

+ Prior to PostgreSQL 9.6, this
Markup <productname>?

Fixed.

+      Note well that if the server crashes during the backup it may not be
+      possible to restart until the <literal>backup_label</> file has been
+      manually deleted from the PGDATA directory.
Missing a markup <envvar> here for PGDATA.

Fixed.

s/Note well/Note as well/, no?

This was a literal translation of nota bene but I've changed it to
simply "Note" as that seems common in the docs.

-     This function, when called on a primary, terminates the backup mode and
+     This function terminates backup mode and
performs an automatic switch to the next WAL segment. The reason for the
switch is to arrange for the last WAL segment written during the backup
-     interval to be ready to archive.  When called on a standby, this function
-     only terminates backup mode.  A subsequent WAL segment switch will be
-     needed in order to ensure that all WAL files needed to restore the backup
-     can be archived; if the primary does not have sufficient write activity
-     to trigger one, <function>pg_switch_wal</function> should be executed on
-     the primary.
+     interval to be ready to archive.
pg_stop_backup() still waits for the last WAL segment to be archived
on the primary. Perhaps we'd want to mention that?

That's detailed in the next paragraph, ISTM.

Documentation does not state yet that the use of low-level APIs for
exclusive backups are not supported on standbys.

The first paragraph of the exclusive section states, "this type of
backup can only be taken on a primary".

Now in the docs:
If the backup process monitors and ensures that all WAL segment files
required for the backup are successfully archived then the second
parameter (which defaults to true) can be set to false to have
I would recommend adding some details here and mention
"wait_for_archive" instead of "second parameter".

Done.

I am wondering as
well if this paragraph should be put in red with a warning or
something like that. This is really, really important to ensure
consistent backups!

Maybe, but this logic could easily apply to a lot of sections in the
backup docs. I'm not sure where it would end.

Thanks!
--
-David
david@pgmasters.net

On Thu, Aug 24, 2017 at 10:49 PM, David Steele <david@pgmasters.net> wrote:

Thanks for reviewing! Sorry for the late response, those eclipses don't
just chase themselves...

That's quite something to see.

On 8/20/17 10:22 PM, Michael Paquier wrote:

On Fri, Aug 18, 2017 at 3:35 AM, David Steele <david@pgmasters.net> wrote:

+ Prior to PostgreSQL 9.6, this
Markup <productname>?

Fixed.

+      Note well that if the server crashes during the backup it may not be
+      possible to restart until the <literal>backup_label</> file has been
+      manually deleted from the PGDATA directory.
Missing a markup <envvar> here for PGDATA.

Fixed.

s/Note well/Note as well/, no?

This was a literal translation of nota bene but I've changed it to
simply "Note" as that seems common in the docs.

Oh, OK.

Documentation does not state yet that the use of low-level APIs for
exclusive backups are not supported on standbys.

The first paragraph of the exclusive section states, "this type of
backup can only be taken on a primary".

Sorry, missed that.

Now in the docs:
If the backup process monitors and ensures that all WAL segment files
required for the backup are successfully archived then the second
parameter (which defaults to true) can be set to false to have
I would recommend adding some details here and mention
"wait_for_archive" instead of "second parameter".

Done.

I am wondering as
well if this paragraph should be put in red with a warning or
something like that. This is really, really important to ensure
consistent backups!

Maybe, but this logic could easily apply to a lot of sections in the
backup docs. I'm not sure where it would end.

True as well. The patch looks good to me. If a committer does not show
up soon, it may be better to register that in the CF and wait. I am
not sure that adding an open item is suited, as docs have the same
problem on 9.6.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/24/17 7:36 PM, Michael Paquier wrote:

True as well. The patch looks good to me. If a committer does not show
up soon, it may be better to register that in the CF and wait. I am
not sure that adding an open item is suited, as docs have the same
problem on 9.6.

Robert said he would commit this so I expect he'll do that if he doesn't
have any objections to the changes.

Robert, if you would prefer me to submit this to the CF I'm happy to do so.

Thanks,
--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Aug 25, 2017 at 3:10 PM, David Steele <david@pgmasters.net> wrote:

On 8/24/17 7:36 PM, Michael Paquier wrote:

True as well. The patch looks good to me. If a committer does not show
up soon, it may be better to register that in the CF and wait. I am
not sure that adding an open item is suited, as docs have the same
problem on 9.6.

Robert said he would commit this so I expect he'll do that if he doesn't
have any objections to the changes.

Robert, if you would prefer me to submit this to the CF I'm happy to do so.

Ha, this note arrived just as I was working on getting this committed.
I'll commit this to 11 and 10 presently; can you produce a version for
9.6?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/25/17 3:13 PM, Robert Haas wrote:

On Fri, Aug 25, 2017 at 3:10 PM, David Steele <david@pgmasters.net> wrote:

Robert said he would commit this so I expect he'll do that if he doesn't
have any objections to the changes.

Robert, if you would prefer me to submit this to the CF I'm happy to do so.

Ha, this note arrived just as I was working on getting this committed.
I'll commit this to 11 and 10 presently; can you produce a version for
9.6?

No problem. I'll base it on your commit to capture any changes you made.

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Aug 25, 2017 at 3:21 PM, David Steele <david@pgmasters.net> wrote:

No problem. I'll base it on your commit to capture any changes you made.

Thanks, but you incorporated everything I wanted in response to my
first review -- so I didn't tweak it any further.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/25/17 3:26 PM, Robert Haas wrote:

On Fri, Aug 25, 2017 at 3:21 PM, David Steele <david@pgmasters.net> wrote:

No problem. I'll base it on your commit to capture any changes you made.

Thanks, but you incorporated everything I wanted in response to my
first review -- so I didn't tweak it any further.

Thank you for committing that. I'll get the 9.6 patch to you early next
week.

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Robert,

On 8/25/17 4:03 PM, David Steele wrote:

On 8/25/17 3:26 PM, Robert Haas wrote:

On Fri, Aug 25, 2017 at 3:21 PM, David Steele <david@pgmasters.net>
wrote:

No problem.  I'll base it on your commit to capture any changes you
made.

Thanks, but you incorporated everything I wanted in response to my
first review -- so I didn't tweak it any further.

Thank you for committing that.  I'll get the 9.6 patch to you early next
week.

Attached is the 9.6 patch. It required a bit more work in func.sgml
than I was expecting so have a close look at that. The rest was mostly
removing irrelevant hunks.

Thanks,
--
-David
david@pgmasters.net

On Tue, Aug 29, 2017 at 10:59 PM, David Steele <david@pgmasters.net> wrote:

Hi Robert,

On 8/25/17 4:03 PM, David Steele wrote:

On 8/25/17 3:26 PM, Robert Haas wrote:

On Fri, Aug 25, 2017 at 3:21 PM, David Steele <david@pgmasters.net>
wrote:

No problem. I'll base it on your commit to capture any changes you
made.

Thanks, but you incorporated everything I wanted in response to my
first review -- so I didn't tweak it any further.

Thank you for committing that. I'll get the 9.6 patch to you early next
week.

Attached is the 9.6 patch. It required a bit more work in func.sgml
than I was expecting so have a close look at that. The rest was mostly
removing irrelevant hunks.

+     switch to the next WAL segment.  On a standby, it is not possible to
+     automatically switch WAL segments, so you may wish to run
+     <function>pg_switch_wal</function> on the primary to perform a manual
+     switch. The reason for the switch is to arrange for
[...]
+    WAL segments have been archived. If write activity on the primary
is low, it
+    may be useful to run <function>pg_switch_wal</> on the primary in order to
+    trigger an immediate segment switch of the last required WAL
It seems to me that both portions are wrong. There is no archiving
wait on standbys for 9.6, and pg_stop_backup triggers by itself the
segment switch, so saying that enforcing pg_switch_wal on the primary
is moot. pg_switch_xlog has been renamed to pg_switch_wal in PG10, so
the former name applies.
-- 
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Michael,

Thanks for reviewing!

On 8/29/17 9:44 PM, Michael Paquier wrote:

On Tue, Aug 29, 2017 at 10:59 PM, David Steele <david@pgmasters.net> wrote:

Attached is the 9.6 patch. It required a bit more work in func.sgml
than I was expecting so have a close look at that. The rest was mostly
removing irrelevant hunks.

+     switch to the next WAL segment.  On a standby, it is not possible to
+     automatically switch WAL segments, so you may wish to run
+     <function>pg_switch_wal</function> on the primary to perform a manual
+     switch. The reason for the switch is to arrange for
[...]
+    WAL segments have been archived. If write activity on the primary
is low, it
+    may be useful to run <function>pg_switch_wal</> on the primary in order to
+    trigger an immediate segment switch of the last required WAL
It seems to me that both portions are wrong. There is no archiving
wait on standbys for 9.6, and 

I think its clearly stated here that pg_stop_backup() does not wait for
WAL to archive on a standby. Even, it is very important for the backup
routine to make sure that all the WAL *is* archived.

pg_stop_backup triggers by itself the
segment switch, so saying that enforcing pg_switch_wal on the primary
is moot.

pg_stop_backup() does not perform a WAL switch on the standby which is
what this sentence is referring to. I have separated this section out
into a new paragraph to (hopefully) make it clearer.

pg_switch_xlog has been renamed to pg_switch_wal in PG10, so
the former name applies.

Whoops!

New patch is attached.

Thanks,
--
-David
david@pgmasters.net

On Wed, Aug 30, 2017 at 8:02 PM, David Steele <david@pgmasters.net> wrote:

On 8/29/17 9:44 PM, Michael Paquier wrote:

On Tue, Aug 29, 2017 at 10:59 PM, David Steele <david@pgmasters.net> wrote:

Attached is the 9.6 patch. It required a bit more work in func.sgml
than I was expecting so have a close look at that. The rest was mostly
removing irrelevant hunks.

+     switch to the next WAL segment.  On a standby, it is not possible to
+     automatically switch WAL segments, so you may wish to run
+     <function>pg_switch_wal</function> on the primary to perform a manual
+     switch. The reason for the switch is to arrange for
[...]
+    WAL segments have been archived. If write activity on the primary
is low, it
+    may be useful to run <function>pg_switch_wal</> on the primary in order to
+    trigger an immediate segment switch of the last required WAL
It seems to me that both portions are wrong. There is no archiving
wait on standbys for 9.6, and

I think its clearly stated here that pg_stop_backup() does not wait for
WAL to archive on a standby. Even, it is very important for the backup
routine to make sure that all the WAL *is* archived.

Yes, it seems that I somewhat missed the "on the primary portion"
during the first read of the patch.

pg_stop_backup triggers by itself the
segment switch, so saying that enforcing pg_switch_wal on the primary
is moot.

pg_stop_backup() does not perform a WAL switch on the standby which is
what this sentence is referring to. I have separated this section out
into a new paragraph to (hopefully) make it clearer.

pg_switch_xlog has been renamed to pg_switch_wal in PG10, so
the former name applies.

Whoops!

New patch is attached.

Thanks for the new version. This looks fine to me.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Aug 30, 2017 at 8:37 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Thanks for the new version. This looks fine to me.

Committed to REL9_6_STABLE with minor wordsmithing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/31/17 4:04 PM, Robert Haas wrote:

On Wed, Aug 30, 2017 at 8:37 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Thanks for the new version. This looks fine to me.

Committed to REL9_6_STABLE with minor wordsmithing.

The edits look good to me. Thanks, Robert!

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Sep 1, 2017 at 5:11 AM, David Steele <david@pgmasters.net> wrote:

On 8/31/17 4:04 PM, Robert Haas wrote:

On Wed, Aug 30, 2017 at 8:37 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Thanks for the new version. This looks fine to me.

Committed to REL9_6_STABLE with minor wordsmithing.

The edits look good to me. Thanks, Robert!

Thanks David for the patch, and Robert for the commit! The final
result is nicely shaped.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers