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

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> 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:

I've committed this patch plus the mentioned revisions, along with a
bunch of code review of my own (mostly doctoring comments that needed
it).  There are a couple of loose ends that should get looked at later:

* AFAICS there is no mechanism preventing the bgwriter from trying to
issue a restartpoint after the end-of-recovery checkpoint and before the
startup process manages to clear SharedRecoveryInProgress.  It's
unlikely, because the EOR checkpoint would advance the local idea of
when the next checkpoint should happen, but it doesn't appear to be
impossible.  If it did happen it would be really bad because
pg_control's notion of the latest checkpoint could go backward, perhaps
into a part of XLOG that isn't there anymore.  You'd never notice unless
you were unfortunate enough to crash before the next regular checkpoint.
I put a hack defense into CreateRestartPoint so that it won't overwrite
pg_control if the state doesn't look right once it's got
ControlFileLock, but I wonder if there's a better answer.

* CreateRestartPoint tries to report recoveryLastXTime, but that is dead
code because that variable isn't maintained in the bgwriter process.
This is not critical functionality so I don't mind releasing 8.4 with it
broken.  Perhaps we should use the time from the restartpoint's
checkpoint?

* I'm unconvinced by the code you added to "Update min recovery point
one last time".  What is that supposed to be good for?  It won't do
anything in a crash-recovery case (when minRecoveryPoint is 0/0);
does that matter?

* I find the RecoveryInProgress test in XLogNeedsFlush rather dubious.
Why is it okay to disable that?  For at least one of the two callers
(SetHintBits) it seems like the safe answer is "true" not "false".
This doesn't matter too much yet but it will for hot standby.

* I find the RecoveryInProgress test in ShutdownXLOG rather dubious too.
If that code path can actually be taken, isn't it trying to shut down
modules that haven't been initialized?

            regards, tom lane