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

Tom Lane wrote:
> Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
>> - 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.
>
> The ordering is necessary because we have to do that before we start
> flushing buffers, and XLogFlush has to see WriteAllowed as FALSE or it
> will do the wrong thing. If we ever put something into
> AdvanceXLInsertBuffer that would depend on this, we could flip the flag
> to TRUE just for the duration of calling AdvanceXLInsertBuffer, though
> I agree that's a bit ugly. Perhaps calling the flag/routine
> XLogInsertAllowed() would alleviate this issue somewhat?

Yeah, it would look much less dirty if called XLogInsertAllowed().

>> - 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)
>
> It would be possible to make LocalWALWriteAllowed a three-state flag:
> * allowed, don't check RecoveryInProgress,
> * disallowed, don't check RecoveryInProgress
> * check RecoveryInProgress, transition to first state if clear
> Not sure if worth the trouble.

One idea is to merge LocalWALWriteAllowed and LocalRecoveryInProgress
(and the corresponding shared variables too) into one XLogState variable
with three states
* in-recovery
* normal operation
* shutdown.

The variable would always move from a lower state to higher, similar to
PMState in postmaster.c, so it could be cached like RecoveryInProgress
works now. WAL inserts would only be allowed in normal operation. An
end-of-recovery checkpoint would set LocalXLogState to "normal
operation" ahead of SharedXLogState, to allow writing the checkpoint
record, and a shutdown checkpoint would set Shared- and LocalXLogState
to "shutdown" right after writing the checkpoint record.

>> 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.
>
> I think we should do it. There is a lot of stuff I'm still not happy
> with in this area, but without a clean and understandable set of state
> definitions we aren't going to be able to improve it.

I agree, we need clear states and invariants that can be checked and
relied on. This gets even more complicated when the hot standby stuff
gets involved.

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