From 0de7613672b2968ed6e5a02c396a173e04e75246 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Fri, 9 Oct 2026 23:37:21 +0300 Subject: [PATCH v5 1/1] Fix checks for aborted updating transaction in multixids As mentioned in the comments in TransactionIdDidAbort() and in heapam_visibility.c, the usual procedure for checking whether a transaction aborted is to first call TransactionIdIsInProgress(), and then !TransactionIdDidCommit(). A couple places didn't get the memo, and used TransactionIdDidAbort() incorrectly. In DoesMultiXactIdConflict(), the consequence was that if a conflicting transaction was just aborting, and had already marked the transaction as aborted in pg_xact, but had not yet removed itself from ProcArray, the function would return false meaning that it does not conflict, even though it was still considered as running by TransactionIdIsInProgress(). That could cause trouble later. One known consequence is that if you then went ahead with updating the row again and called MultiXactIdExpand(), MultiXactIdExpand() would see both the old aborting transaction and the new updater as still running, and throw an ERROR: new multixact has more than one updating member. The multixact-aborted-updater.spec included in this commit exercises that scenario. Another place that got this wrong was in heap_update(), where we check if the updating transaction that was part of a multixid committed or aborted. Because it used TransactionIdDidAbort(), if the updating transaction crashed, it was considered as committed and heap_update() incorrectly returned TM_Updated indicating a conflict, even though the row could be updated. This would only happen if the multixid as whole was still considered running, i.e. if the tuple was still locked by other transactions while the updating transaction crashed. That can happen if the locker is a prepared transaction. The included 060_multixact_crashed_updater.pl test tests that scenario. Author: Chee Wooson Discussion: https://www.postgresql.org/message-id/5778D6297D3CCF21+202609151836512678486@vastdata.com.cn Backpatch-through: 14 --- src/backend/access/heap/heapam.c | 30 +++-- src/backend/access/transam/xact.c | 6 + src/test/modules/injection_points/Makefile | 3 +- .../expected/multixact-aborted-updater.out | 84 ++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/multixact-aborted-updater.spec | 107 ++++++++++++++++++ src/test/recovery/meson.build | 1 + .../t/060_multixact_crashed_updater.pl | 64 +++++++++++ 8 files changed, 286 insertions(+), 10 deletions(-) create mode 100644 src/test/modules/injection_points/expected/multixact-aborted-updater.out create mode 100644 src/test/modules/injection_points/specs/multixact-aborted-updater.spec create mode 100644 src/test/recovery/t/060_multixact_crashed_updater.pl diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 5c1eaadd442..6a09a1a1517 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3621,14 +3621,25 @@ l2: else update_xact = InvalidTransactionId; - /* - * There was no UPDATE in the MultiXact; or it aborted. No - * TransactionIdIsInProgress() call needed here, since we called - * MultiXactIdWait() above. - */ - if (!TransactionIdIsValid(update_xact) || - TransactionIdDidAbort(update_xact)) + if (!TransactionIdIsValid(update_xact)) + { + /* no UPDATE in the MultiXact */ can_continue = true; + } + else + { + /* + * There was another UPDATE. Did it abort? + * + * Note: We check !TransactionIdDidCommit() instead of + * TransactionIdDidAbort(), to treat crashed transactions as + * aborted. No TransactionIdIsInProgress() call is needed + * here, because the MultiXactIdWait() call above already + * waited for it to finish. + */ + if (!TransactionIdDidCommit(update_xact)) + can_continue = true; + } } else if (TransactionIdIsCurrentTransactionId(xwait)) { @@ -7864,8 +7875,9 @@ DoesMultiXactIdConflict(MultiXactId multi, uint16 infomask, if (ISUPDATE_from_mxstatus(members[i].status)) { - /* ignore aborted updaters */ - if (TransactionIdDidAbort(memxid)) + /* ignore aborted (or crashed) updaters */ + if (!TransactionIdIsInProgress(memxid) && + !TransactionIdDidCommit(memxid)) continue; } else diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index cab99f25e1e..702cbb9df10 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1925,6 +1925,12 @@ RecordTransactionAbort(bool isSubXact) if (ndroppedstats) pfree(droppedstats); + /* + * Test the window where the transaction is aborted in pg_xact but still + * present in ProcArray. + */ + INJECTION_POINT("transaction-abort-after-clog", NULL); + return latestXid; } diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 2463d6e0fb9..775ad37860d 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -35,7 +35,8 @@ ISOLATION = basic \ syscache-update-pruned \ $(WAIT_CLEANUP_TST) \ heap_lock_update \ - on_conflict_probe_window + on_conflict_probe_window \ + multixact-aborted-updater # some isolation tests require wal_level=replica ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf diff --git a/src/test/modules/injection_points/expected/multixact-aborted-updater.out b/src/test/modules/injection_points/expected/multixact-aborted-updater.out new file mode 100644 index 00000000000..40750714734 --- /dev/null +++ b/src/test/modules/injection_points/expected/multixact-aborted-updater.out @@ -0,0 +1,84 @@ +Parsed test spec with 4 sessions + +starting permutation: s1begin s1update s2lock s1abort s3nowait wake +step s1begin: BEGIN; +step s1update: UPDATE mxact_abort SET filler = 's1' WHERE id = 1; +step s2lock: SELECT * FROM mxact_abort WHERE id = 1 FOR KEY SHARE; +id|filler +--+------- + 1|initial +(1 row) + +step s1abort: ROLLBACK; +step s3nowait: SELECT * FROM mxact_abort WHERE id = 1 FOR NO KEY UPDATE NOWAIT; +ERROR: could not obtain lock on row in relation "mxact_abort" +step wake: + SELECT FROM injection_points_detach('transaction-abort-after-clog'); + SELECT FROM injection_points_wakeup('transaction-abort-after-clog'); + +step s1abort: <... completed> + +starting permutation: s1begin s1update s2lock s1abort s3update wake +step s1begin: BEGIN; +step s1update: UPDATE mxact_abort SET filler = 's1' WHERE id = 1; +step s2lock: SELECT * FROM mxact_abort WHERE id = 1 FOR KEY SHARE; +id|filler +--+------- + 1|initial +(1 row) + +step s1abort: ROLLBACK; +step s3update: UPDATE mxact_abort SET filler = 's3' WHERE id = 1; +step wake: + SELECT FROM injection_points_detach('transaction-abort-after-clog'); + SELECT FROM injection_points_wakeup('transaction-abort-after-clog'); + +step s3update: <... completed> +step s1abort: <... completed> + +starting permutation: s1begin s1update s2lock s1abort s3delete wake s3check +step s1begin: BEGIN; +step s1update: UPDATE mxact_abort SET filler = 's1' WHERE id = 1; +step s2lock: SELECT * FROM mxact_abort WHERE id = 1 FOR KEY SHARE; +id|filler +--+------- + 1|initial +(1 row) + +step s1abort: ROLLBACK; +step s3delete: DELETE FROM mxact_abort WHERE id = 1; +step wake: + SELECT FROM injection_points_detach('transaction-abort-after-clog'); + SELECT FROM injection_points_wakeup('transaction-abort-after-clog'); + +step s3delete: <... completed> +step s1abort: <... completed> +step s3check: SELECT * FROM mxact_abort ORDER BY id; +id|filler +--+------ +(0 rows) + + +starting permutation: s1begin s1update s2lock s1abort s3keyupdate wake s3check +step s1begin: BEGIN; +step s1update: UPDATE mxact_abort SET filler = 's1' WHERE id = 1; +step s2lock: SELECT * FROM mxact_abort WHERE id = 1 FOR KEY SHARE; +id|filler +--+------- + 1|initial +(1 row) + +step s1abort: ROLLBACK; +step s3keyupdate: UPDATE mxact_abort SET id = 2 WHERE id = 1; +step wake: + SELECT FROM injection_points_detach('transaction-abort-after-clog'); + SELECT FROM injection_points_wakeup('transaction-abort-after-clog'); + +step s3keyupdate: <... completed> +step s1abort: <... completed> +step s3check: SELECT * FROM mxact_abort ORDER BY id; +id|filler +--+------- + 2|initial +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index b4a0079484b..60148f96fda 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -43,6 +43,7 @@ injection_points_isolation = [ 'syscache-update-pruned', 'heap_lock_update', 'on_conflict_probe_window', + 'multixact-aborted-updater', ] # wait_cleanup terminates another backend, whose FATAL message can be lost diff --git a/src/test/modules/injection_points/specs/multixact-aborted-updater.spec b/src/test/modules/injection_points/specs/multixact-aborted-updater.spec new file mode 100644 index 00000000000..98d265a2e94 --- /dev/null +++ b/src/test/modules/injection_points/specs/multixact-aborted-updater.spec @@ -0,0 +1,107 @@ +# Test concurrent behavior of transaction abort in the window between marking +# the transaction as aborted in pg_xact and removing it from the ProcArray. +# +# In most cases, the correct way to check if a transaction aborted is to first +# check TransactionIdIsInProgress(), followed by !TransactionDidCommit(). The +# TransactionIdIsInProgress() call ensures that the transaction is only +# considered as aborted after it's been removed from the ProcArray, and using +# !TransactionDidCommit() instead of TransactionIdDidAbort() ensures that you +# treat crashed transactions -- i.e. transactions that were in progress when the +# system crashed and didn't write an ABORT WAL record -- as aborted too. +# +# This test uses an injection point to pause an aborting transaction between +# updating pg_xact and removing it from the ProcArray, and performs different +# concurrent operations involving multixids. This exercises various codepaths +# where the correct ordering of TransactionIdIsInProgress() and +# TransactionIdDidCommit() matters. + +setup +{ + CREATE EXTENSION injection_points; + + CREATE TABLE mxact_abort (id int PRIMARY KEY, filler text); + INSERT INTO mxact_abort VALUES (1, 'initial'); +} + +teardown +{ + DROP TABLE mxact_abort; + DROP EXTENSION injection_points; +} + +# Transaction 1 performs an UPDATE, aborts, and pauses between updating pg_xact +# and ProcArray cleanup +session s1 +setup { + SELECT FROM injection_points_set_local(); + SELECT FROM injection_points_attach('transaction-abort-after-clog', 'wait'); +} +step s1begin { BEGIN; } +step s1update { UPDATE mxact_abort SET filler = 's1' WHERE id = 1; } +step s1abort { ROLLBACK; } + +# Transaction 2 locks the row in KEY SHARE mode, to cause a multixact to be +# created. +session s2 +step s2lock { SELECT * FROM mxact_abort WHERE id = 1 FOR KEY SHARE; } + +# Transaction 3 performs a concurrent operation while transaction 1 is aborting +session s3 +step s3nowait { SELECT * FROM mxact_abort WHERE id = 1 FOR NO KEY UPDATE NOWAIT; } +step s3update { UPDATE mxact_abort SET filler = 's3' WHERE id = 1; } +step s3delete { DELETE FROM mxact_abort WHERE id = 1; } +step s3keyupdate { UPDATE mxact_abort SET id = 2 WHERE id = 1; } +step s3check { SELECT * FROM mxact_abort ORDER BY id; } + +session waker +step wake { + SELECT FROM injection_points_detach('transaction-abort-after-clog'); + SELECT FROM injection_points_wakeup('transaction-abort-after-clog'); +} + +# A SELECT .. NOWAIT while the updating transaction is paused in the abort +# considers the updater as still running, and reports an error. +permutation + s1begin + s1update + s2lock # succeeds and creates a multixid + s1abort # blocks on the injection point + # fails, the aborting updater is still considered running + s3nowait + wake + +# A conflicting UPDATE waits for the first updater to fully finish. +# +# Report the abort after the conflicting operation in these permutations +# to keep the completion output stable. +permutation + s1begin + s1update + s2lock + s1abort(s3update) + # blocks waiting for the aborting updater to finish + s3update + wake + +# Same for a DELETE +permutation + s1begin + s1update + s2lock + s1abort(s3delete) + # blocks waiting for the aborting updater to finish + s3delete + wake + s3check + +# A key-changing UPDATE must also wait before deciding which members of +# the multixact survive. +permutation + s1begin + s1update + s2lock + s1abort(s3keyupdate) + # blocks waiting for the aborting updater to finish + s3keyupdate + wake + s3check diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index e1cf57ff611..b6ac9dd07a7 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -68,6 +68,7 @@ tests += { 't/057_snapshot_commit_race.pl', 't/058_shutdown_crash_restart.pl', 't/059_remote_apply_status_interval.pl', + 't/060_multixact_crashed_updater.pl', ], }, } diff --git a/src/test/recovery/t/060_multixact_crashed_updater.pl b/src/test/recovery/t/060_multixact_crashed_updater.pl new file mode 100644 index 00000000000..3379a5cdc49 --- /dev/null +++ b/src/test/recovery/t/060_multixact_crashed_updater.pl @@ -0,0 +1,64 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Test that a crashed updating transaction that is part of a multixid is treated +# as aborted. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf('postgresql.conf', 'max_prepared_transactions = 10'); +$node->start; + +$node->safe_psql('postgres', q( + CREATE TABLE mx_crashed (id int PRIMARY KEY, payload text); + INSERT INTO mx_crashed VALUES (1, 'initial'); +)); + +# Keep the updater open while another transaction locks the same row. This +# creates a multixid with the updating and locking xids as members. +my $updater = $node->background_psql('postgres'); +$updater->query_safe('BEGIN'); +$updater->query_safe(q(UPDATE mx_crashed SET payload = 'crashed' WHERE id = 1)); + +$node->safe_psql('postgres', q( + BEGIN; + SELECT id FROM mx_crashed WHERE id = 1 FOR KEY SHARE; + PREPARE TRANSACTION 'mx_crashed_locker'; +)); + +# The PREPARE TRANSACTION flushed the preceding heap update and multixact WAL +# records, too. Kill the server to leave the updating transaction as crashed, +# ie. it is marked neither as committed nor aborted in pg_xact. +$node->stop('immediate'); +$updater->{run}->finish; +$node->start; + +# Update the row again after restart. The crashed updater should be ignored, but +# the FOR KEY SHARE lock should remain. +my ($stdout, $stderr); +my $ret = $node->psql('postgres', q( + BEGIN ISOLATION LEVEL REPEATABLE READ; + UPDATE mx_crashed SET payload = 'after crash' WHERE id = 1; + COMMIT; +), stdout => \$stdout, stderr => \$stderr); +is($ret, 0, 'repeatable read update ignores the crashed updater') + or diag($stderr); + +is($node->safe_psql('postgres', + q(SELECT payload FROM mx_crashed WHERE id = 1)), + 'after crash', 'row was updated'); + +# Check that the row remains locked for the locking transaction +$node->psql('postgres', 'SELECT * FROM mx_crashed FOR UPDATE NOWAIT', + stderr => \$stderr); +like($stderr, qr/could not obtain lock on row in relation/, + "prepared share locker survives after crash"); + +$node->stop(); +done_testing(); -- 2.47.3