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

From: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
To: Nikhil Sontakke <nikhil(at)planetscale(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Hot standby accepts connections (giving wrong results) before consistency after crash reset
Date: 2026-08-07 15:13:09
Message-ID: CAHGQGwGH4qM6M_j4bOyuhH1hPAFnmictBf4Eii5=vOOdWKSZXg@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bertrand Drouvot 2026-08-07 15:13:43 Re: pgstat: allow a stats kind to use its own dedicated dsa/dshash
Previous Message Jan Nidzwetzki 2026-08-07 14:43:48 Re: [PATCH] Fix PITR pause bypass when initial XLOG_RUNNING_XACTS has subxid overflow