Fix hot standby accepting connections too early after a crash reset

Commit b53b88109f9 made the postmaster maintain reachedConsistency in
addition to the startup process.  Since the startup process is forked
from the postmaster, it begins life holding whatever value the
postmaster last set.

On a crash reset the postmaster re-forks the startup process while its
own copy still says true: it clears that copy only on receipt of
PMSIGNAL_RECOVERY_STARTED, which the replacement process cannot send
before it exists.  The replacement therefore starts out believing the
database is already consistent.

CheckRecoveryConsistency() then skips the minRecoveryPoint comparison
altogether, so hot standby is announced at redo start while replay may
be arbitrarily far behind minRecoveryPoint.  Read-only connections are
accepted and answer from heap pages that were flushed ahead of the
replay position, returning wrong results with no error raised.  The
same branch also runs XLogCheckInvalidPages() and
CheckTablespaceDirectory(), which are skipped as well, and
log_invalid_page() treats page references that are normal before
consistency as a PANIC.

Fix by clearing reachedConsistency in InitWalRecovery(), so that a
startup process never depends on the value it inherited.  The
postmaster's own copy is deliberately left alone: forked backends read
it to choose the "not yet accepting connections" errdetail, and it
converges once the new startup process sends PMSIGNAL_RECOVERY_STARTED
and, on reaching minRecoveryPoint, PMSIGNAL_RECOVERY_CONSISTENT.

Successive crash resets alternate.  A startup process that skips the
branch never sends PMSIGNAL_RECOVERY_CONSISTENT, so the postmaster's
copy stays false and the next reset forks a process holding the correct
value; that pass reaches consistency properly, which sets the
postmaster's copy back to true and re-arms the problem for the reset
after it.  Roughly every other crash reset is therefore affected, not
just the first one.  EXEC_BACKEND builds are unaffected, as
reachedConsistency is not carried in BackendParameters.

Backpatch to v18, where commit b53b88109f9 introduced this issue.

Reported-by: Eric Ridge <eebbrr@planetscale.com>
Author: Nikhil Sontakke <nikhil@planetscale.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CA+UBoq2n2Zg9rKgMfUtUohzGssisF9cDeyjKqPrnRNFprEyX1Q@mail.gmail.com
Backpatch-through: 18

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 5f3b065b8942e20ed53a86b0489bb1153168cc87..6de13b91748c5c309260c300dd639c6f5827cc77 100644 (file)
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -471,6 +471,15 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
 
        dbstate_at_startup = ControlFile->state;
 
+       /*
+        * A startup process always starts with an inconsistent database. Set the
+        * flag accordingly, even if it was inherited from a postmaster that had
+        * already marked the database as consistent. This keeps the invariant
+        * local to the startup process without requiring every fork path to clear
+        * the flag.
+        */
+       reachedConsistency = false;
+
        /*
         * Initialize on the assumption we want to recover to the latest timeline
         * that's active according to pg_control.