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

On Tue, Jul 11, 2017 at 9:28 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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