Re: pg_stop_backup(wait_for_archive := true) on standby server

On Tue, Jul 11, 2017 at 9:28 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Attached updated version patch. Please review it.

Cool, thanks.

+    useless. If the second parameter <parameter>wait_for_archive</> is true and
+    the backup is taken on a standby, <function>pg_stop_backup</> waits for WAL
+    to archived when <varname>archive_mode</> is <literal>always</>.  Enforcing
+    manually a WAL segment swtich to happen with for example
1) "waits for WAL to BE archived".
2) s/swtich/switch

+    to <literal>false</> will control the wait time, thought all the
WAL segments
s/thought/though/

                /*
                 * During recovery, since we don't use the end-of-backup WAL
                 * record and don't write the backup history file, the
                 * starting WAL location doesn't need to be unique.
This means
                 * that two base backups started at the same time
might use
                 * the same checkpoint as starting locations.
                 */
This comment in xlog.c needs an update. Two base backups started at
the same can generate a backup history file with the same offset, aka
same file name. I don't think that it matters much for this file
honestly. I think that this would become meaningful once such files
play a more important role, in which case having a unique identifier
would be way more interesting, but this day has IMO not come yet.
Other thoughts are welcome.

    if (waitforarchive && XLogArchivingActive())
    {
+       /* Don't wait if WAL archiving not enabled always in recovery */
+       if (backup_started_in_recovery && !XLogArchivingAlways())
+           return stoppoint;
+
This has the smell of breakage waiting to happen, I think that we
should have just one single return point, which updates as well the
stop TLI at the end of the routine. This can just be a single
condition.

+   if (stoptli_p)
+       *stoptli_p = stoptli;
+
Not sure there is any point to move that around, on top of previous comment.

+                            errhint("Backup has been taken from a
standby, check if the WAL segment "
+                                    "needed for this backup have been
completed, in which case a "
+                                    "foreced segment switch may can
be needed on the primary. "
+                                    "If a segment swtich has already
happend check that your "
+                                    "archive_command is executing properly."
+                                    "pg_stop_backup can be canceled
safely, but the database backup "
+                                    "will not be usable without all
the WAL segments.")));
Some comments here:
1) s/foreced/forced/
2) s/may can/may/
3) s/swtich/switch/
4) s/happend/happened/
5) "Backup has been taken from a standby" should be a single sentence.

This is beginning to shape.

Thanks,
-- 
Michael