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

Hi Fujii-san,

You are right that the test was expensive for what it checks. I have
reworked it: rather than adding a new file, v2 extends the existing
016_min_consistency.pl tap test.

The new test apparatus turns out to be unnecessary, because
016_min_consistency.pl already builds it. It runs with the same
shared_buffers = 128kB, its table is large enough at fillfactor 10 to
keep the startup process evicting, and at the point where it takes its
restart point no restartpoint has run yet, so the redo pointer in the
control file is still the one taken by the base backup.

So v2 adds a phase to that test, between the last wait_for_catchup and
the CHECKPOINT that creates the restart point. It kills a backend to
force the crash reset, then checks that replay gets back up to
minRecoveryPoint before a connection is accepted, and that the first
connection admitted afterwards sees the table as it should be.

Numbers on my machine: 10 runs with the fix all pass, 10 runs with it
reverted all fail, and the recovery positions are byte-identical across
every run, so it is not timing-sensitive. The added runtime is two to
three seconds. 016's own assertion passes in both directions, and
make check in src/test/recovery is green.

Please let me know if this works for you.

Thanks,
Nikhil
---
Nikhil Sontakke
PlanetScale

On Fri, Aug 7, 2026 at 9:21 PM Nikhil Sontakke <nikhil(at)planetscale(dot)com>
wrote:

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