Assertion with aborted UPDATE in subtransaction

Hi,

Is this assertion something that is worthwhile to fix?

Thanks,
Jasper Smit

On Wed, Mar 26, 2025 at 4:26 PM Jasper Smit <jbsmit@gmail.com> wrote:

The considerations we ran in to are is that now you will create more calls to TransactionIdIsInProgress.
However, we think it would then also create a correct multixact with the right members only, and thereby avoid the assertion.

The one thing we have not been able to figure out yet, is what other fallout there would be if we don’t fix this. Apart from the assert triggering we have not really observed any problem.

Is there someone that can shed some more light on the tradeoffs here, and whether or not this is the right fix or there should be something else fixed instead?

Thanks,
Luc

From: Jasper Smit <jbsmit@gmail.com>
Date: Monday, 31. March 2025 at 12:02
To: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: Assertion with aborted UPDATE in subtransaction

[External Email]

________________________________
Hi,

Is this assertion something that is worthwhile to fix?

Thanks,
Jasper Smit

On Wed, Mar 26, 2025 at 4:26 PM Jasper Smit <jbsmit@gmail.com<mailto:jbsmit@gmail.com>> wrote:
Hi,

My colleague Oleksii Kozlov ran into an assertion while testing aborted UPDATE-commands in sub transactions.
To reproduce this assertion run the SQl in the attached script. I tested this on 15.10 and 17.4

Running the script will lead to the the assertion:
TRAP: failed Assert("HEAP_XMAX_IS_LOCKED_ONLY(infomask_lock_old_tuple)"), File: "/usr/local/postgresql-17.4/debug-build/../src/backend/access/heap/heapam.c", Line: 3766, PID: 15604

After analysis with Luc Vlaming, we believe that the problem is caused by a stale multixact member of an aborted subtransaction.

At the time of the assertion, we established that the new tuple does not fit on the same page as the old tuple. The
tuple lock needs to be updated while the page lock is temporarily released.

One line above the assertion, compute_new_xmax_infomask() is called, which will in turn call MultiXactIdExpand().
In MultiXactIdExpand() we determine that the requested txid/status is already a member of the current multixact, therefore skipping
the removal of dead members further below in that function. The multixact has in fact an aborted transaction included in it.
Because the aborted transaction was not removed, later in GetMultiXactIdHintBits(), HEAP_XMAX_LOCK_ONLY is not added to the infomask.
The absence of this bit in the infomask, will eventually lead to the assertion.

A possible fix is to change MultiXactIdExpand() to not skip the removal of dead members. See the proposed patch attached to this email.
Another alternative is to remove the assertion, as I think that at relevant places the transaction statuses of multixact members get checked.

Regards,
Jasper Smit

Hi,

I'd like to bring this thread back up. I ran into the same assertion
independently, and it still fails on master (4848fd6752a).

Why it matters to me: I develop an extension (pg_tviews) that keeps
summary tables up to date from statement-level AFTER triggers with
transition tables. The triggers upsert with INSERT ... ON CONFLICT DO
UPDATE, and some run inside PL/pgSQL exception blocks that roll back an
earlier update of the same row. I run my randomized correctness tests
on assert-enabled builds (16, 18 and 19), and this assertion crashes
the backend there. In practice I can't run that suite on cassert
builds until it's fixed. As Luc found, I saw no wrong results on
non-assert builds.

Here is a smaller repro than the original script (assert-enabled build,
fresh database):

CREATE TABLE t (a int, b text);
INSERT INTO t VALUES (1, '');
BEGIN;
SELECT * FROM t FOR NO KEY UPDATE;
SAVEPOINT s;
UPDATE t SET a = 2;
ROLLBACK TO SAVEPOINT s;
RELEASE SAVEPOINT s;
UPDATE t SET b = repeat('x', 3000); -- TRAP
COMMIT;

TRAP: failed Assert("HEAP_XMAX_IS_LOCKED_ONLY(infomask_lock_old_tuple)"),
File: "heapam.c", Line: 3893

It fails on master, REL_19_STABLE, 18.6 and REL_14_STABLE. The original
assert.sql no longer triggers it on REL_19 for me, because the page
never fills up, so the repro above uses a value over the TOAST threshold
instead.

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.

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.

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.

Regards,
Lionel Hamayon