> On Oct 8, 2026, at 23:54, Zhao Song <[email protected]> wrote:
>
> Hi,
>
> I noticed a redundant MultiXactIdIsRunning() check in
> HeapTupleSatisfiesUpdate(), when xmax is a multixact containing an update
> (the HEAP_XMAX_IS_MULTI branch in heapam_visibility.c).
>
> The relevant code is:
>
> if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false))
> return TM_BeingModified;
>
> if (TransactionIdDidCommit(xmax))
> { ... return TM_Updated / TM_Deleted; }
>
> /*
> * By here, the update in the Xmax is either aborted or crashed, but
> * what about the other members?
> */
>
> if (!MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false))
> {
> SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
> return TM_Ok;
> }
> else
> {
> /* There are lockers running */
> return TM_BeingModified;
> }
>
> The else branch can't be reached. The first MultiXactIdIsRunning() call
> already returns if any member, updater or locker, is still running. Between
> the two calls, we only check TransactionIdDidCommit(xmax). Since the members
> of a MultiXactId never change (as the comment in MultiXactIdIsRunning() says,
> "it is not legal to add members to an existing MultiXactId"), and a member
> that wasn't running at the first check can't become running again, the second
> call must always return false.
>
> I went through the commit history to understand why this check exists:
>
> * 1ce150b7bb1 added the second check and changed the first one to
> TransactionIdIsInProgress(xmax), which only checked the updater.
> * 07aeb1fec57 added the else branch to handle the case where lockers were
> still running.
> * 05315498012 changed the first check back to MultiXactIdIsRunning(...,
> false), covering all members again.
>
> So since 05315498012, the second check is redundant and its else branch
> unreachable.
>
> The attached patch removes the second MultiXactIdIsRunning() call and the
> unreachable branch. This should not change any behavior, but avoids an
> unnecessary multixact member lookup and proc array scan when the updater has
> aborted, and makes the code easier to understand.
>
> Regards,
> Zhao Song
> <v1-0001-Remove-redundant-MultiXactIdIsRunning-check-in-He.patch>
I think your analysis is correct. The "if (TransactionIdDidCommit(xmax))"
branch cannot change the result of MultiXactIdIsRunning(), so the later check
is redundant.
But I think we should delete the “By here …” comment and retain the “There's no
member …” comment because it explains why marking xmax invalid is safe.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
