Re: RecoveryInProgress() has critical side effects

On Mon, Nov 15, 2021 at 9:34 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> Patch 0001 means that the startup process would set up the structures
> to be able to build WAL records before running any kind of recovery
> action rather than doing it when it really needs it. That's fine by
> me.

Great, thanks. I think I'll go ahead and commit this one, then.

> Is patch 0002 actually right regarding the handling of doPageWrites?
> Once applied, we finish by setting it when the startup process starts
> and not anymore at the end of recovery based on its the state of
> Insert, but this could have changed while in recovery when replaying
> one or more XLOG_FPW_CHANGE records.

Maybe I'm not understanding you properly here, but it seems like you
might be forgetting that this is a local variable and thus every
backend is going to have something different. In the startup process,
it will be initialized by StartupXLOG(); in other processes, it's
currently initialized by RecoveryInProgress(), but with this patch it
wouldn't be. Either way, it's then updated by future calls to
XLogInsertRecord() as required. XLOG_FPW_CHANGE records might affect
the new value that gets set the next time XLogInsertRecord(), but they
don't directly affect doPageWrites.

> > I'm personally not too worried about a ~4% regression in this
> > particular benchmark...
>
> This is not a hot code path, that should be fine.

OK. I'll wait a while and see if anyone else wants to weigh in.

--
Robert Haas
EDB: http://www.enterprisedb.com