Re: Hot standby accepts connections (giving wrong results) before consistency after crash reset

Hi Fujii-san,

Thanks for pushing the fix. I agree that keeping the source comment concise
is
better, and I'm happy to look into refining the TAP test to be more
lightweight
while still providing the necessary coverage. Let me know if that's what we
want
to do.

Regards,
Nikhil

On Fri, Aug 7, 2026 at 8:43 PM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> On Fri, Aug 7, 2026 at 6:52 PM Nikhil Sontakke <nikhil(at)planetscale(dot)com>
> wrote:
> > 0002 clears reachedConsistency in InitWalRecovery(), so that a startup
> > process never depends on the value it inherited. The postmaster's
> > own copy is left alone, since forked backends still read it for
> > the errdetail, and it converges once the new startup process sends
> > PMSIGNAL_RECOVERY_STARTED and later
> > PMSIGNAL_RECOVERY_CONSISTENT.
>
> Thanks for the report, analysis, and patches!
>
> Your analysis looks correct to me, and 0002 seems like the right minimal
> fix.
>
> I've pushed 0002, with a slightly shorter source comment. The original
> comment
> was a bit too detailed for an in-code comment; I think those details are
> better
> covered by the commit message.
>
>
> > I put the reset in the startup process rather than having the postmaster
> > clear its copy before forking, so that the invariant lives with the
> > process that owns it and no future fork path has to remember to clear it
> > first. A tidier alternative would be to give the postmaster its own
> > flag and return reachedConsistency to being startup-process-private, as
> > it effectively was through 17.x, but that seemed too invasive for
> > something that wants back-patching to 18. I am happy to write it that
> > way for master if people prefer.
>
> Agreed. For the backpatchable fix, resetting reachedConsistency in the
> startup process seems simple and sufficient.
>
> In the longer term, it may be worth separating the postmaster's state
> from the startup process's state on master, since they use this
> information for slightly different purposes. But I don't think that is
> necessary for this bug fix.
>
>
> > 0001 adds a TAP test. It fails on master and on back branches down to
> > 18, so a reviewer can confirm the problem before applying the fix.
> > The test uses recovery_min_apply_delay to keep the check from
> > depending on timing: recoveryApplyDelay() ignores the delay until
> > consistency is reached, so a correct standby replays to
> > minRecoveryPoint at full speed while an affected one stops well
> > short of it.
>
> I have not committed 0001 for now.
>
> The test does cover the problem, but it seems somewhat expensive for
> what it checks, since it creates a 20,000-row table and updates the
> whole table several times to create the required gap. I'd like to
> discuss whether we really want the test in this form, or whether the
> same coverage can be achieved more cheaply or simply.
>
> Regards,
>
> --
> Fujii Masao
>