Re: Incorrect logic in XLogNeedsFlush()

Thanks for taking time to reply!

On Wed, Sep 3, 2025 at 11:38 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Sat, Aug 30, 2025 at 4:16 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>
> > Why is it okay for other processes than the startup process to
> > initialize LocalMinRecoveryPoint from ControlFile->minRecoveryPoint
> > during crash recovery?
>
> During crash recovery we don't yet know the minRecoveryPoint until we
> replay all the WALs and once it is done it will be updated in the
> ControlFile at PerformRecoveryXLogAction()->CreateEndOfRecoveryRecord().
> After this point it will be valid to use by any process including the
> StartupProcess.  So I don't think until ControlFile->minRecoveryPoint
> is initialized it is safe to use for any process.  In fact, before any
> connections are allowed this has to be set either by
> CreateEndOfRecoveryRecord() if recovering a primary from crash or by
> ReachedEndOfBackup() if you are starting a standby from backup.

So, looking at the code, it seems like most places we examine
ControlFile->minRecoveryPoint, we condition it on "InArchiveRecovery".
Is this something we want to do in XLogNeedsFlush() to avoid reading
from ControlFile->minRecoveryPoint()?

Though, it seems like LocalMinRecoveryPoint must be getting
incorrectly set elsewhere, otherwise this would have guarded us from
examining the control file:

        if (XLogRecPtrIsInvalid(LocalMinRecoveryPoint) && InRecovery)
            updateMinRecoveryPoint = false;

        /* Quick exit if already known to be updated or cannot be updated */
        if (record <= LocalMinRecoveryPoint || !updateMinRecoveryPoint)
            return false;

> > Besides this question, I find the use of the global variable
> > InRecovery as a proxy for RecoveryInProgress() + "I'm the startup
> > process" to be confusing. It seems like UpdateMinRecoveryPoint() and
> > XLogNeedsFlush() could just test MyBackendType == B_STARTUP and make
> > it explicit.
>
> IIUC InRecovery is slightly different than "RecoveryInProgress() +
> "I'm the startup", in StartupXlog, we first set InRecovery to false
> and then update the minRecoveryPoint in CreateEndOfRecoveryRecord()
> and after that we clear RecoveryInProgress (i.e. set
> XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE;).

Hmm. So, RecoveryInProgress() can be true in the startup process and
InRecovery is false. I see.

- Melanie