Thread: [HACKERS] Creating backup history files for backups taken from standbys
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: https://www.postgresql.org/message-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? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Re: [HACKERS] Creating backup history files for backups taken from standbys
From
Masahiko Sawada
Date:
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: > https://www.postgresql.org/message-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
Re: [HACKERS] Creating backup history files for backups taken from standbys
From
Michael Paquier
Date:
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
Re: [HACKERS] Creating backup history files for backups taken from standbys
From
Masahiko Sawada
Date:
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
Re: [HACKERS] Creating backup history files for backups taken from standbys
From
Michael Paquier
Date:
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 https://www.postgresql.org/message-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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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 > https://www.postgresql.org/message-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
Re: [HACKERS] Creating backup history files for backups taken from standbys
From
Michael Paquier
Date:
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 >> https://www.postgresql.org/message-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 >>> https://www.postgresql.org/message-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
Re: [HACKERS] Creating backup history files for backups taken from standbys
From
Masahiko Sawada
Date:
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 >>>> https://www.postgresql.org/message-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
Re: [HACKERS] Creating backup history files for backups taken from standbys
From
Masahiko Sawada
Date:
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 >>>>> https://www.postgresql.org/message-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 >>>> https://www.postgresql.org/message-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
Re: [HACKERS] Creating backup history files for backups taken from standbys
From
Michael Paquier
Date:
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
Attachment
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
Re: [HACKERS] Creating backup history files for backups taken from standbys
From
Masahiko Sawada
Date:
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
Re: [HACKERS] Creating backup history files for backups taken fromstandbys
From
Michael Paquier
Date:
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
Attachment
Re: [HACKERS] Creating backup history files for backups taken fromstandbys
From
Michael Paquier
Date:
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
Attachment
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
Re: [HACKERS] Creating backup history files for backups taken fromstandbys
From
Michael Paquier
Date:
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
Attachment
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
Re: [HACKERS] Creating backup history files for backups taken fromstandbys
From
Michael Paquier
Date:
On Fri, Mar 02, 2018 at 02:29:13AM +0900, Fujii Masao wrote: > + * 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? Yeah, that's the intention behind the patch. Would that actually happen in practice though? We would talk about two backups running simultaneously on a standby, which would overlap with each other to generate a file aimed only at being helpful for debugging purposes, and we provide no information now for backups taken from standbys. We could of course make that logic a bit smarter by checking if there is an extsing file with the same name and create a new file with a different name. But is that worth the complication? That's where I am not convinced, and that's the reason why this patch is doing things this way. -- Michael
Attachment
Re: [HACKERS] Creating backup history files for backups taken fromstandbys
From
Michael Paquier
Date:
On Thu, Mar 01, 2018 at 07:26:09PM -0800, Andres Freund wrote: > On 2018-03-02 02:29:13 +0900, Fujii Masao wrote: >> + * 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? Just answered to that question. -- Michael
Attachment
On 3/1/18 11:07 PM, Michael Paquier wrote: > On Fri, Mar 02, 2018 at 02:29:13AM +0900, Fujii Masao wrote: >> + * 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? > > Yeah, that's the intention behind the patch. Would that actually happen > in practice though? We would talk about two backups running > simultaneously on a standby, which would overlap with each other to > generate a file aimed only at being helpful for debugging purposes, and > we provide no information now for backups taken from standbys. We could > of course make that logic a bit smarter by checking if there is an > extsing file with the same name and create a new file with a different > name. But is that worth the complication? That's where I am not > convinced, and that's the reason why this patch is doing things this > way. +1. Given that the history files are not used during restore and are present primarily for debugging purposes, I can't see worrying too much about this unlikely (if possible) race condition. -- -David david@pgmasters.net
Attachment
On Fri, Mar 2, 2018 at 1:07 PM, Michael Paquier <michael@paquier.xyz> wrote: > On Fri, Mar 02, 2018 at 02:29:13AM +0900, Fujii Masao wrote: >> + * 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? > > Yeah, that's the intention behind the patch. Would that actually happen > in practice though? Yes, I think. During recovery, all the pg_basebackup would use the same backup starting point and create the backup history file with the same name until the next restartpoint runs. So unless the restartpoint interval is short, ISTM that such a problematic case can happen even in practice. No? > We would talk about two backups running > simultaneously on a standby, which would overlap with each other to > generate a file aimed only at being helpful for debugging purposes, and > we provide no information now for backups taken from standbys. We could > of course make that logic a bit smarter by checking if there is an > extsing file with the same name and create a new file with a different > name. But is that worth the complication? That's where I am not > convinced, and that's the reason why this patch is doing things this > way. What about including the backup history file in the base backup instead of creating it in the standby's pg_wal and archiving it? As the good side effect of this approach, we can use the backup history file for debugging purpose even when WAL archiving is not enabled. Regards, -- Fujii Masao
On Fri, Mar 2, 2018 at 12:26 PM, Andres Freund <andres@anarazel.de> wrote: > 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? Yes. Thanks for the ping! Regards, -- Fujii Masao
Hi, On 3/2/18 1:03 PM, Fujii Masao wrote: > On Fri, Mar 2, 2018 at 1:07 PM, Michael Paquier <michael@paquier.xyz> wrote: > >> We would talk about two backups running >> simultaneously on a standby, which would overlap with each other to >> generate a file aimed only at being helpful for debugging purposes, and >> we provide no information now for backups taken from standbys. We could >> of course make that logic a bit smarter by checking if there is an >> extsing file with the same name and create a new file with a different >> name. But is that worth the complication? That's where I am not >> convinced, and that's the reason why this patch is doing things this >> way. > > What about including the backup history file in the base backup instead of > creating it in the standby's pg_wal and archiving it? As the good side effect > of this approach, we can use the backup history file for debugging purpose > even when WAL archiving is not enabled. I don't think this is a good idea, even if it does solve the race condition. First, it would be different from the way primary-style backups generate this file. Second, these files would not be excluded from future restore / backup cycles so they could build up after a while and cause confusion. I guess we could teach PG to delete them or pg_basebackup to ignore them, but that doesn't seem worth the effort. Regards, -- -David david@pgmasters.net
Re: [HACKERS] Creating backup history files for backups taken fromstandbys
From
Michael Paquier
Date:
On Fri, Mar 02, 2018 at 03:41:57PM -0500, David Steele wrote: > On 3/2/18 1:03 PM, Fujii Masao wrote: >> On Fri, Mar 2, 2018 at 1:07 PM, Michael Paquier <michael@paquier.xyz> wrote: >>> We would talk about two backups running >>> simultaneously on a standby, which would overlap with each other to >>> generate a file aimed only at being helpful for debugging purposes, and >>> we provide no information now for backups taken from standbys. We could >>> of course make that logic a bit smarter by checking if there is an >>> extsing file with the same name and create a new file with a different >>> name. But is that worth the complication? That's where I am not >>> convinced, and that's the reason why this patch is doing things this >>> way. Sorry for my late reply, I was thinking more about the whole thing. >> What about including the backup history file in the base backup instead of >> creating it in the standby's pg_wal and archiving it? As the good side effect >> of this approach, we can use the backup history file for debugging purpose >> even when WAL archiving is not enabled. That's not going to be much helpful for tar'ed base backups as the backup history file would be at the end of the stream :( For a debug file, that's not really helpful. > I don't think this is a good idea, even if it does solve the race > condition. First, it would be different from the way primary-style > backups generate this file. Second, these files would not be excluded > from future restore / backup cycles so they could build up after a while > and cause confusion. I guess we could teach PG to delete them or > pg_basebackup to ignore them, but that doesn't seem worth the effort. Something I did not consider though is that this causes archive commands to choke on those identical file names, so any archive command which is not able to handle that would cause WAL to bloat on the standby. For this reason I would rather drop the current approach taken. Remember that the docs tell to use a plain "cp" for example, so this would cause standbys to go silently down. Something that I would find way more portable though is the addition of the backup history file in the output of pg_stop_backup(), which would map as well with what Fujii-san's idea to add this file to the backup data itself, and do the same for backups taken on a primary. However this would mean moving the 'c' marker marking the end of the tar stream within do_pg_stop_backup(), and I am in no way a fan of that: the handling of the tar stream should be out of reach of the start and stop phases. Another idea that I have here as well would be to change a bit the history backup file name so as the backup name is added to it, but that does not fly high as pg_basebackup uses its own name with spaces (Yeah!), or even better the PID of the backend taking the backup. The renaming is more appealing, still not perfect. So my mood of the day would be to just drop the patch entirely. -- Michael
Attachment
On 3/5/18 1:06 AM, Michael Paquier wrote: > On Fri, Mar 02, 2018 at 03:41:57PM -0500, David Steele wrote: >> On 3/2/18 1:03 PM, Fujii Masao wrote: >>> On Fri, Mar 2, 2018 at 1:07 PM, Michael Paquier <michael@paquier.xyz> wrote: >>>> We would talk about two backups running >>>> simultaneously on a standby, which would overlap with each other to >>>> generate a file aimed only at being helpful for debugging purposes, and >>>> we provide no information now for backups taken from standbys. We could >>>> of course make that logic a bit smarter by checking if there is an >>>> extsing file with the same name and create a new file with a different >>>> name. But is that worth the complication? That's where I am not >>>> convinced, and that's the reason why this patch is doing things this >>>> way. > > Sorry for my late reply, I was thinking more about the whole thing. > >>> What about including the backup history file in the base backup instead of >>> creating it in the standby's pg_wal and archiving it? As the good side effect >>> of this approach, we can use the backup history file for debugging purpose >>> even when WAL archiving is not enabled. > > That's not going to be much helpful for tar'ed base backups as the > backup history file would be at the end of the stream :( For a debug > file, that's not really helpful. > >> I don't think this is a good idea, even if it does solve the race >> condition. First, it would be different from the way primary-style >> backups generate this file. Second, these files would not be excluded >> from future restore / backup cycles so they could build up after a while >> and cause confusion. I guess we could teach PG to delete them or >> pg_basebackup to ignore them, but that doesn't seem worth the effort. > > Something I did not consider though is that this causes archive commands > to choke on those identical file names, so any archive command which is > not able to handle that would cause WAL to bloat on the standby. For > this reason I would rather drop the current approach taken. Remember > that the docs tell to use a plain "cp" for example, so this would cause > standbys to go silently down. That's a good point. The pgBackRest archive command would definitely not like this. > Something that I would find way more portable though is the addition of > the backup history file in the output of pg_stop_backup(), which would > map as well with what Fujii-san's idea to add this file to the backup > data itself, and do the same for backups taken on a primary. However > this would mean moving the 'c' marker marking the end of the tar stream > within do_pg_stop_backup(), and I am in no way a fan of that: the > handling of the tar stream should be out of reach of the start and stop > phases. If the file is stored with the backup instead of the archive then I don't think it adds much. Why not just add stop time and stop LSN to backup_label and get rid of the history file entirely. I've been working closely with backup for nearly five years now and I've need had need to look at a .backup file. Other than writing code to handle it in archiving, I've barely given it a thought. > Another idea that I have here as well would be to change a bit the > history backup file name so as the backup name is added to it, but that > does not fly high as pg_basebackup uses its own name with spaces > (Yeah!), or even better the PID of the backend taking the backup. This could work, but seems complicated for little benefit. It would also require some archive commands to be updated to understand the new format. > The renaming is more appealing, still not perfect. So my mood of the > day would be to just drop the patch entirely. I think that's the best idea for now. It doesn't seem worth using cycles in the last CF coming up with a new approach. Regards, -- -David david@pgmasters.net
Attachment
On Mon, Mar 5, 2018 at 11:01 PM, David Steele <david@pgmasters.net> wrote: > On 3/5/18 1:06 AM, Michael Paquier wrote: >> On Fri, Mar 02, 2018 at 03:41:57PM -0500, David Steele wrote: >>> On 3/2/18 1:03 PM, Fujii Masao wrote: >>>> On Fri, Mar 2, 2018 at 1:07 PM, Michael Paquier <michael@paquier.xyz> wrote: >>>>> We would talk about two backups running >>>>> simultaneously on a standby, which would overlap with each other to >>>>> generate a file aimed only at being helpful for debugging purposes, and >>>>> we provide no information now for backups taken from standbys. We could >>>>> of course make that logic a bit smarter by checking if there is an >>>>> extsing file with the same name and create a new file with a different >>>>> name. But is that worth the complication? That's where I am not >>>>> convinced, and that's the reason why this patch is doing things this >>>>> way. >> >> Sorry for my late reply, I was thinking more about the whole thing. >> >>>> What about including the backup history file in the base backup instead of >>>> creating it in the standby's pg_wal and archiving it? As the good side effect >>>> of this approach, we can use the backup history file for debugging purpose >>>> even when WAL archiving is not enabled. >> >> That's not going to be much helpful for tar'ed base backups as the >> backup history file would be at the end of the stream :( For a debug >> file, that's not really helpful. >> >>> I don't think this is a good idea, even if it does solve the race >>> condition. First, it would be different from the way primary-style >>> backups generate this file. Second, these files would not be excluded >>> from future restore / backup cycles so they could build up after a while >>> and cause confusion. I guess we could teach PG to delete them or >>> pg_basebackup to ignore them, but that doesn't seem worth the effort. >> >> Something I did not consider though is that this causes archive commands >> to choke on those identical file names, so any archive command which is >> not able to handle that would cause WAL to bloat on the standby. For >> this reason I would rather drop the current approach taken. Remember >> that the docs tell to use a plain "cp" for example, so this would cause >> standbys to go silently down. > > That's a good point. The pgBackRest archive command would definitely > not like this. > >> Something that I would find way more portable though is the addition of >> the backup history file in the output of pg_stop_backup(), which would >> map as well with what Fujii-san's idea to add this file to the backup >> data itself, and do the same for backups taken on a primary. However >> this would mean moving the 'c' marker marking the end of the tar stream >> within do_pg_stop_backup(), and I am in no way a fan of that: the >> handling of the tar stream should be out of reach of the start and stop >> phases. > > If the file is stored with the backup instead of the archive then I > don't think it adds much. Why not just add stop time and stop LSN to > backup_label and get rid of the history file entirely. > > I've been working closely with backup for nearly five years now and I've > need had need to look at a .backup file. Other than writing code to > handle it in archiving, I've barely given it a thought. > >> Another idea that I have here as well would be to change a bit the >> history backup file name so as the backup name is added to it, but that >> does not fly high as pg_basebackup uses its own name with spaces >> (Yeah!), or even better the PID of the backend taking the backup. > > This could work, but seems complicated for little benefit. It would > also require some archive commands to be updated to understand the new > format. > >> The renaming is more appealing, still not perfect. So my mood of the >> day would be to just drop the patch entirely. > > I think that's the best idea for now. It doesn't seem worth using > cycles in the last CF coming up with a new approach. I have no objection to mark the patch "returned with feedback". Regards, -- Fujii Masao
Re: [HACKERS] Creating backup history files for backups taken fromstandbys
From
Michael Paquier
Date:
On Tue, Mar 06, 2018 at 02:23:19AM +0900, Fujii Masao wrote: > I have no objection to mark the patch "returned with feedback". Yes I have done so, that's way too late. -- Michael