Re: An improvement of ProcessTwoPhaseBuffer logic

On Wed, Jan 15, 2025 at 05:00:51PM -0800, Noah Misch wrote:
> I agree with accepting +4 bytes in GlobalTransactionData.

Let's just bite the bullet and do that on HEAD and v17, then,
integrating deeper FullTransactionIds into the internals of
twophase.c.

> 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.

> 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.

With the additions attached, FullTransactionIdFromAllowableAt() gets
down from 8 to 6 calls in twophase.c.  The change related to
MarkAsPreparingGuts() seems optional, though.  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.

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