Creating backup history files for backups taken from standbys

On Thu, Aug 10, 2017 at 3:56 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Hi all,

In the recent thread related to a bug in pg_stop_backup when waiting
for WAL segments to be archived, it has been mentioned that it would
be nice to get backup history files generated as well on standbys:
/messages/by-id/CA+Tgmobh3=quCzugFP5yA3-z5uzz=YKZ7arnoApJoPN7=0H8TQ@mail.gmail.com

Backup history files are not essential to backups, but they are for
debugging purposes, and it has bugged me hard that we should have a
consistent behavior in this area, particularly because with
archive_mode = always they can be archived by a standby.

The previous thread has ended with 52f8a59d, which addressed the
original problem but discarded the backup history file generation
during recovery. Attached is a patch to fix this inconsistency, parked
in next CF. I am not arguing for this change as a bug fix, but as an
improvement for PG11.

Thoughts?

Thank you for the patch. Regarding to creating the backup history file
on stanbys, is there any difference from the patch posted on earlier
thread?

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

On Thu, Aug 10, 2017 at 9:45 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Thank you for the patch. Regarding to creating the backup history file
on stanbys, is there any difference from the patch posted on earlier
thread?

That's a rebased version on top of what has been applied, except that
I have fixed an incorrect comment and shuffled the code building the
WAL segment name for the stop position.
--
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 Thu, Aug 10, 2017 at 4:50 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Aug 10, 2017 at 9:45 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Thank you for the patch. Regarding to creating the backup history file
on stanbys, is there any difference from the patch posted on earlier
thread?

That's a rebased version on top of what has been applied, except that
I have fixed an incorrect comment and shuffled the code building the
WAL segment name for the stop position.

Understood, I'll review this patch.

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

On Thu, Aug 10, 2017 at 4:55 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Aug 10, 2017 at 4:50 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Aug 10, 2017 at 9:45 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Thank you for the patch. Regarding to creating the backup history file
on stanbys, is there any difference from the patch posted on earlier
thread?

That's a rebased version on top of what has been applied, except that
I have fixed an incorrect comment and shuffled the code building the
WAL segment name for the stop position.

Understood, I'll review this patch.

Here is an updated patch with refreshed documentation, as a result of
449338c which was discussed in thread
/messages/by-id/d4d951b9-89c0-6bc1-b6ff-d0b2dd5a8966@pgmasters.net.
I am just outlining the fact that a backup history file gets created
on standbys and that it is archived.
--
Michael

Hi Michael,

On 8/31/17 11:56 PM, Michael Paquier wrote:

Here is an updated patch with refreshed documentation, as a result of
449338c which was discussed in thread
/messages/by-id/d4d951b9-89c0-6bc1-b6ff-d0b2dd5a8966@pgmasters.net.
I am just outlining the fact that a backup history file gets created
on standbys and that it is archived.

The patch looks good overall. It applies cleanly and passes all tests.

One question. Do we want to create this file all the time (as written),
or only when archive_mode = always?

It appears that CleanupBackupHistory() will immediately remove the
history file when archive_mode != always so it seems useless to write
it. On the other hand the code is a bit simpler this way. Thoughts?

Regards,
--
-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 Tue, Sep 19, 2017 at 8:14 AM, David Steele <david@pgmasters.net> wrote:

On 8/31/17 11:56 PM, Michael Paquier wrote:

Here is an updated patch with refreshed documentation, as a result of
449338c which was discussed in thread
/messages/by-id/d4d951b9-89c0-6bc1-b6ff-d0b2dd5a8966@pgmasters.net.
I am just outlining the fact that a backup history file gets created
on standbys and that it is archived.

The patch looks good overall. It applies cleanly and passes all tests.

One question. Do we want to create this file all the time (as written),
or only when archive_mode = always?

It appears that CleanupBackupHistory() will immediately remove the
history file when archive_mode != always so it seems useless to write
it. On the other hand the code is a bit simpler this way. Thoughts?

With archive_mode = off, the bytes of the backup history file are
still written to disk, so my take on the matter is to keep the code
simple.
--
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 9/18/17 7:26 PM, Michael Paquier wrote:

On Tue, Sep 19, 2017 at 8:14 AM, David Steele <david@pgmasters.net> wrote:

On 8/31/17 11:56 PM, Michael Paquier wrote:

Here is an updated patch with refreshed documentation, as a result of
449338c which was discussed in thread
/messages/by-id/d4d951b9-89c0-6bc1-b6ff-d0b2dd5a8966@pgmasters.net.
I am just outlining the fact that a backup history file gets created
on standbys and that it is archived.

The patch looks good overall. It applies cleanly and passes all tests.

One question. Do we want to create this file all the time (as written),
or only when archive_mode = always?

It appears that CleanupBackupHistory() will immediately remove the
history file when archive_mode != always so it seems useless to write
it. On the other hand the code is a bit simpler this way. Thoughts?

With archive_mode = off, the bytes of the backup history file are
still written to disk, so my take on the matter is to keep the code
simple.

I'm OK with that.

I'll give Masahiko some time to respond before marking it RFC.

Regards,
--
-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 Tue, Sep 19, 2017 at 8:33 AM, David Steele <david@pgmasters.net> wrote:

On 9/18/17 7:26 PM, Michael Paquier wrote:

On Tue, Sep 19, 2017 at 8:14 AM, David Steele <david@pgmasters.net> wrote:

On 8/31/17 11:56 PM, Michael Paquier wrote:

Here is an updated patch with refreshed documentation, as a result of
449338c which was discussed in thread
/messages/by-id/d4d951b9-89c0-6bc1-b6ff-d0b2dd5a8966@pgmasters.net.
I am just outlining the fact that a backup history file gets created
on standbys and that it is archived.

The patch looks good overall. It applies cleanly and passes all tests.

One question. Do we want to create this file all the time (as written),
or only when archive_mode = always?

It appears that CleanupBackupHistory() will immediately remove the
history file when archive_mode != always so it seems useless to write
it. On the other hand the code is a bit simpler this way. Thoughts?

With archive_mode = off, the bytes of the backup history file are
still written to disk, so my take on the matter is to keep the code
simple.

I'm OK with that.

I'll give Masahiko some time to respond before marking it RFC.

Thanks, I'll review it and send a comment by this Wednesday.

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

On Tue, Sep 19, 2017 at 2:48 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Sep 19, 2017 at 8:33 AM, David Steele <david@pgmasters.net> wrote:

On 9/18/17 7:26 PM, Michael Paquier wrote:

On Tue, Sep 19, 2017 at 8:14 AM, David Steele <david@pgmasters.net> wrote:

On 8/31/17 11:56 PM, Michael Paquier wrote:

Here is an updated patch with refreshed documentation, as a result of
449338c which was discussed in thread
/messages/by-id/d4d951b9-89c0-6bc1-b6ff-d0b2dd5a8966@pgmasters.net.
I am just outlining the fact that a backup history file gets created
on standbys and that it is archived.

The patch looks good overall. It applies cleanly and passes all tests.

One question. Do we want to create this file all the time (as written),
or only when archive_mode = always?

It appears that CleanupBackupHistory() will immediately remove the
history file when archive_mode != always so it seems useless to write
it. On the other hand the code is a bit simpler this way. Thoughts?

With archive_mode = off, the bytes of the backup history file are
still written to disk, so my take on the matter is to keep the code
simple.

I'm OK with that.

I'll give Masahiko some time to respond before marking it RFC.

Thanks, I'll review it and send a comment by this Wednesday.

I've reviewed and tested the latest patch but fond no problems, so
I've marked this patch to Ready for Committer.

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

On 19 September 2017 at 00:33, David Steele <david@pgmasters.net> wrote:

On 9/18/17 7:26 PM, Michael Paquier wrote:

On Tue, Sep 19, 2017 at 8:14 AM, David Steele <david@pgmasters.net> wrote:

On 8/31/17 11:56 PM, Michael Paquier wrote:

Here is an updated patch with refreshed documentation, as a result of
449338c which was discussed in thread
/messages/by-id/d4d951b9-89c0-6bc1-b6ff-d0b2dd5a8966@pgmasters.net.
I am just outlining the fact that a backup history file gets created
on standbys and that it is archived.

The patch looks good overall. It applies cleanly and passes all tests.

One question. Do we want to create this file all the time (as written),
or only when archive_mode = always?

It appears that CleanupBackupHistory() will immediately remove the
history file when archive_mode != always so it seems useless to write
it. On the other hand the code is a bit simpler this way. Thoughts?

With archive_mode = off, the bytes of the backup history file are
still written to disk, so my take on the matter is to keep the code
simple.

I'm OK with that.

I'm not.

If we want to do this why not only do it in the modes that have meaning?
i.e. put an if() test in for archive_mode == always

Which also makes it a smaller and clearer patch

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jan 5, 2018 at 11:27 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

I'm not.

If we want to do this why not only do it in the modes that have meaning?
i.e. put an if() test in for archive_mode == always.

OK, I can see value in your point as well. The check is a bit more
complicated that just looking for archive_mode == always though, you
need to look at if the backup has been started in recovery or not, and
then adapt using XLogArchivingActive() and XLogArchivingAlways(),
similarly to what is done when waiting for the archives.

Which also makes it a smaller and clearer patch

Yes, this generates less diffs, reducing the likelihood of bugs. What
do you think about the v3 attached?
--
Michael

On 1/6/18 3:48 AM, Michael Paquier wrote:

On Fri, Jan 5, 2018 at 11:27 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

Which also makes it a smaller and clearer patch

Yes, this generates less diffs, reducing the likelihood of bugs. What
do you think about the v3 attached?

I agree that this is a cleaner solution.

--
-David
david@pgmasters.net

On Sun, Jan 7, 2018 at 1:35 AM, David Steele <david@pgmasters.net> wrote:

On 1/6/18 3:48 AM, Michael Paquier wrote:

On Fri, Jan 5, 2018 at 11:27 PM, Simon Riggs <simon@2ndquadrant.com>
wrote:

Which also makes it a smaller and clearer patch

Yes, this generates less diffs, reducing the likelihood of bugs. What
do you think about the v3 attached?

I agree that this is a cleaner solution.

+1. And the changes looks good to me.

Regards,

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

On Thu, Jan 11, 2018 at 07:10:50PM +0900, Masahiko Sawada wrote:

On Sun, Jan 7, 2018 at 1:35 AM, David Steele <david@pgmasters.net> wrote:

On 1/6/18 3:48 AM, Michael Paquier wrote:

On Fri, Jan 5, 2018 at 11:27 PM, Simon Riggs <simon@2ndquadrant.com>
wrote:

Which also makes it a smaller and clearer patch

Yes, this generates less diffs, reducing the likelihood of bugs. What
do you think about the v3 attached?

I agree that this is a cleaner solution.

+1. And the changes looks good to me.

Cool. Thanks for the feedback.
--
Michael

On Thu, Jan 11, 2018 at 09:47:42PM +0900, Michael Paquier wrote:

Cool. Thanks for the feedback.

The last patch still applies, but no committer has been interested, so I
am moving my patch to the next CF.
--
Michael

On Thu, Jan 11, 2018 at 9:47 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Jan 11, 2018 at 07:10:50PM +0900, Masahiko Sawada wrote:

On Sun, Jan 7, 2018 at 1:35 AM, David Steele <david@pgmasters.net> wrote:

On 1/6/18 3:48 AM, Michael Paquier wrote:

On Fri, Jan 5, 2018 at 11:27 PM, Simon Riggs <simon@2ndquadrant.com>
wrote:

Which also makes it a smaller and clearer patch

Yes, this generates less diffs, reducing the likelihood of bugs. What
do you think about the v3 attached?

I agree that this is a cleaner solution.

+1. And the changes looks good to me.

The patch basically looks good to me. Here are some small comments.

<para>
The backup history file is not created in the database cluster backed up.
</para>

The above should be deleted in pg_basebackup.sgml.

* During recovery, since we don't use the end-of-backup WAL
* record and don't write the backup history file, the

This comment needs to be updated in xlog.c.

Regards,

--
Fujii Masao

On Fri, Feb 02, 2018 at 12:47:26AM +0900, Fujii Masao wrote:

The patch basically looks good to me. Here are some small comments.

<para>
The backup history file is not created in the database cluster backed up.
</para>

The above should be deleted in pg_basebackup.sgml.

* During recovery, since we don't use the end-of-backup WAL
* record and don't write the backup history file, the

This comment needs to be updated in xlog.c.

Thanks Fujii-san for the review. Indeed those portions need a refresh..
--
Michael

On Fri, Feb 2, 2018 at 2:06 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Feb 02, 2018 at 12:47:26AM +0900, Fujii Masao wrote:

The patch basically looks good to me. Here are some small comments.

<para>
The backup history file is not created in the database cluster backed up.
</para>

The above should be deleted in pg_basebackup.sgml.

* During recovery, since we don't use the end-of-backup WAL
* record and don't write the backup history file, the

This comment needs to be updated in xlog.c.

Thanks Fujii-san for the review. Indeed those portions need a refresh..

Thanks for updating the patch!

+ * write a backup history file with the same name.

So more than one backup history files with the same name
but the diffferent content can be created and archived.
Isn't this problematic because the backup history file that
users want to use later might be overwritten unexpectedly?

Regards,

--
Fujii Masao

Hi!

On 2018-03-02 02:29:13 +0900, Fujii Masao wrote:

On Fri, Feb 2, 2018 at 2:06 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Feb 02, 2018 at 12:47:26AM +0900, Fujii Masao wrote:

The patch basically looks good to me. Here are some small comments.

<para>
The backup history file is not created in the database cluster backed up.
</para>

The above should be deleted in pg_basebackup.sgml.

* During recovery, since we don't use the end-of-backup WAL
* record and don't write the backup history file, the

This comment needs to be updated in xlog.c.

Thanks Fujii-san for the review. Indeed those portions need a refresh..

Thanks for updating the patch!

You'd claimed this patch as a committer. You're still planning to commit
it?

+ * write a backup history file with the same name.

So more than one backup history files with the same name
but the diffferent content can be created and archived.
Isn't this problematic because the backup history file that
users want to use later might be overwritten unexpectedly?

Michael?

Greetings,

Andres Freund