Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server

On Wed, Jul 12, 2017 at 5:06 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> 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.

Thank you for reviewing the patch.

>
> +    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.
>

Sorry, I missed lots of typo in the last patch. All comments from you
are incorporated into the attached latest patch and I've checked it
whether there is other typos. Please review it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

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