Thread: Minor changes to Recovery related code

Minor changes to Recovery related code

From
"Simon Riggs"
Date:
I'd like to make the following changes to recovery related code over the
next few days/weeks. If anybody insists I do this by freeze or not at
all, then I'll submit patches for 1,3,4,5,10 before Saturday night. I'd
rather take a bit more time and do this in one drop and there are some
code dependencies between these changes and other patches from
Koichi-san and myself.

1. Current xlog should be archived at shutdown for smart shutdown
- check that archiver is active prior to shutdown checkpoint request
- if (shutdown checkpoint && XLogArchivingActive()) RequestXLogSwitch()
- for smart shutdown, have archiver complete its work before exiting

2. pg_stop_backup() should wait until all archive files are safely
archived before returning

3. Need a %r parameter for restore_command, to allow the restore command
be passed the name of the file containing the last restartpoint. This
will allow the restore command to clean down old archive files more
safely/cleanly in Warm Standby operation.
- change to pg_standby to accept the parameter and use %r rather than -k
parameter

4. Add an option to pg_standby to have it complete all outstanding
archive files after it has been triggered, minimising data loss at the
slight expense of cut-over time

5. log_restartpoint option in recovery.conf
LOG each restorepoint, so can understand whether restartable recovery
will be effective or not

6. refactor recovery.conf so that it uses a GUC-like parser

7. refactor all xlog _desc routines into one module, so these can be
more easily used by xlogviewer utility

8. get xlogviewer utility a freshen-up so it can be part of main
release, possibly including further refactoring of xlog.c

9. Another round of doc updates to highlight the use of pg_standby and
Koichi-san's work. I think Doug Knight might have some additional code
examples to include as well, from previous discussions.

10. Changes to ensure WAL-avoiding operations and hot backups cannot be
executed simultaneously. One of these two options, ISTM:

a) Following a change to archive_command while server is running. Log
the xid of the WAL-avoiding operations when they start and have
pg_start_backup() wait for those xids to complete before continuing.

b) Introduce a new parameter, archive_mode = on | off that can only be
set at server start. If archive_mode = on then XLogArchivingActive();
archiving only takes place when archive_command is not ''. This allows
archive_command to be changed while server running, yet without any
danger from WAL-avoiding operations.

[7 & 8 would be complete by about 5-6 weeks from now. Others much
earlier]

Comments?

--  Simon Riggs              EnterpriseDB   http://www.enterprisedb.com




Re: Minor changes to Recovery related code

From
Tom Lane
Date:
"Simon Riggs" <simon@2ndquadrant.com> writes:
> I'd like to make the following changes to recovery related code over the
> next few days/weeks. If anybody insists I do this by freeze or not at
> all, then I'll submit patches for 1,3,4,5,10 before Saturday night. I'd
> rather take a bit more time and do this in one drop and there are some
> code dependencies between these changes and other patches from
> Koichi-san and myself.

Well, I've got a proposal for a pg_proc change that I haven't even started
coding yet, so personally I won't hold you to having the patch submitted
as long as the design is agreed to before feature freeze.  However:

> 2. pg_stop_backup() should wait until all archive files are safely
> archived before returning

Not sure I agree with that one.  If it fails, you can't tell whether the
action is done and it failed while waiting for the archiver, or if you
need to redo it.

> 6. refactor recovery.conf so that it uses a GUC-like parser

I would suggest that after feature freeze is not the time for code
beautification efforts like this.  (I rather doubt that it's worth doing
at all actually, but definitely not now.)

> 7. refactor all xlog _desc routines into one module, so these can be
> more easily used by xlogviewer utility

Even more so.

> 8. get xlogviewer utility a freshen-up so it can be part of main
> release, possibly including further refactoring of xlog.c

This is not happening for 8.3, either.

> 10. Changes to ensure WAL-avoiding operations and hot backups cannot be
> executed simultaneously. One of these two options, ISTM:
> b) Introduce a new parameter, archive_mode = on | off that can only be
> set at server start. If archive_mode = on then XLogArchivingActive();
> archiving only takes place when archive_command is not ''. This allows
> archive_command to be changed while server running, yet without any
> danger from WAL-avoiding operations.

I think I'd go with (b) since a lot of people felt it should've been
like that from the beginning, and found the magic "empty string"
behavior confusing.

> [7 & 8 would be complete by about 5-6 weeks from now. Others much
> earlier]

We are hoping to go beta in less time than that.  While I'm willing to
cut a little slack, anything you can't submit before about mid-April
is not going to make it into 8.3.
        regards, tom lane


Re: Minor changes to Recovery related code

From
"Simon Riggs"
Date:
On Fri, 2007-03-30 at 16:34 -0400, Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.com> writes:
> > I'd like to make the following changes to recovery related code over the
> > next few days/weeks. If anybody insists I do this by freeze or not at
> > all, then I'll submit patches for 1,3,4,5,10 before Saturday night. I'd
> > rather take a bit more time and do this in one drop and there are some
> > code dependencies between these changes and other patches from
> > Koichi-san and myself.
> 
> Well, I've got a proposal for a pg_proc change that I haven't even started
> coding yet, so personally I won't hold you to having the patch submitted
> as long as the design is agreed to before feature freeze.  However:

Cool

> > 2. pg_stop_backup() should wait until all archive files are safely
> > archived before returning
> 
> Not sure I agree with that one.  If it fails, you can't tell whether the
> action is done and it failed while waiting for the archiver, or if you
> need to redo it.

There's a slight delay between pg_stop_backup() completing and the
archiver doing its stuff. Currently if somebody does a -m fast straight
after the pg_stop_backup() the backup may be unusable.

We need a way to plug that small hole.

I suggest that pg_stop_backup() polls once per second until
pg_xlog/archive_status/LOG.ready disappears, in which case it ends
successfully. If it does this for more than 60 seconds it ends
successfully but produces a WARNING.

> > 6. refactor recovery.conf so that it uses a GUC-like parser
> 
> I would suggest that after feature freeze is not the time for code
> beautification efforts like this.  (I rather doubt that it's worth doing
> at all actually, but definitely not now.)

OK

> > 7. refactor all xlog _desc routines into one module, so these can be
> > more easily used by xlogviewer utility
> 
> Even more so.

OK

> > 8. get xlogviewer utility a freshen-up so it can be part of main
> > release, possibly including further refactoring of xlog.c
> 
> This is not happening for 8.3, either.

OK

> > 10. Changes to ensure WAL-avoiding operations and hot backups cannot be
> > executed simultaneously. One of these two options, ISTM:
> > b) Introduce a new parameter, archive_mode = on | off that can only be
> > set at server start. If archive_mode = on then XLogArchivingActive();
> > archiving only takes place when archive_command is not ''. This allows
> > archive_command to be changed while server running, yet without any
> > danger from WAL-avoiding operations.
> 
> I think I'd go with (b) since a lot of people felt it should've been
> like that from the beginning, and found the magic "empty string"
> behavior confusing.

OK

> We are hoping to go beta in less time than that.  While I'm willing to
> cut a little slack, anything you can't submit before about mid-April
> is not going to make it into 8.3.

No probs, you struck a line through the more time consuming items. :-)

--  Simon Riggs              EnterpriseDB   http://www.enterprisedb.com




Re: Minor changes to Recovery related code

From
"Florian G. Pflug"
Date:
Simon Riggs wrote:
> On Fri, 2007-03-30 at 16:34 -0400, Tom Lane wrote:
>> "Simon Riggs" <simon@2ndquadrant.com> writes:
>>> 2. pg_stop_backup() should wait until all archive files are safely
>>> archived before returning
>> Not sure I agree with that one.  If it fails, you can't tell whether the
>> action is done and it failed while waiting for the archiver, or if you
>> need to redo it.
> 
> There's a slight delay between pg_stop_backup() completing and the
> archiver doing its stuff. Currently if somebody does a -m fast straight
> after the pg_stop_backup() the backup may be unusable.
> 
> We need a way to plug that small hole.
> 
> I suggest that pg_stop_backup() polls once per second until
> pg_xlog/archive_status/LOG.ready disappears, in which case it ends
> successfully. If it does this for more than 60 seconds it ends
> successfully but produces a WARNING.

I fear that ending sucessfully despite having not archived all wals
will make this feature less worthwile. If a dba knows what he is
doing, he can code a perfectly safe backup script using 8.2 too.
He'll just have to check the current wal position after pg_stop_backup(),
(There is a function for that, right?), and wait until the corresponding
wal was archived.

In realitly, however, I feare that most people will just create a script
that does 'echo "select pg_stop_backup | psql"' or something similar.
If they're a bit more carefull, they will enable ON_ERROR_STOP, and check
the return value of pgsql. I believe that those are the people who would
really benefit from a pg_stop_backup() that waits for archiving to complete.
But they probably won't check for WARNINGs.

Maybe doing it the other way round would be an option?
pg_stop_backup() could wait for the archiver to complete forever, but
spit out a warning every 60 seconds or so "WARNING: Still waiting
for wal archiving of wal ??? to complete". If someone really wants
a 60-second timeout, he can just use statement_timeout.

Anyway, just my 0.02 eurocents, maybe I'm totally mistaken about the
postgresql dba's out there...

greetings, Florian Pflug


Re: Minor changes to Recovery related code

From
Jim Nasby
Date:
On Mar 30, 2007, at 5:51 PM, Florian G. Pflug wrote:
> In realitly, however, I feare that most people will just create a  
> script
> that does 'echo "select pg_stop_backup | psql"' or something similar.
> If they're a bit more carefull, they will enable ON_ERROR_STOP, and  
> check
> the return value of pgsql. I believe that those are the people who  
> would
> really benefit from a pg_stop_backup() that waits for archiving to  
> complete.
> But they probably won't check for WARNINGs.
>
> Maybe doing it the other way round would be an option?
> pg_stop_backup() could wait for the archiver to complete forever, but
> spit out a warning every 60 seconds or so "WARNING: Still waiting
> for wal archiving of wal ??? to complete". If someone really wants
> a 60-second timeout, he can just use statement_timeout.

I agree; people are far more likely to take the lazy way out, so it'd  
be good to protect against that. Esp. since statement_timeout  
provides a means to get the other behavior.
--
Jim Nasby                                            jim@nasby.net
EnterpriseDB      http://enterprisedb.com      512.569.9461 (cell)




Re: Minor changes to Recovery related code

From
"Simon Riggs"
Date:
On Sat, 2007-03-31 at 00:51 +0200, Florian G. Pflug wrote:
> Simon Riggs wrote:
> > On Fri, 2007-03-30 at 16:34 -0400, Tom Lane wrote:
> >> "Simon Riggs" <simon@2ndquadrant.com> writes:
> >>> 2. pg_stop_backup() should wait until all archive files are safely
> >>> archived before returning
> >> Not sure I agree with that one.  If it fails, you can't tell whether the
> >> action is done and it failed while waiting for the archiver, or if you
> >> need to redo it.
> > 
> > There's a slight delay between pg_stop_backup() completing and the
> > archiver doing its stuff. Currently if somebody does a -m fast straight
> > after the pg_stop_backup() the backup may be unusable.
> > 
> > We need a way to plug that small hole.
> > 
> > I suggest that pg_stop_backup() polls once per second until
> > pg_xlog/archive_status/LOG.ready disappears, in which case it ends
> > successfully. If it does this for more than 60 seconds it ends
> > successfully but produces a WARNING.
> 
> I fear that ending sucessfully despite having not archived all wals
> will make this feature less worthwile. If a dba knows what he is
> doing, he can code a perfectly safe backup script using 8.2 too.
> He'll just have to check the current wal position after pg_stop_backup(),
> (There is a function for that, right?), and wait until the corresponding
> wal was archived.
> 
> In realitly, however, I feare that most people will just create a script
> that does 'echo "select pg_stop_backup | psql"' or something similar.
> If they're a bit more carefull, they will enable ON_ERROR_STOP, and check
> the return value of pgsql. I believe that those are the people who would
> really benefit from a pg_stop_backup() that waits for archiving to complete.
> But they probably won't check for WARNINGs.
> 
> Maybe doing it the other way round would be an option?
> pg_stop_backup() could wait for the archiver to complete forever, but
> spit out a warning every 60 seconds or so "WARNING: Still waiting
> for wal archiving of wal ??? to complete". If someone really wants
> a 60-second timeout, he can just use statement_timeout.

I've just come up against this problem again, so I think it is a must
fix for this release. Other problems exist also, mentioned on separate
threads.

We have a number of problems surrounding pg_stop_backup/shutdown:

1. pg_stop_backup() currently returns before the WAL file containing the
last change is correctly archived. That is a small hole, but one that is
exposed when people write test scripts that immediately shutdown the
database after issuing pg_stop_backup(). It doesn't make much sense to
shutdown immediately after a hot backup, but it should still work
sensibly.

2. We've also had problems caused by making the archiver wait until all
WAL files are archived. If there is a backlog for some reason and the
DBA issues a restart (i.e. stop and immediate restart) then making the
archiver loop while it tries (and possibly fails) to archive all files
would cause an outage. Avoiding this is why we do the current
get-out-fast approach.
There are some sub scenarios:
a) there is a backlog of WAL files, but no error has occurred on the
*last* file (we might have just fixed a problem).
b) there is a backlog of WAL files, but an error is causing a retry of
the last file.

My proposal is for us to record somewhere other than the logs that a
failure to archive has occurred and is being retried. Failure to archive
will be recorded in the archive_status directory as an additional file
called archive_error, which will be deleted in the case of archive
success and created in the case of archive error. This maintains
archiver's lack of attachment to shared memory and general simplicity of
design.

- pg_stop_backup() will wait until the WAL file that ends the backup is
safely archived, even if a failure to archive occurs. This is a change
to current behaviour, but since it implements the originally *expected*
behaviour IMHO it should be the default.

- new function: pg_stop_backup_nowait() return immediately without
waiting for archive, the same as the current pg_stop_backup()

- new function: pg_stop_backup_wait(int seconds) wait until either an
archival fails or the ending WAL file is archived, with a max wait as
specified. wait=0 means wait until archive errors are resolved.

Alternatives?

--  Simon Riggs              EnterpriseDB   http://www.enterprisedb.com




Re: Minor changes to Recovery related code

From
"Simon Riggs"
Date:
On Thu, 2007-06-07 at 21:53 +0100, Simon Riggs wrote:

> We have a number of problems surrounding pg_stop_backup/shutdown:

I'll start coding up my proposals, in the absence of alternate views.

--  Simon Riggs              EnterpriseDB   http://www.enterprisedb.com




Re: Minor changes to Recovery related code

From
Bruce Momjian
Date:
This has been saved for the 8.4 release:
http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Simon Riggs wrote:
> On Sat, 2007-03-31 at 00:51 +0200, Florian G. Pflug wrote:
> > Simon Riggs wrote:
> > > On Fri, 2007-03-30 at 16:34 -0400, Tom Lane wrote:
> > >> "Simon Riggs" <simon@2ndquadrant.com> writes:
> > >>> 2. pg_stop_backup() should wait until all archive files are safely
> > >>> archived before returning
> > >> Not sure I agree with that one.  If it fails, you can't tell whether the
> > >> action is done and it failed while waiting for the archiver, or if you
> > >> need to redo it.
> > > 
> > > There's a slight delay between pg_stop_backup() completing and the
> > > archiver doing its stuff. Currently if somebody does a -m fast straight
> > > after the pg_stop_backup() the backup may be unusable.
> > > 
> > > We need a way to plug that small hole.
> > > 
> > > I suggest that pg_stop_backup() polls once per second until
> > > pg_xlog/archive_status/LOG.ready disappears, in which case it ends
> > > successfully. If it does this for more than 60 seconds it ends
> > > successfully but produces a WARNING.
> > 
> > I fear that ending sucessfully despite having not archived all wals
> > will make this feature less worthwile. If a dba knows what he is
> > doing, he can code a perfectly safe backup script using 8.2 too.
> > He'll just have to check the current wal position after pg_stop_backup(),
> > (There is a function for that, right?), and wait until the corresponding
> > wal was archived.
> > 
> > In realitly, however, I feare that most people will just create a script
> > that does 'echo "select pg_stop_backup | psql"' or something similar.
> > If they're a bit more carefull, they will enable ON_ERROR_STOP, and check
> > the return value of pgsql. I believe that those are the people who would
> > really benefit from a pg_stop_backup() that waits for archiving to complete.
> > But they probably won't check for WARNINGs.
> > 
> > Maybe doing it the other way round would be an option?
> > pg_stop_backup() could wait for the archiver to complete forever, but
> > spit out a warning every 60 seconds or so "WARNING: Still waiting
> > for wal archiving of wal ??? to complete". If someone really wants
> > a 60-second timeout, he can just use statement_timeout.
> 
> I've just come up against this problem again, so I think it is a must
> fix for this release. Other problems exist also, mentioned on separate
> threads.
> 
> We have a number of problems surrounding pg_stop_backup/shutdown:
> 
> 1. pg_stop_backup() currently returns before the WAL file containing the
> last change is correctly archived. That is a small hole, but one that is
> exposed when people write test scripts that immediately shutdown the
> database after issuing pg_stop_backup(). It doesn't make much sense to
> shutdown immediately after a hot backup, but it should still work
> sensibly.
> 
> 2. We've also had problems caused by making the archiver wait until all
> WAL files are archived. If there is a backlog for some reason and the
> DBA issues a restart (i.e. stop and immediate restart) then making the
> archiver loop while it tries (and possibly fails) to archive all files
> would cause an outage. Avoiding this is why we do the current
> get-out-fast approach.
> There are some sub scenarios:
> a) there is a backlog of WAL files, but no error has occurred on the
> *last* file (we might have just fixed a problem).
> b) there is a backlog of WAL files, but an error is causing a retry of
> the last file.
> 
> My proposal is for us to record somewhere other than the logs that a
> failure to archive has occurred and is being retried. Failure to archive
> will be recorded in the archive_status directory as an additional file
> called archive_error, which will be deleted in the case of archive
> success and created in the case of archive error. This maintains
> archiver's lack of attachment to shared memory and general simplicity of
> design.
> 
> - pg_stop_backup() will wait until the WAL file that ends the backup is
> safely archived, even if a failure to archive occurs. This is a change
> to current behaviour, but since it implements the originally *expected*
> behaviour IMHO it should be the default.
> 
> - new function: pg_stop_backup_nowait() return immediately without
> waiting for archive, the same as the current pg_stop_backup()
> 
> - new function: pg_stop_backup_wait(int seconds) wait until either an
> archival fails or the ending WAL file is archived, with a max wait as
> specified. wait=0 means wait until archive errors are resolved.
> 
> Alternatives?
> 
> -- 
>   Simon Riggs             
>   EnterpriseDB   http://www.enterprisedb.com
> 
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
> 
>                http://www.postgresql.org/docs/faq

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Minor changes to Recovery related code

From
Simon Riggs
Date:
Follow-up during March 2008 CommitFest

On Thu, 2007-06-07 at 21:53 +0100, Simon Riggs wrote:
> On Sat, 2007-03-31 at 00:51 +0200, Florian G. Pflug wrote:
> > Simon Riggs wrote:
> > > On Fri, 2007-03-30 at 16:34 -0400, Tom Lane wrote:
> > >> "Simon Riggs" <simon@2ndquadrant.com> writes:
> > >>> 2. pg_stop_backup() should wait until all archive files are safely
> > >>> archived before returning
> > >> Not sure I agree with that one.  If it fails, you can't tell whether the
> > >> action is done and it failed while waiting for the archiver, or if you
> > >> need to redo it.
> > > 
> > > There's a slight delay between pg_stop_backup() completing and the
> > > archiver doing its stuff. Currently if somebody does a -m fast straight
> > > after the pg_stop_backup() the backup may be unusable.
> > > 
> > > We need a way to plug that small hole.
> > > 
> > > I suggest that pg_stop_backup() polls once per second until
> > > pg_xlog/archive_status/LOG.ready disappears, in which case it ends
> > > successfully. If it does this for more than 60 seconds it ends
> > > successfully but produces a WARNING.
> > 
> > I fear that ending sucessfully despite having not archived all wals
> > will make this feature less worthwile. If a dba knows what he is
> > doing, he can code a perfectly safe backup script using 8.2 too.
> > He'll just have to check the current wal position after pg_stop_backup(),
> > (There is a function for that, right?), and wait until the corresponding
> > wal was archived.
> > 
> > In realitly, however, I feare that most people will just create a script
> > that does 'echo "select pg_stop_backup | psql"' or something similar.
> > If they're a bit more carefull, they will enable ON_ERROR_STOP, and check
> > the return value of pgsql. I believe that those are the people who would
> > really benefit from a pg_stop_backup() that waits for archiving to complete.
> > But they probably won't check for WARNINGs.
> > 
> > Maybe doing it the other way round would be an option?
> > pg_stop_backup() could wait for the archiver to complete forever, but
> > spit out a warning every 60 seconds or so "WARNING: Still waiting
> > for wal archiving of wal ??? to complete". If someone really wants
> > a 60-second timeout, he can just use statement_timeout.
> 
> I've just come up against this problem again, so I think it is a must
> fix for this release. Other problems exist also, mentioned on separate
> threads.
> 
> We have a number of problems surrounding pg_stop_backup/shutdown:
> 
> 1. pg_stop_backup() currently returns before the WAL file containing the
> last change is correctly archived. That is a small hole, but one that is
> exposed when people write test scripts that immediately shutdown the
> database after issuing pg_stop_backup(). It doesn't make much sense to
> shutdown immediately after a hot backup, but it should still work
> sensibly.
> 
> 2. We've also had problems caused by making the archiver wait until all
> WAL files are archived. If there is a backlog for some reason and the
> DBA issues a restart (i.e. stop and immediate restart) then making the
> archiver loop while it tries (and possibly fails) to archive all files
> would cause an outage. Avoiding this is why we do the current
> get-out-fast approach.
> There are some sub scenarios:
> a) there is a backlog of WAL files, but no error has occurred on the
> *last* file (we might have just fixed a problem).
> b) there is a backlog of WAL files, but an error is causing a retry of
> the last file.
> 
> My proposal is for us to record somewhere other than the logs that a
> failure to archive has occurred and is being retried. Failure to archive
> will be recorded in the archive_status directory as an additional file
> called archive_error, which will be deleted in the case of archive
> success and created in the case of archive error. This maintains
> archiver's lack of attachment to shared memory and general simplicity of
> design.
> 
> - pg_stop_backup() will wait until the WAL file that ends the backup is
> safely archived, even if a failure to archive occurs. This is a change
> to current behaviour, but since it implements the originally *expected*
> behaviour IMHO it should be the default.

The most straightforward thing is to make pg_stop_backup() wait as
described above.

If people want it to timeout, they can use a statement_timeout as
suggested by Florian.

This can be implemented by having the function poll in an infinite loop
for archive_status/LOG.done. Also, as Florian suggests, we should have
it output a WARNING message every 60 seconds.

I'll write a patch now.

> - new function: pg_stop_backup_nowait() return immediately without
> waiting for archive, the same as the current pg_stop_backup()
> 
> - new function: pg_stop_backup_wait(int seconds) wait until either an
> archival fails or the ending WAL file is archived, with a max wait as
> specified. wait=0 means wait until archive errors are resolved.

So I won't do these.

--  Simon Riggs 2ndQuadrant  http://www.2ndQuadrant.com 
 PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk