FullTransactionIdAdvance question
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t50295psql -h localhost -U postgresBuilt from patchset v3 (message #3), September 20, 2026 at 04:15 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t50295_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t50295_3 && git checkout t50295_3Patchset v3 (message #3) is on t50295_3
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
Hi,
On 2024-09-20 17:38:40 +0800, Andy Fan wrote:
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).
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
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