Thread: FullTransactionIdAdvance question

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