Re: Fix for consume_xids advancing XIDs incorrectly

On 2024/10/18 14:57, Yushi Ogiwara wrote:
> In conclusion, I agree that:
>
> * Update lastxid with GetTopTransactionId().
> * consume_xids with 0 should raise an error.
>
> I have attached a new patch that incorporates your suggestions.

One concern in this discussion is that the return value of this function isn't
entirely clear. To address this, how about adding a comment at the beginning of
consume_xids() like: "Returns the last XID assigned by consume_xids()"?

* the cache overflows, but beyond that, we don't keep track of the
* consumed XIDs.
*/
- (void) GetTopTransactionId();
+ if(!FullTransactionIdIsValid(GetTopFullTransactionIdIfAny()))
+ {
+ lastxid = GetTopTransactionId();
+ consumed++;
+ }

How about appending the following to the end of the first paragraph in
the above source comments?

If no top-level XID is assigned, a new one is obtained,
and the consumed XID counter is incremented.

if (xids_left > 2000 &&
consumed - last_reported_at < REPORT_INTERVAL &&
MyProc->subxidStatus.overflowed)
{
int64 consumed_by_shortcut = consume_xids_shortcut();

if (consumed_by_shortcut > 0)
{
consumed += consumed_by_shortcut;
continue;
}
}

By the way, this isn't directly related to the proposed patch, but while reading
the xid_wraparound code, I noticed that consume_xids_common() could potentially
return an unexpected XID if consume_xids_shortcut() returns a value greater
than 2000. Based on the current logic, consume_xids_common() should always return
a value less than 2000, so the issue I'm concerned about shouldn't occur. Still,
would it be worth adding an assertion to ensure that consume_xids_common() never
returns a value greater than 2000?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION