Re: An improvement of ProcessTwoPhaseBuffer logic

On Mon, Dec 30, 2024 at 10:08:31AM +0900, Michael Paquier wrote:
> On Fri, Dec 27, 2024 at 06:16:24PM +0300, Vitaly Davydov wrote:
> > As an idea, I would like to propose to store FullTransactionId in
> > global transaction state instead of TransactionId. I'm not sure, it
> > will consume significant additional memory, but it make the code
> > more clear and probably result into less number of locks.
> 
> FWIW, I was wondering about doing the same thing.  However, I have
> concluded that this some refactoring work out of the scope of fixing
> the primary issue we have here, as we are hit by the way the file
> names are built when we attempt to remove them.
> 
> Note that the memory footprint of storing a FullTransactionId in
> twophase.c's GlobalTransactionData does not worry me much.  It is more  
> important to not increase the size of the two-phase state data,
> impacting files on disk and their WAL records.  This size argument
> has been mentioned on the thread that has added epochs to the 2PC file
> names, as far as I recall.

I agree with accepting +4 bytes in GlobalTransactionData.

> At the end, I have applied two patches, the first one down to 13 that
> took care of the "future" issue, with tests added in the v14~v16
> range.  v13 was lacking a perl routine, and it's not a big deal to not
> have coverage for this code path anyway.  The second patch has been
> applied down to v17, to fix the epoch issue, with the more advanced
> tests.
> 
> If you have a suggestion of patch to plug in a FullTransactionId into
> GlobalTransactionData rather than a TransactionId, feel free to
> propose something!  Help is always welcome, and this would be
> HEAD-only work, making it less urgent to deal with.

I suspect we should do that and back-patch to v17 before the next releases,
because commit 7e125b2 wrote this function:

	/*
	 * Compute FullTransactionId for the given TransactionId, using the current
	 * epoch.
	 */
	static inline FullTransactionId
	FullTransactionIdFromCurrentEpoch(TransactionId xid)
	{
		FullTransactionId fxid;
		FullTransactionId nextFullXid;
		uint32		epoch;
	
		nextFullXid = ReadNextFullTransactionId();
		epoch = EpochFromFullTransactionId(nextFullXid);
	
		fxid = FullTransactionIdFromEpochAndXid(epoch, xid);
		return fxid;
	}

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

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?