Re: Fix for consume_xids advancing XIDs incorrectly

On 2024/10/24 5:23, Masahiko Sawada wrote:
>>          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.
> 
> Good point. Yes, the function doesn't return a value greater than 2000
> as long as we do "distance = Min(distance, COMMIT_TS_XACTS_PER_PAGE -
> rem);". But it's true with <= 16KB block sizes.
> 
>> Still,
>> would it be worth adding an assertion to ensure that consume_xids_common() never
>> returns a value greater than 2000?
> 
> While adding an assertion makes sense to me, another idea is to set
> last_xid even in the shortcut path. That way, it works even with 32KB
> block size.

Agreed on making xid_wraparound compatible with a 32k block size. As you pointed out,
with 32k blocks, XidSkip() can return values greater than 2000. So, the first step is
to adjust the following if-condition by increasing "2000" to a higher value.
Since XidSkip() can return up to 3276 (based on COMMIT_TS_XACTS_PER_PAGE (BLCKSZ / 10) with 32k blocks),
we could, for instance, update "2000" to "4000."

        if (xids_left > 2000 &&
            consumed - last_reported_at < REPORT_INTERVAL &&
            MyProc->subxidStatus.overflowed)


To ensure lastxid is set even in the shortcut case, what about modifying XidSkip()
so it returns "distance - 1" instead of "distance"? This change would make
consume_xids_common() run at least one more loop cycle,
triggering GetNewTransactionId() and setting lastxid correctly.


    consumed = XidSkip(nextXid);
    if (consumed > 0)
        TransamVariables->nextXid.value += (uint64) consumed;

BTW, the code snippet above in consume_xids_shortcut() could potentially set
the next XID to a value below FirstNormalTransactionId? If yes, we should account for
FirstNormalFullTransactionId when increasing the next XID, similar to
how FullTransactionIdAdvance() handles it.

Regards,

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