Re: Stefan's bug (was: max_standby_delay considered harmful)

On Tue, May 18, 2010 at 10:40 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, May 18, 2010 at 8:35 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, May 17, 2010 at 10:40 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Mon, May 17, 2010 at 10:20 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> 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?
>>>
>>> Yeah, the way is:
>>> StartupXLOG() --> ReadCheckpointRecord() --> ReadRecord() -->
>>> XLogPageRead() --> XLogFileReadAnyTLI() --> XLogFileRead() -->
>>> RestoreArchivedFile()
>>>
>>> ReadCheckpointRecord() is called before pg_control is updated.
>>
>> OK.  In that case, I'm wondering if we should reverse course and
>> rejigger the logic so that the shutdown gets processed when we
>> transition to PM_RECOVERY.  Seems like that might be simpler.
>
> You mean keeping shutdown waiting until the postmaster has reached
> PM_RECOVERY, i.e., the startup process has sent PMSIGNAL_RECOVERY_STARTED?
>
> The startup process must call ReadCheckpointRecord() before sending
> that signal. ReadCheckpointRecord() might get stuck for some reasons,
> e.g., neither master nor standby might have the recovery starting
> checkpoint WAL record. So that signal might not be sent forever,
> in this case, shutdown would get stuck.

Ah, OK.

In terms of removing the backup label file, can we simply have an
additional boolean in the postmaster that indicates whether we've ever
reached PM_RUN, and only consider removing the backup file if so?

>>> ISTM that walreceiver might be invoked even after shutdown is requested.
>>> We should prevent the postmaster from starting up walreceiver if
>>> Shutdown > NoShutdown?
>>
>> Well, when we did the previous shutdown patch, we decided it was not
>> right to kill walreceiver until all backends had exited, so it seems
>> inconsistent to make the opposite decision here.
>
> 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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company