Re: Remove redundant MultiXactIdIsRunning() check in HeapTupleSatisfiesUpdate()

> On Oct 8, 2026, at 23:54, Zhao Song <songzhao(dot)asm(at)icloud(dot)com> 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/