Обсуждение: [HACKERS] Update low-level backup documentation to match actual behavior
As discussed in [1] our low-level backup documentation does not quite match the actual behavior of the functions on primary vs. standby. Since it appears we have decided that the remaining behavioral differences after 52f8a59dd953c68 are bugs in the documentation, the attached is a first pass at bringing the documentation up to date. The biggest change is to recognize that exclusive backups can only be run on a primary and to adjust the text accordingly. Also, I did not mention the wait_for_archive param in the exclusive instructions since they are deprecated. This patch should be sufficient for 10/11 but will need some minor changes for 9.6 to remove the reference to wait_for_archive. Note that this patch ignores Michael's patch [2] to create WAL history files on a standby as this will likely only be applied to master. In addition, I have formatted the text to produce minimal diffs for review, but it could be tightened up before commit. -- -David david@pgmasters.net [1] https://www.postgresql.org/message-id/20170814152816.GF4628%40tamriel.snowman.net [2] https://www.postgresql.org/message-id/CAB7nPqQvVpMsqJExSVXHUwpXFRwojsb-jb4BYnxEQbjJLfw-yQ%40mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
- the next WAL segment. The reason for the switch is to arrange for + the next WAL segment when run on a primary. On a standby you can call + <function>pg_switch_wal</function> on the primary to perform a manual + switch. + The reason for the switch is to arrange for Tacking on "when run on a primary" onto the end of the existing sentence is a little ambiguous: does that clause apply only to the last part, or to the whole sentence? I suggest something like: This terminates the backup mode. On a primary, it also performs an automatic switch to the next WAL segment. On a standby, it is not possible to automatically switch WAL segments, so you may wish to consider running <function>pg_switch_wal</function> on the primary to perform a manual switch. - backup files. If <varname>archive_mode</> is enabled, + backup files. On a primary, if <varname>archive_mode</> is enabled and the + <literal>wait_for_archive</> parameter is <literal>true</>, <function>pg_stop_backup</> does not return until thelast segment has been archived. + On a standby, <varname>archive_mode</> must be <literal>always</> in order + for <function>pg_stop_backup</> to wait. Looks good. - <title>Making an exclusive low level backup</title> + <title>Making an exclusive low level backup on a primary</title> I'd omit this hunk. - more than one concurrent backup to run, and there can be some issues on + more than one concurrent backup to run, must be run on a primary, and there + can be some issues on Maybe this would be clearer: This type of backup can only be taken on a primary, does not allow more than one ... - This function, when called on a primary, terminates the backup mode and + This function terminates the backup mode and performs an automatic switch to the next WAL segment. The reason forthe switch is to arrange for the last WAL segment written during the backup - interval to be ready to archive. When called on a standby, this function - only terminates backup mode. A subsequent WAL segment switch will be - needed in order to ensure that all WAL files needed to restore the backup - can be archived; if the primary does not have sufficient write activity - to trigger one, <function>pg_switch_wal</function> should be executed on - the primary. + interval to be ready to archive. Why do you want to delete all that text? It seems like good text to me. - The function also creates a backup history file in the write-ahead log + When executed on a primary, the function also creates a backup history file + in the write-ahead log Looks good. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, Thanks for reviewing! On 8/18/17 2:45 PM, Robert Haas wrote: > - the next WAL segment. The reason for the switch is to arrange for > + the next WAL segment when run on a primary. On a standby you can call > + <function>pg_switch_wal</function> on the primary to perform a manual > + switch. > + The reason for the switch is to arrange for > > Tacking on "when run on a primary" onto the end of the existing > sentence is a little ambiguous: does that clause apply only to the > last part, or to the whole sentence? I suggest something like: This > terminates the backup mode. On a primary, it also performs an > automatic switch to the next WAL segment. On a standby, it is not > possible to automatically switch WAL segments, so you may wish to > consider running <function>pg_switch_wal</function> on the primary to > perform a manual switch. Looks good. > > - <title>Making an exclusive low level backup</title> > + <title>Making an exclusive low level backup on a primary</title> > > I'd omit this hunk. OK, but I was trying to make it very clear that this backup method only works on a primary. If you think the text is in the first paragraph is enough then I'm willing to go with that, though. > - more than one concurrent backup to run, and there can be some issues on > + more than one concurrent backup to run, must be run on a > primary, and there > + can be some issues on > > Maybe this would be clearer: This type of backup can only be taken on > a primary, does not allow more than one ... Looks good. > - This function, when called on a primary, terminates the backup mode and > + This function terminates the backup mode and > performs an automatic switch to the next WAL segment. The reason for the > switch is to arrange for the last WAL segment written during the backup > - interval to be ready to archive. When called on a standby, this function > - only terminates backup mode. A subsequent WAL segment switch will be > - needed in order to ensure that all WAL files needed to restore the backup > - can be archived; if the primary does not have sufficient write activity > - to trigger one, <function>pg_switch_wal</function> should be executed on > - the primary. > + interval to be ready to archive. > > Why do you want to delete all that text? It seems like good text to me. Since the exclusive method only works on a primary... -- -David david@pgmasters.net
On Fri, Aug 18, 2017 at 2:58 PM, David Steele <david@pgmasters.net> wrote: > OK, but I was trying to make it very clear that this backup method only > works on a primary. If you think the text is in the first paragraph is > enough then I'm willing to go with that, though. Yeah, I think the text is enough. > Since the exclusive method only works on a primary... Oh, right. Duh. If you update the patch I'll apply it to 11 and 10. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 8/18/17 3:00 PM, Robert Haas wrote: > > If you update the patch I'll apply it to 11 and 10. Attached is the updated patch. I didn't like the vague "there can be some issues on the server if it crashes during the backup" so I added a new paragraph at the appropriate step to give a more detailed explanation of the problem. Thanks, -- -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] Update low-level backup documentation to match actual behavior
От
Michael Paquier
Дата:
On Fri, Aug 18, 2017 at 3:35 AM, David Steele <david@pgmasters.net> wrote: > This patch should be sufficient for 10/11 but will need some minor > changes for 9.6 to remove the reference to wait_for_archive. Note that > this patch ignores Michael's patch [2] to create WAL history files on a > standby as this will likely only be applied to master. I'll just rebase as needed the other patch, this documentation update is very important in itself, so let's not worry about that. On Sat, Aug 19, 2017 at 7:46 AM, David Steele <david@pgmasters.net> wrote: > On 8/18/17 3:00 PM, Robert Haas wrote: >> >> If you update the patch I'll apply it to 11 and 10. > > Attached is the updated patch. > > I didn't like the vague "there can be some issues on the server if it > crashes during the backup" so I added a new paragraph at the appropriate > step to give a more detailed explanation of the problem. Thanks for the patch. - This terminates the backup mode and performs an automatic switch to - the next WAL segment. The reason for the switch is to arrange for + This terminates backup mode. On a primary, it also performs an automatic + switch to the next WAL segment. On a standby, it is not possible to + automatically switch WAL segments, so you may wish to run + <function>pg_switch_wal</function> on the primary to perform a manual + switch. The reason for the switch is to arrange for the last WAL segment file written during the backup intervalto be ready to archive. [...] - backup files. If <varname>archive_mode</> is enabled, + backup files. On a primary, if <varname>archive_mode</> is enabled and the + <literal>wait_for_archive</> parameter is <literal>true</>, <function>pg_stop_backup</> does not return until thelast segment has been archived. + On a standby, <varname>archive_mode</> must be <literal>always</> in order + for <function>pg_stop_backup</> to wait. This level of details is good to have. Thanks. + Prior to PostgreSQL 9.6, this Markup <productname>? + Note well that if the server crashes during the backup it may not be + possible to restart until the <literal>backup_label</> file has been + manually deleted from the PGDATA directory. Missing a markup <envvar> here for PGDATA. s/Note well/Note as well/, no? - This function, when called on a primary, terminates the backup mode and + This function terminates backup mode and performs an automatic switch to the next WAL segment. The reason for the switch is to arrange for the last WAL segment written during the backup - interval to be ready to archive. When called on a standby, this function - only terminates backup mode. A subsequent WAL segment switch will be - needed in order to ensure that all WAL files needed to restore the backup - can be archived; if the primary does not have sufficient write activity - to trigger one, <function>pg_switch_wal</function> should be executed on - the primary. + interval to be ready to archive. pg_stop_backup() still waits for the last WAL segment to be archived on the primary. Perhaps we'd want to mention that? Documentation does not state yet that the use of low-level APIs for exclusive backups are not supported on standbys. Now in the docs: If the backup process monitors and ensures that all WAL segment files required for the backup aresuccessfully archived then the second parameter (which defaults to true) can be set to false to have I would recommend adding some details here and mention "wait_for_archive" instead of "second parameter". I am wondering as well if this paragraph should be put in red with a warning or something like that. This is really, really important to ensure consistent backups! -- Michael
Hi Michael, Thanks for reviewing! Sorry for the late response, those eclipses don't just chase themselves... On 8/20/17 10:22 PM, Michael Paquier wrote: > On Fri, Aug 18, 2017 at 3:35 AM, David Steele <david@pgmasters.net> wrote: > > + Prior to PostgreSQL 9.6, this > Markup <productname>? Fixed. > + Note well that if the server crashes during the backup it may not be > + possible to restart until the <literal>backup_label</> file has been > + manually deleted from the PGDATA directory. > Missing a markup <envvar> here for PGDATA. Fixed. > s/Note well/Note as well/, no? This was a literal translation of nota bene but I've changed it to simply "Note" as that seems common in the docs. > - This function, when called on a primary, terminates the backup mode and > + This function terminates backup mode and > performs an automatic switch to the next WAL segment. The reason for the > switch is to arrange for the last WAL segment written during the backup > - interval to be ready to archive. When called on a standby, this function > - only terminates backup mode. A subsequent WAL segment switch will be > - needed in order to ensure that all WAL files needed to restore the backup > - can be archived; if the primary does not have sufficient write activity > - to trigger one, <function>pg_switch_wal</function> should be executed on > - the primary. > + interval to be ready to archive. > pg_stop_backup() still waits for the last WAL segment to be archived > on the primary. Perhaps we'd want to mention that? That's detailed in the next paragraph, ISTM. > Documentation does not state yet that the use of low-level APIs for > exclusive backups are not supported on standbys. The first paragraph of the exclusive section states, "this type of backup can only be taken on a primary". > Now in the docs: > If the backup process monitors and ensures that all WAL segment files > required for the backup are successfully archived then the second > parameter (which defaults to true) can be set to false to have > I would recommend adding some details here and mention > "wait_for_archive" instead of "second parameter". Done. > I am wondering as > well if this paragraph should be put in red with a warning or > something like that. This is really, really important to ensure > consistent backups! Maybe, but this logic could easily apply to a lot of sections in the backup docs. I'm not sure where it would end. Thanks! -- -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] Update low-level backup documentation to match actual behavior
От
Michael Paquier
Дата:
On Thu, Aug 24, 2017 at 10:49 PM, David Steele <david@pgmasters.net> wrote: > Thanks for reviewing! Sorry for the late response, those eclipses don't > just chase themselves... That's quite something to see. > On 8/20/17 10:22 PM, Michael Paquier wrote: >> On Fri, Aug 18, 2017 at 3:35 AM, David Steele <david@pgmasters.net> wrote: >> >> + Prior to PostgreSQL 9.6, this >> Markup <productname>? > > Fixed. > >> + Note well that if the server crashes during the backup it may not be >> + possible to restart until the <literal>backup_label</> file has been >> + manually deleted from the PGDATA directory. >> Missing a markup <envvar> here for PGDATA. > > Fixed. > >> s/Note well/Note as well/, no? > > This was a literal translation of nota bene but I've changed it to > simply "Note" as that seems common in the docs. Oh, OK. >> Documentation does not state yet that the use of low-level APIs for >> exclusive backups are not supported on standbys. > > The first paragraph of the exclusive section states, "this type of > backup can only be taken on a primary". Sorry, missed that. >> Now in the docs: >> If the backup process monitors and ensures that all WAL segment files >> required for the backup are successfully archived then the second >> parameter (which defaults to true) can be set to false to have >> I would recommend adding some details here and mention >> "wait_for_archive" instead of "second parameter". > > Done. > >> I am wondering as >> well if this paragraph should be put in red with a warning or >> something like that. This is really, really important to ensure >> consistent backups! > > Maybe, but this logic could easily apply to a lot of sections in the > backup docs. I'm not sure where it would end. True as well. The patch looks good to me. If a committer does not show up soon, it may be better to register that in the CF and wait. I am not sure that adding an open item is suited, as docs have the same problem on 9.6. -- Michael
On 8/24/17 7:36 PM, Michael Paquier wrote: > > True as well. The patch looks good to me. If a committer does not show > up soon, it may be better to register that in the CF and wait. I am > not sure that adding an open item is suited, as docs have the same > problem on 9.6. Robert said he would commit this so I expect he'll do that if he doesn't have any objections to the changes. Robert, if you would prefer me to submit this to the CF I'm happy to do so. Thanks, -- -David david@pgmasters.net
On Fri, Aug 25, 2017 at 3:10 PM, David Steele <david@pgmasters.net> wrote: > On 8/24/17 7:36 PM, Michael Paquier wrote: >> >> True as well. The patch looks good to me. If a committer does not show >> up soon, it may be better to register that in the CF and wait. I am >> not sure that adding an open item is suited, as docs have the same >> problem on 9.6. > > Robert said he would commit this so I expect he'll do that if he doesn't > have any objections to the changes. > > Robert, if you would prefer me to submit this to the CF I'm happy to do so. Ha, this note arrived just as I was working on getting this committed. I'll commit this to 11 and 10 presently; can you produce a version for 9.6? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 8/25/17 3:13 PM, Robert Haas wrote: > On Fri, Aug 25, 2017 at 3:10 PM, David Steele <david@pgmasters.net> wrote: >> >> Robert said he would commit this so I expect he'll do that if he doesn't >> have any objections to the changes. >> >> Robert, if you would prefer me to submit this to the CF I'm happy to do so. > > Ha, this note arrived just as I was working on getting this committed. > I'll commit this to 11 and 10 presently; can you produce a version for > 9.6? No problem. I'll base it on your commit to capture any changes you made. -- -David david@pgmasters.net
On Fri, Aug 25, 2017 at 3:21 PM, David Steele <david@pgmasters.net> wrote: > No problem. I'll base it on your commit to capture any changes you made. Thanks, but you incorporated everything I wanted in response to my first review -- so I didn't tweak it any further. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 8/25/17 3:26 PM, Robert Haas wrote: > On Fri, Aug 25, 2017 at 3:21 PM, David Steele <david@pgmasters.net> wrote: >> No problem. I'll base it on your commit to capture any changes you made. > > Thanks, but you incorporated everything I wanted in response to my > first review -- so I didn't tweak it any further. Thank you for committing that. I'll get the 9.6 patch to you early next week. -- -David david@pgmasters.net
Hi Robert, On 8/25/17 4:03 PM, David Steele wrote: > On 8/25/17 3:26 PM, Robert Haas wrote: >> On Fri, Aug 25, 2017 at 3:21 PM, David Steele <david@pgmasters.net> >> wrote: >>> No problem. I'll base it on your commit to capture any changes you >>> made. >> >> Thanks, but you incorporated everything I wanted in response to my >> first review -- so I didn't tweak it any further. > > Thank you for committing that. I'll get the 9.6 patch to you early next > week. Attached is the 9.6 patch. It required a bit more work in func.sgml than I was expecting so have a close look at that. The rest was mostly removing irrelevant hunks. Thanks, -- -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] Update low-level backup documentation to match actual behavior
От
Michael Paquier
Дата:
On Tue, Aug 29, 2017 at 10:59 PM, David Steele <david@pgmasters.net> wrote: > Hi Robert, > > On 8/25/17 4:03 PM, David Steele wrote: >> On 8/25/17 3:26 PM, Robert Haas wrote: >>> On Fri, Aug 25, 2017 at 3:21 PM, David Steele <david@pgmasters.net> >>> wrote: >>>> No problem. I'll base it on your commit to capture any changes you >>>> made. >>> >>> Thanks, but you incorporated everything I wanted in response to my >>> first review -- so I didn't tweak it any further. >> >> Thank you for committing that. I'll get the 9.6 patch to you early next >> week. > > Attached is the 9.6 patch. It required a bit more work in func.sgml > than I was expecting so have a close look at that. The rest was mostly > removing irrelevant hunks. + switch to the next WAL segment. On a standby, it is not possible to + automatically switch WAL segments, so you may wish to run + <function>pg_switch_wal</function> on the primary to perform a manual + switch. The reason for the switch is to arrange for [...] + WAL segments have been archived. If write activity on the primary is low, it + may be useful to run <function>pg_switch_wal</> on the primary in order to + trigger an immediate segment switch of the last required WAL It seems to me that both portions are wrong. There is no archiving wait on standbys for 9.6, and pg_stop_backup triggers by itself the segment switch, so saying that enforcing pg_switch_wal on the primary is moot. pg_switch_xlog has been renamed to pg_switch_wal in PG10, so the former name applies. -- Michael
Hi Michael, Thanks for reviewing! On 8/29/17 9:44 PM, Michael Paquier wrote: > On Tue, Aug 29, 2017 at 10:59 PM, David Steele <david@pgmasters.net> wrote: >> >> Attached is the 9.6 patch. It required a bit more work in func.sgml >> than I was expecting so have a close look at that. The rest was mostly >> removing irrelevant hunks. > > + switch to the next WAL segment. On a standby, it is not possible to > + automatically switch WAL segments, so you may wish to run > + <function>pg_switch_wal</function> on the primary to perform a manual > + switch. The reason for the switch is to arrange for > [...] > + WAL segments have been archived. If write activity on the primary > is low, it > + may be useful to run <function>pg_switch_wal</> on the primary in order to > + trigger an immediate segment switch of the last required WAL > It seems to me that both portions are wrong. There is no archiving > wait on standbys for 9.6, and I think its clearly stated here that pg_stop_backup() does not wait for WAL to archive on a standby. Even, it is very important for the backup routine to make sure that all the WAL *is* archived. > pg_stop_backup triggers by itself the > segment switch, so saying that enforcing pg_switch_wal on the primary > is moot. pg_stop_backup() does not perform a WAL switch on the standby which is what this sentence is referring to. I have separated this section out into a new paragraph to (hopefully) make it clearer. > pg_switch_xlog has been renamed to pg_switch_wal in PG10, so > the former name applies. Whoops! New patch is attached. Thanks, -- -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] Update low-level backup documentation to match actual behavior
От
Michael Paquier
Дата:
On Wed, Aug 30, 2017 at 8:02 PM, David Steele <david@pgmasters.net> wrote: > On 8/29/17 9:44 PM, Michael Paquier wrote: >> On Tue, Aug 29, 2017 at 10:59 PM, David Steele <david@pgmasters.net> wrote: >>> >>> Attached is the 9.6 patch. It required a bit more work in func.sgml >>> than I was expecting so have a close look at that. The rest was mostly >>> removing irrelevant hunks. >> >> + switch to the next WAL segment. On a standby, it is not possible to >> + automatically switch WAL segments, so you may wish to run >> + <function>pg_switch_wal</function> on the primary to perform a manual >> + switch. The reason for the switch is to arrange for >> [...] >> + WAL segments have been archived. If write activity on the primary >> is low, it >> + may be useful to run <function>pg_switch_wal</> on the primary in order to >> + trigger an immediate segment switch of the last required WAL >> It seems to me that both portions are wrong. There is no archiving >> wait on standbys for 9.6, and > I think its clearly stated here that pg_stop_backup() does not wait for > WAL to archive on a standby. Even, it is very important for the backup > routine to make sure that all the WAL *is* archived. Yes, it seems that I somewhat missed the "on the primary portion" during the first read of the patch. >> pg_stop_backup triggers by itself the >> segment switch, so saying that enforcing pg_switch_wal on the primary >> is moot. > > pg_stop_backup() does not perform a WAL switch on the standby which is > what this sentence is referring to. I have separated this section out > into a new paragraph to (hopefully) make it clearer. > >> pg_switch_xlog has been renamed to pg_switch_wal in PG10, so >> the former name applies. > > Whoops! > > New patch is attached. Thanks for the new version. This looks fine to me. -- Michael
On Wed, Aug 30, 2017 at 8:37 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > Thanks for the new version. This looks fine to me. Committed to REL9_6_STABLE with minor wordsmithing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 8/31/17 4:04 PM, Robert Haas wrote: > On Wed, Aug 30, 2017 at 8:37 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> Thanks for the new version. This looks fine to me. > > Committed to REL9_6_STABLE with minor wordsmithing. The edits look good to me. Thanks, Robert! -- -David david@pgmasters.net
Re: [HACKERS] Update low-level backup documentation to match actual behavior
От
Michael Paquier
Дата:
On Fri, Sep 1, 2017 at 5:11 AM, David Steele <david@pgmasters.net> wrote: > On 8/31/17 4:04 PM, Robert Haas wrote: >> On Wed, Aug 30, 2017 at 8:37 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> Thanks for the new version. This looks fine to me. >> >> Committed to REL9_6_STABLE with minor wordsmithing. > > The edits look good to me. Thanks, Robert! Thanks David for the patch, and Robert for the commit! The final result is nicely shaped. -- Michael