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

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> Here's a patch implementing the WALWriteAllowed() idea (I'm not wedded
> to the name).

I'm not either ... this morning it seems to me that XLogWriteAllowed
(or maybe XLogInsertAllowed?) would be more in keeping with the existing
code names in this area.

> 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.

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?

> - 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.

> 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.

regards, tom lane