From: Andy Fan <zhihuifan1213@163.com>
To: Andres Freund <andres@anarazel.de>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: FullTransactionIdAdvance question
In-Reply-To: <lstjexkttz7vkywlumhzqzf6altzydzfv64sbmuegg3nkkgeb5@nnkkta645abz>
	(Andres Freund's message of "Fri, 20 Sep 2024 14:17:05 -0400")
References: <87frpu4qcf.fsf@163.com>
	<lstjexkttz7vkywlumhzqzf6altzydzfv64sbmuegg3nkkgeb5@nnkkta645abz>
Date: Mon, 23 Sep 2024 08:53:24 +0800
Message-ID: <87v7yn8a2j.fsf@163.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: <https://www.postgresql.org/message-id/87v7yn8a2j.fsf%40163.com>
Precedence: bulk

--=-=-=
Content-Type: text/plain


Hi Andres:

> On 2024-09-20 17:38:40 +0800, Andy Fan wrote:
>> static inline void
>> FullTransactionIdAdvance(FullTransactionId *dest)
>> {
..
>> }
>> 
>> 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).
>
> 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.

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.

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

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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=v20240923-0001-Add-unlikely-to-FullTransactionIdAdvanc-ch.patch

From 92e0d65605b185588b57415a640187b4ccc2ae74 Mon Sep 17 00:00:00 2001
From: Andy Fan <zhihuifan1213@163.com>
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;
 
 	while (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
-- 
2.45.1


--=-=-=--