Stefan's bug (was: max_standby_delay considered harmful)
in [PgSql]
|
From: Simon Riggs on 17 May 2010 07:38 On Mon, 2010-05-17 at 07:33 -0400, Robert Haas wrote: > I would > prefer that we focus on the technical issues here rather than who > wrote the patch. There are three patches now from 2 authors. I agree we should focus on the technical issues, but which issues, in which patch? If Masao had accepted your version he would not have written another, would he? -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Fujii Masao on 17 May 2010 07:44 On Mon, May 17, 2010 at 8:02 PM, Robert Haas <robertmhaas(a)gmail.com> wrote: >> (1) >> Smart or fast shutdown requested in PM_STARTUP state always removes >> the backup_label file if it exists. But it might be still required >> for subsequent recovery. I changed your patch so that additionally >> the postmaster skips deleting the backup_label in that case. > > Can you explain in a little more detail how this can cause a problem? > I'm not very familiar with how the backup label is used. > > Also, why is this different in PM_STARTUP than in PM_RECOVERY? > PM_RECOVERY doesn't guarantee that we've reached consistency. Before the startup process sends the PMSIGNAL_RECOVERY_STARTED signal (i.e., when the postmaster is in PM_STARTUP state), it reads the backup_label file to know the recovery starting WAL location, saves that information in pg_control file, and rename the file "backup_label" to "backup_label.old". If the backup_label file is removed before pg_control is updated, subsequent recovery cannot get the right recovery starting location. This is the problem that I'm concerned. The smart shutdown during recovery and the fast shutdown might call CancelBackup() and remove the backup_label file. So if shutdown is requested in PM_STARTUP state, the problem would happen. In the patch, if shutdown is requested in PM_STARTUP, the postmaster skips calling CancelBackup() since the backup_label file might be required. >> (2) >> pg_ctl -ms stop emits the following warning whenever there is the >> backup_label file in $PGDATA. >> >> WARNING: online backup mode is active >> Shutdown will not complete until pg_stop_backup() is called. >> >> This warning doesn't fit in with the shutdown during recovery case. >> Since smart shutdown might be requested by other than pg_ctl, the >> warning should be emitted in server side rather than client, I think. >> How about moving the warning to the server side? > > Hmm, I'm not sure whether that's a good idea or not. Perhaps we > should discuss for 9.1? Okay, this is not a critical problem. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 17 May 2010 07:57 On Mon, May 17, 2010 at 7:38 AM, Simon Riggs <simon(a)2ndquadrant.com> wrote: > On Mon, 2010-05-17 at 07:33 -0400, Robert Haas wrote: >> I would >> prefer that we focus on the technical issues here rather than who >> wrote the patch. > > There are three patches now from 2 authors. I agree we should focus on > the technical issues, but which issues, in which patch? If Masao had > accepted your version he would not have written another, would he? He wrote his version first. I reviewed it and submitted a simplified version. He reviewed mine and submitted an updated version that addresses an additional possible issue, which is currently under discussion. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Simon Riggs on 17 May 2010 08:01 On Mon, 2010-05-17 at 16:38 +0900, Fujii Masao wrote: > On Mon, May 17, 2010 at 10:25 AM, Robert Haas <robertmhaas(a)gmail.com> wrote: > > Therefore I think > > Fujii Masao's original idea was the best, but I have what I believe is > > an equivalent but simpler implementation, which is attached. > > Seems good. > > I found another two problems related to shutdown in PM_STARTUP state: > > (1) > Smart or fast shutdown requested in PM_STARTUP state always removes > the backup_label file if it exists. But it might be still required > for subsequent recovery. I changed your patch so that additionally > the postmaster skips deleting the backup_label in that case. Don't like the name NeedBackupLabel seems too specific. That really corresponds to "we were in recovery". We should have a couple of super-states that correspond to am in recovery/am not in recovery so we can drive it from that. > (2) > pg_ctl -ms stop emits the following warning whenever there is the > backup_label file in $PGDATA. > > WARNING: online backup mode is active > Shutdown will not complete until pg_stop_backup() is called. > > This warning doesn't fit in with the shutdown during recovery case. > Since smart shutdown might be requested by other than pg_ctl, the > warning should be emitted in server side rather than client, I think. > How about moving the warning to the server side? +1 > > Thoughts? Should we try to fix this in 8.4 also, or just in HEAD? > > 8.3 and 8.2 never handle a smart shutdown prior to entering normal > > running, and while that seems pretty useless, doing something > > different would be a behavior change, so that seems like a > > non-starter. 8.4 has the same behavior as HEAD, though it's not > > documented in the release notes, so it's not clear how intentional the > > change was. > > In 8.4, smart shutdown during recovery waits until the startup process > has exited. So the backporting to 8.4 doesn't improve any situation, > I think. We shouldn't be discussing backporting a behaviour change. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 17 May 2010 09:20
On Mon, May 17, 2010 at 7:44 AM, Fujii Masao <masao.fujii(a)gmail.com> wrote: > On Mon, May 17, 2010 at 8:02 PM, Robert Haas <robertmhaas(a)gmail.com> wrote: >>> (1) >>> Smart or fast shutdown requested in PM_STARTUP state always removes >>> the backup_label file if it exists. But it might be still required >>> for subsequent recovery. I changed your patch so that additionally >>> the postmaster skips deleting the backup_label in that case. >> >> Can you explain in a little more detail how this can cause a problem? >> I'm not very familiar with how the backup label is used. >> >> Also, why is this different in PM_STARTUP than in PM_RECOVERY? >> PM_RECOVERY doesn't guarantee that we've reached consistency. > > Before the startup process sends the PMSIGNAL_RECOVERY_STARTED signal > (i.e., when the postmaster is in PM_STARTUP state), it reads the > backup_label file to know the recovery starting WAL location, saves > that information in pg_control file, and rename the file "backup_label" > to "backup_label.old". > > If the backup_label file is removed before pg_control is updated, > subsequent recovery cannot get the right recovery starting location. > This is the problem that I'm concerned. > > The smart shutdown during recovery and the fast shutdown might call > CancelBackup() and remove the backup_label file. So if shutdown is > requested in PM_STARTUP state, the problem would happen. OK, I think I understand now. But, the SIGTERM sent by the postmaster doesn't kill the recovery process unconditionally. It will invoke StartupProcShutdownHandler(), which will set set shutdown_requested = true. That gets checked by RestoreArchivedFile() and HandleStartupProcInterrupts(), and I think that neither of those can get invoked until after the control file has been updated. Do you see a way it can happen? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |