Re: An improvement of ProcessTwoPhaseBuffer logic

On Thu, Jan 16, 2025 at 12:52:54PM -0800, Noah Misch wrote:
> On Thu, Jan 16, 2025 at 04:50:09PM +0900, Michael Paquier wrote:
> > As far as I understand, the most important point of the logic is to
> > detect and discard the future files first in restoreTwoPhaseData() ->
> > ProcessTwoPhaseBuffer() when scanning the contents of pg_twophase at
> > the beginning of recovery. Once this filtering is done, it should be
> > safe to use your FullTransactionIdFromAllowableAt() when doing
> > the fxid <-> xid transitions between the records and the files on disk
> > flushed by a restartpoint which store an XID, and the shmem state of
> > GlobalTransactionData with a fxid.
>
> (I did not expect that a function called restoreTwoPhaseData() would run before
> a function called PrescanPreparedTransactions(), but so it is.)
>
> How is it that restoreTwoPhaseData() -> ProcessTwoPhaseBuffer() can safely
> call TransactionIdDidAbort() when we've not replayed WAL to make CLOG
> consistent? What can it assume about the value of TransamVariables->nextXid
> at that early time?

I think it can't use CLOG safely. I regret answering my own question now as
opposed to refraining from asking it, but hopefully this will save you time.

Since EndPrepare() flushes the XLOG_XACT_PREPARE record before anything writes
to pg_twophase, presence of a pg_twophase file tells us its record once
existed in the flushed WAL stream. Even before we've started the server on a
base backup, each pg_twophase file pertains to an XLOG_XACT_PREPARE LSN no
later than the end-of-backup checkpoint. If a later file exists, the backup
copied a file after the end-of-backup checkpoint started, which is a violation
of the base backup spec.

Elsewhere in recovery, we have the principle that data directory content means
little until we reach a consistent state by replaying the end-of-backup
checkpoint or by crash recovery reaching the end of WAL. For twophase, that
calls for ignoring pg_twophase content. If a restartpoint has a pg_twophase
file to write, we should allow that to overwrite an old file iff we've not
reached consistency. (We must not read the old pg_twophase file, which may
contain an unfinished write.)

By the time we reach consistency, every file in pg_twophase will be applicable
(not committed or aborted). Each file either predates the start-of-backup
checkpoint (or crash recovery equivalent), or recovery wrote that file. We
need readdir(pg_twophase) only at end of recovery, when we need
_twophase_recover callbacks to restore enough state to accept writes. (During
hot standby, state from XLOG_RUNNING_XACTS suffices[1] for the actions
possible in that mode.)

In other words, the root problem that led to commits e358425 and 7e125b2 was
recovery interpreting pg_twophase file content before reaching consistency.
We can't make the ProcessTwoPhaseBuffer() checks safe before reaching
consistency. Is that right? Once ProcessTwoPhaseBuffer() stops running
before consistency, it won't strictly need the range checks. We may as well
have something like those range checks as a best-effort way to detect base
backup spec violations.

Thanks,
nm

[1] Incidentally, the comment lock_twophase_standby_recover incorrectly claims
it runs "when starting up into hot standby mode."