> On Oct 8, 2026, at 23:54, Zhao Song <songzhao.asm@icloud.com> wrote:
>
> Hi,
>
> I noticed a redundant MultiXactIdIsRunning() check in HeapTupleSatisfiesUpdate(), when xmax is a multixact containing
anupdate (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
MultiXactIdnever 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
alwaysreturn 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
theupdater.
> * 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
anybehavior, but avoids an unnecessary multixact member lookup and proc array scan when the updater has aborted, and
makesthe 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
whymarking xmax invalid is safe.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
