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.