Stefan's bug (was: max_standby_delay considered harmful)
in [PgSql]
|
From: Simon Riggs on 24 May 2010 09:28 On Mon, 2010-05-24 at 09:26 -0400, Robert Haas wrote: > On Mon, May 24, 2010 at 1:27 AM, Fujii Masao <masao.fujii(a)gmail.com> wrote: > > On Wed, May 19, 2010 at 2:47 PM, Fujii Masao <masao.fujii(a)gmail.com> wrote: > >>>> Oh, right. How about allowing the postmaster only in PM_STARTUP, > >>>> PM_RECOVERY, PM_HOT_STANDBY or PM_WAIT_READONLY state to invoke > >>>> walreceiver? We can keep walreceiver alive until all read only > >>>> backends have gone, and prevent unexpected startup of walreceiver. > >>> > >>> Yes, that seems like something we should be checking, if we aren't already. > >> > >> I'll do that. > > > > Here is the updated version. I added the above-mentioned check > > into the patch. > > This looks pretty reasonable to me, but I guess I feel like it would > be better to drive the CancelBackup() decision off of whether we've > ever reached PM_RUN rather than consulting XLogCtl. That is exactly what XLogCtl tells us and why it is suggested for use. > It just feels > cleaner to me to drive all of the postmaster decisions off of the same > signalling mechanism rather than having a separate one (that only > works because it's used very late in shutdown when we theoretically > don't need a lock) just for this one case. > > I could be all wet, though. > -- 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 24 May 2010 09:26 On Mon, May 24, 2010 at 1:27 AM, Fujii Masao <masao.fujii(a)gmail.com> wrote: > On Wed, May 19, 2010 at 2:47 PM, Fujii Masao <masao.fujii(a)gmail.com> wrote: >>>> Oh, right. How about allowing the postmaster only in PM_STARTUP, >>>> PM_RECOVERY, PM_HOT_STANDBY or PM_WAIT_READONLY state to invoke >>>> walreceiver? We can keep walreceiver alive until all read only >>>> backends have gone, and prevent unexpected startup of walreceiver. >>> >>> Yes, that seems like something we should be checking, if we aren't already. >> >> I'll do that. > > Here is the updated version. I added the above-mentioned check > into the patch. This looks pretty reasonable to me, but I guess I feel like it would be better to drive the CancelBackup() decision off of whether we've ever reached PM_RUN rather than consulting XLogCtl. It just feels cleaner to me to drive all of the postmaster decisions off of the same signalling mechanism rather than having a separate one (that only works because it's used very late in shutdown when we theoretically don't need a lock) just for this one case. I could be all wet, though. -- 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: Robert Haas on 24 May 2010 09:40 On Mon, May 24, 2010 at 9:28 AM, Simon Riggs <simon(a)2ndquadrant.com> wrote: > On Mon, 2010-05-24 at 09:26 -0400, Robert Haas wrote: >> On Mon, May 24, 2010 at 1:27 AM, Fujii Masao <masao.fujii(a)gmail.com> wrote: >> > On Wed, May 19, 2010 at 2:47 PM, Fujii Masao <masao.fujii(a)gmail.com> wrote: >> >>>> Oh, right. How about allowing the postmaster only in PM_STARTUP, >> >>>> PM_RECOVERY, PM_HOT_STANDBY or PM_WAIT_READONLY state to invoke >> >>>> walreceiver? We can keep walreceiver alive until all read only >> >>>> backends have gone, and prevent unexpected startup of walreceiver. >> >>> >> >>> Yes, that seems like something we should be checking, if we aren't already. >> >> >> >> I'll do that. >> > >> > Here is the updated version. I added the above-mentioned check >> > into the patch. >> >> This looks pretty reasonable to me, but I guess I feel like it would >> be better to drive the CancelBackup() decision off of whether we've >> ever reached PM_RUN rather than consulting XLogCtl. > > That is exactly what XLogCtl tells us and why it is suggested for use. Sure. My only point is that the postmaster doesn't (and can't) use that method of getting the information at any other time when it is needed, so I don't know why we'd want to use it in just this one case. Maybe there's a reason, but it's not obvious to me. >> It just feels >> cleaner to me to drive all of the postmaster decisions off of the same >> signalling mechanism rather than having a separate one (that only >> works because it's used very late in shutdown when we theoretically >> don't need a lock) just for this one case. >> >> I could be all wet, though. -- 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: Tom Lane on 24 May 2010 10:19 Robert Haas <robertmhaas(a)gmail.com> writes: > On Mon, May 24, 2010 at 9:28 AM, Simon Riggs <simon(a)2ndquadrant.com> wrote: >> On Mon, 2010-05-24 at 09:26 -0400, Robert Haas wrote: >>> This looks pretty reasonable to me, but I guess I feel like it would >>> be better to drive the CancelBackup() decision off of whether we've >>> ever reached PM_RUN rather than consulting XLogCtl. >> >> That is exactly what XLogCtl tells us and why it is suggested for use. > Sure. My only point is that the postmaster doesn't (and can't) use > that method of getting the information at any other time when it is > needed, so I don't know why we'd want to use it in just this one case. > Maybe there's a reason, but it's not obvious to me. I'm with Robert on this. The postmaster is designed to be driven by an internal state machine. Making it rely on the contents of shared memory is a fundamentally dangerous idea. It might coincidentally be safe in this one case, but I can easily imagine that property failing as a result of subsequent changes. The postmaster should not look at shared memory if there is any reasonable alternative, and we clearly have a reasonable alternative. regards, tom lane -- 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 24 May 2010 10:35
On Mon, May 24, 2010 at 10:26 PM, Robert Haas <robertmhaas(a)gmail.com> wrote: > This looks pretty reasonable to me, but I guess I feel like it would > be better to drive the CancelBackup() decision off of whether we've > ever reached PM_RUN rather than consulting XLogCtl. It just feels > cleaner to me to drive all of the postmaster decisions off of the same > signalling mechanism rather than having a separate one (that only > works because it's used very late in shutdown when we theoretically > don't need a lock) just for this one case. Okay, how about the attached patch? It uses the postmaster-local flag "ReachedEndOfRecovery" (better name?) instead of XLogCtl one. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center |