Re: Assertion with aborted UPDATE in subtransaction

On 28/10/2026 17:59, Lionel Hamayon wrote:
> Jasper's multixact.patch still applies cleanly to master, and with it:
>
> - the repro above and the variants I tried pass (FOR UPDATE with a
> key-changing update, ON CONFLICT DO UPDATE, MERGE, PL/pgSQL exception
> blocks, a full page);
> - make check and the isolation tests pass;
> - on Luc's cost question: rebuilding a multixact with the same members
> returns the existing one from the backend-local cache, so no extra
> multixact IDs are used. I ran 500 TOAST updates while another
> transaction held FOR KEY SHARE on the row, and both the patched and
> unpatched builds used 2 multixacts. What remains is one
> TransactionIdIsInProgress() call per member, and only on this path,
> which as far as I can tell is reachable only from heap_update()'s
> TOAST/new-page branch.
>
> The patch still needs its comments updated: the function header and the
> comment above the loop both still say the existing multixact is
> returned as-is.

I looked at this patch and also the patch proposed at [1], and came up
with the attached version. The main difference is that it doesn't rely
on the cache to detect that the members haven't changed. It's probably
not significant performance-wise, but it's also very easy to track that
in MultiXactIdExpand() itself.

The extra TransactionIdIsInProgress() calls might matter if the "xid is
already a member" case was common. But I don't think it is. Notably,
heap_lock_tuple() doesn't rely on that but has different checks for
whether you're already holding the suitable lock. So if you do e.g.
"SELECT FOR KEY SHARE" twice on the same set of tuples, there's no
performance difference.

There's a missed opportunity in heap_update(): if the tuple is already
locked, there's no need to lock it again. If heap_update() checked for
that like heap_lock_tuple() does, then it it would never reach
MultiXactIdExpand() when the XID is already a member. We should perhaps
implement that in 'master'.

Anyway, thoughts on the attached patch?

> Attached is a regression test for combocid.sql that goes on top of
> Jasper's patch. It has three cases, and each one crashes an
> assert-enabled build without the fix.

I kept only one of the three cases in the attached patch, since they all
reach the same codepath and assertion in the end. But thanks for
including them, it's always good to have more coverage during
development, even if not all of the test cases are added to the
permanent regression suite.

> In full disclosure: I'm not a PostgreSQL internals developer. The
> analysis and the test were done with an AI assistant (Claude). I checked
> the results by running them, but I can't vouch for the reasoning in
> depth, so please treat it accordingly. I hope that, together with
> Jasper's patch, this is enough to get the fix in.

Thanks!

PS. While working on this, I notice another bug in the heap_update()
locking code, see
https://www.postgresql.org/message-id/ce8b11dd-5908-45ac-bb3a-4ccfd5f88cf5@iki.fi.
It's unrelated to this one though, except that it's in the same piece of
code as the assertion.

[1]
https://www.postgresql.org/message-id/CAPgh2mJhTo2yq_vLrT6OZPn54hDxJpDVu%3D9L7Ay_M5qogNFuTw%40mail.gmail.com

- Heikki