Re: FullTransactionIdAdvance question

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