On Jul8, 2011, at 17:56 , Peter Geoghegan wrote:
> On 8 July 2011 15:58, Florian Pflug <fgp@phlo.org> wrote:
>> SyncRepWaitForLSN() says
>> /*
>> * Wait on latch for up to 60 seconds. This allows us to check for
>> * postmaster death regularly while waiting. Note that timeout here
>> * does not necessarily release from loop.
>> */
>> WaitLatch(&MyProc->waitLatch, 60000000L);
>>
>> I guess that 60-second timeout is unnecessary now that we'll wake up
>> on postmaster death anyway.
>
> We won't wake up on Postmaster death here, because we haven't asked to
> - not yet, anyway. We're just using the new interface here for that
> one function call in v8.
Oh, Right. I still think it'd might be worthwhile to convert it, but
but not in this patch.
>> Also, none of the callers of WaitLatch() seems to actually inspect
>> the WL_POSTMASTER_DEATH bit of the result. We might want to make
>> their !PostmasterIsAlive() check conditional on the WL_POSTMASTER_DEATH
>> bit being set. At least in the case of SyncRepWaitForLSN(), it seems
>> that avoiding the extra read() syscall might be beneficial.
>
> I don't think so. Postmaster death is an anomaly, so why bother with
> any sort of optimisation for that case? Also, that's exactly the sort
> of thing that we're trying to caution callers against doing with this
> comment:
>
> "That should be rare in practice, but the caller should not use the
> return value for anything critical, re-checking the situation with
> PostmasterIsAlive() or read() on a socket if necessary."
Uh, I phrased that badly. What I meant was doing
if ((result & WL_POSTMASTER_DEATH) && (!PostmasterIsAlive())
instead of
if (!PostmasterIsAlive)
It seems that currently SyncRepWaitForLSN() will execute
PostmasterIsAlive() after every wake up. But actually it only needs
to do that if WaitLatch() sets WL_POSTMASTER_DEATH. Usually we wouldn't
care, but in the case of SyncRepWaitForLSN() I figures that we might.
It's in the code path of COMMIT (in the case of synchronous replication)
after all...
We'd not optimize the case of a dead postmaster, but the case of
an live one. Which I do hope is the common case ;-)
> You might say that the only reason we even bother reporting postmaster
> death in the returned bitfield is because there is an expectation that
> it will report it, given that we use the same masks on wakeEvents to
> inform the function what events we'll actually be waiting on for the
> call.
I kinda guessed that to be the reason after reading the latest patch ;-)
best regards,
Florian Pflug