Re: [HACKERS] Update low-level backup documentation to match actual behavior

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
intervalto 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
thelast 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
aresuccessfully 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