Subject: FullTransactionIdAdvance question

From: Andy Fan <[email-masked]>
Date: Fri, 20 Sep 2024 17:38:40 +0800

Hi,

static inline void
FullTransactionIdAdvance(FullTransactionId *dest)
{
	dest->value++;

	/* see FullTransactionIdAdvance() */
	if (FullTransactionIdPrecedes(*dest, FirstNormalFullTransactionId))
		return;

	while (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
		dest->value++;
}

I understand this functiona as: 'dest->value++' increases the epoch when
necessary and we don't want use the TransactionId which is smaller than
FirstNormalTransactionId. But what is the point of the below code:

/* see FullTransactionIdAdvance() */
if (FullTransactionIdPrecedes(*dest, FirstNormalFullTransactionId))
	return;

It looks to me it will be never true(I added a 'Assert(false);' above
the return, make check-world pass). and if it is true somehow, retruning
a XID which is smaller than FirstNormalTransactionId looks strange as
well. IIUC, should we remove it to save a prediction on each
GetNewTransactionId call?   

-- 
Best Regards
Andy Fan

---

From: Andres Freund <[email-masked]>
Date: Fri, 20 Sep 2024 14:17:05 -0400

Hi,

On 2024-09-20 17:38:40 +0800, Andy Fan wrote:
> the return, make check-world pass).

Hm. I think in the past we did have some code that could end up calling
FullTransactionIdAdvance() on special xids for some reason, IIRC it was
related to BootstrapTransactionId. Turning those into a normal xid doesn't
seem quite right, I guess and could hide bugs.

But I'm not sure it'd not better to simply assert out in those cases.

> and if it is true somehow, retruning a XID which is smaller than
> FirstNormalTransactionId looks strange as well.

Well, it'd be true if you passed it a special xid.

> IIUC, should we remove it to save a prediction on each GetNewTransactionId
> call?

I could see adding an unlikely() to make sure the compiler orders the code to
make it statically predictable.

Greetings,

Andres Freund

---

From: Andy Fan <[email-masked]>
Date: Mon, 23 Sep 2024 08:53:24 +0800

>> FullTransactionIdAdvance(FullTransactionId *dest)
..
> But I'm not sure it'd not better to simply assert out in those cases.

Per my current understanding, special XIDs are special and better be
used explicitly (vs advance special XID-1 to special XID-2)? Currently
if the input is BootstrapTransactionId, then we would get
FrozenTransactionId, which I'm still can't understand a reason of it. I
checkout the code to the commit where FullTransactionIdAdvance was
introduced, and then add the "Assert(false)". make clean .. make
check-world still passed.

> make it statically predictable.

Thanks, Attached is the version to add the 'unlikely' while I'm still
searching the possibility to remove the code. I will defer to you after
my understanding is expressed. Actually I'm more fan on the knowledge
about the XIDs which I am not aware of. I think either the fix of
'prediction miss' or removing the code probably not make any noticeable
improvements in this case.  So thanks for checking this!

-- 
Best Regards
Andy Fan

>From 92e0d65605b185588b57415a640187b4ccc2ae74 Mon Sep 17 00:00:00 2001
From: Andy Fan <[email-masked]>
Date: Mon, 23 Sep 2024 08:42:31 +0800
Subject: [PATCH v20240923 1/1] Add unlikely to FullTransactionIdAdvanc check.

The 'FullTransactionIdPrecedes(*dest, FirstNormalFullTransactionId)' are
unlikely to be true, so add a 'unlikely' to make the code statically
predictable.
---
 src/include/access/transam.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/access/transam.h b/src/include/access/transam.h
index 28a2d287fd..8a0a67d4a4 100644
--- a/src/include/access/transam.h
+++ b/src/include/access/transam.h
@@ -130,7 +130,7 @@ FullTransactionIdAdvance(FullTransactionId *dest)
 	dest->value++;
 
 	/* see FullTransactionIdAdvance() */
-	if (FullTransactionIdPrecedes(*dest, FirstNormalFullTransactionId))
+	if (unlikely(FullTransactionIdPrecedes(*dest, FirstNormalFullTransactionId)))
 		return;
 
2.45.1