Re: [PATCH] Fix PITR pause bypass when initial XLOG_RUNNING_XACTS has subxid overflow - Mailing list pgsql-hackers

Hi Hackers,

This is a follow-up to Matt Blewitt's report and patch from February [1], which identified the following bug: when the
firstXLOG_RUNNING_XACTS record a standby replays has subxid_overflow set, standbyState gets stuck at
STANDBY_SNAPSHOT_PENDING,and hot standby is never activated. As a consequence, recovery_target_action = 'pause' is
silentlyignored: recoveryPausesHere() returns immediately because !LocalHotStandbyActive, the PAUSE case falls through,
andthe server promotes instead of pausing. 

We'd like to propose an alternative fix for the same problem and describe why we believe serving read-only queries in
thisstate is safe, and why we deliberately do not advance standbyState to STANDBY_SNAPSHOT_READY as the earlier patch
did.


Patches
=======
 0001 - Behavior-preserving refactor: pull the connection-enabling block
        out of CheckRecoveryConsistency() into a small helper.

 0002 - The fix: call EnableHotStandbyConnections() from the
        RECOVERY_TARGET_ACTION_PAUSE path, just before recoveryPausesHere(), and add a TAP test.


Why we believe enabling reads is correct
=======

The reason a standby normally refuses queries from an overflowed snapshot is the risk of an incorrect visibility
decisionfor a subtransaction whose top-level transaction is still running on the primary. 

When the initial RUNNING_XACTS is overflowed, KnownAssignedXids may be missing some subxids. For such a recovery
snapshot,XidInMVCCSnapshot() first maps the xid to its topmost parent via  SubTransGetTopmostTransaction() and then
looksit up in the in-progress set. If that mapping is not available in pg_subtrans and the xid is not present in
KnownAssignedXids,the xid is treated as "not in the snapshot", and the final committed/aborted decision is delegated to
TransactionIdDidCommit()in HeapTupleSatisfiesMVCC(). 

During active WAL replay, this is the dangerous case: a subxid S of a still-running top transaction T may have its row
presenton disk; if S cannot be resolved as in-progress and T's commit record is later replayed, CLOG flips T to
committed,and a query could suddenly see a row that was not committed as of its snapshot's xmax. That is exactly why
connectionsare withheld until a non-overflowed snapshot (STANDBY_SNAPSHOT_READY) gives complete knowledge. 

At an end-of-recovery pause, this hazard disappears because replay is frozen. The only ways out of
recoveryPausesHere(true)are promotion and shutdown. A pg_wal_replay_resume() at the end of recovery falls through to
promotionrather than resuming replay. 

Therefore, no commit record for any in-progress transaction will ever be replayed, so CLOG cannot transition T (or its
subxids)to committed after this point. TransactionIdDidCommit() for such an xid stays false forever. So, the MVCC
visibilityfallback keeps the row invisible. 

In short, the set of transactions a query can observe as committed is now stable, and is exactly the set that committed
beforereplay stopped. The single condition that makes overflowed-snapshot reads unsafe during live replay (a
transactionthat was in progress as of a snapshot later being observed as committed) cannot arise once replay halts. So
thepending snapshot, while still overflowed, yields correct and stable visibility. 


Why we keep standbyState at STANDBY_SNAPSHOT_PENDING
=======

The earlier patch forced standbyState to STANDBY_SNAPSHOT_READY at the pause point and re-ran
CheckRecoveryConsistency().We chose not to do that. 

STANDBY_SNAPSHOT_READY means we have full knowledge of the transactions that were running on the primary and that
snapshotsare complete and need not be treated as overflowed. That is not true here since the snapshot is still
overflowed(visibility stays correct for the frozen-replay reason above, not because the snapshot has become complete).
Forcingthe state to READY would assert something false about the recovery state. 


Original report and first patch by Matt Blewitt. Thanks also for the analysis in that thread.

Thoughts welcome.

[1] https://www.postgresql.org/message-id/CACy-Nv24ZORVN9_S_yHF5Nsip45HKCBtKVNC3XdKgz%2B1wvGvEQ%40mail.gmail.com

Best Regards
Jan Nidzwetzki
On behalf of PlanetScale