Re: BUG #4879: bgwriter fails to fsync the file in recovery mode

Simon Riggs wrote:
> On Fri, 2009-06-26 at 05:14 +0100, Simon Riggs wrote:
>> On Thu, 2009-06-25 at 20:25 -0400, Tom Lane wrote:
>
>>> What I am thinking is that instead of the hack of clearing
>>> LocalRecoveryInProgress to allow the current process to write WAL,
>>> we should have a separate test function WALWriteAllowed() with a
>>> state variable LocalWALWriteAllowed, and the hack should set that
>>> state without playing any games with LocalRecoveryInProgress. Then
>>> RecoveryInProgress() remains true during the end-of-recovery checkpoint
>>> and we can condition the TruncateMultiXact and TruncateSUBTRANS calls
>>> on that. Meanwhile the various places that check RecoveryInProgress
>>> to decide if WAL writing is allowed should call WALWriteAllowed()
>>> instead.
>> No need.
>
> Belay that. Yes, agree need for additional state, though think its more
> like EndRecoveryIsComplete().

Here's a patch implementing the WALWriteAllowed() idea (I'm not wedded
to the name). There's two things that trouble me with this patch:

- CreateCheckPoint() calls AdvanceXLInsertBuffer() before setting
LocalWALWriteAllowed. I don't see anything in AdvanceXLInsertBuffer()
that would fail, but it doesn't feel right. While strictly speaking it
doesn't insert new WAL records, it does write WAL page headers.

- As noted with an XXX comment in the patch, CreateCheckPoint() now
resets LocalWALWriteAllowed to false after a shutdown/end-of-recovery
checkpoint. But that's not enough to stop WAL inserts after a shutdown
checkpoint, because when RecoveryInProgress() is false, we
WALWriteAllowed() still returns true. We haven't had such a safeguard in
place before, so we can keep living without it, but now that we have a
WALWriteAllowed() macro it would be nice if it returned false when WAL
writes are no longer allowed after a shutdown checkpoint. (that would've
caught a bug in Guillaume Smet's original patch to rotate a WAL segment
at shutdown, where the xlog switch was done after shutdown checkpoint)

On whole, this is probably the right way going forward, but I'm not sure
if it'd make 8.4 more or less robust than what's in CVS now.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com