Re: An improvement of ProcessTwoPhaseBuffer logic

On Thu, Jan 16, 2025 at 04:50:09PM +0900, Michael Paquier wrote:
> On Wed, Jan 15, 2025 at 05:00:51PM -0800, Noah Misch wrote:
> > I think "using the current epoch" is wrong for half of the nextFullXid values
> > having epoch > 0. For example, nextFullId==2^32 is in epoch 1, but all the
> > allowable XIDs are in epoch 0. (I mean "allowable" in the sense of
> > AssertTransactionIdInAllowableRange().) From then until we assign another
> > 2^31 XIDs, epochs 0 and 1 are both expected in XID values. After 2^31 XID
> > assignments, every allowable XID be in epoch 1. Hence, twophase.c would need
> > two-epoch logic like we have in widen_snapshot_xid() and XLogRecGetFullXid().
> > Is that right? (I wrote this in a hurry, so this email may have more than the
> > standard level of errors.) Before commit 7e125b2, twophase also had that
> > logic. I didn't work out the user-visible consequences of that logic's new
> > absence here, but I bet on twophase recovery breakage. Similar problem here
> > (up to two epochs are acceptable, not just one):
> >
> > + /* Discard files from past epochs */
> > + if (EpochFromFullTransactionId(fxid) < EpochFromFullTransactionId(nextXid))
>
> Oops, you're right. Your suggestion to unify all that in a single
> routine is an excellent idea. Missed the bits in xid8funcs.c.

Added.

> > I wrote the attached half-baked patch to fix those, but I tend to think it's
> > better to use FullTransactionId as many places as possible in twophase.c.
> > (We'd still need to convert XIDs that we read from xl_xact_prepare records,
> > along the lines of XLogRecGetFullXid().) How do you see it?
>
> I'm all for integrating more FullTransactionIds now that these reflect
> in the file names, and do a deeper cut.
>
> 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?

> With the additions attached, FullTransactionIdFromAllowableAt() gets
> down from 8 to 6 calls in twophase.c. The change related to
> MarkAsPreparingGuts() seems optional, though.

Thanks. It's probably not worth doing at that level of reduction. I tried
spreading fxid further and got it down to three conversions, corresponding to
redo of each of XLOG_XACT_PREPARE, XLOG_XACT_COMMIT_PREPARED, and
XLOG_XACT_ABORT_PREPARED. I'm attaching the WIP of that. It's not as
satisfying as I expected, so FullTransactionIdFromCurrentEpoch-v0.patch may
yet be the better direction.

The ProcessTwoPhaseBuffer() code to remove "past two-phase state" seems
best-effort, independent of this change. Just because an XID is in a
potentially-acceptable epoch doesn't mean TransactionIdDidCommit() will find
clog for it. I've not studied whether that matters.

Incidentally, this comment about when a function is called:

* PrescanPreparedTransactions
*
* Scan the shared memory entries of TwoPhaseState and determine the range
* of valid XIDs present. This is run during database startup, after we
* have completed reading WAL. TransamVariables->nextXid has been set to
* one more than the highest XID for which evidence exists in WAL.

doesn't match the timing of the actual earliest call:

/* REDO */
if (InRecovery)
{
...
if (ArchiveRecoveryRequested && EnableHotStandby)
{
...
if (wasShutdown)
oldestActiveXID = PrescanPreparedTransactions(&xids, &nxids);
else
oldestActiveXID = checkPoint.oldestActiveXid;
...
PerformWalRecovery();
performedWalRecovery = true;

> I am trying to figure
> out how to write a regression test to trigger this error, lacking a
> bit of time today. That's going to require more trickery with
> pg_resetwal to make that cheap, or something like that.. Attached are
> some suggestions, as of a 0002 that applies on top of your 0001.

Thanks. I'd value having your regression test, but manual-ish testing could
suffice if it's too hard.

> XLogRecGetFullXid() is used nowhere. This could be removed, perhaps,
> or not?

Maybe. Looks like it was born unused in 67b9b3c (2019-07), so removing may as
well be a separate discussion.