From 7cafc4abbdc7b63fa6bbc948347392560dd2451b Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Thu, 15 Oct 2026 11:23:06 +0300 Subject: [PATCH] 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 Tested-by: Lionel Hamayon Discussion: https://www.postgresql.org/message-id/5778D6297D3CCF21+202609151836512678486@vastdata.com.cn Backpatch-through: 14 --- src/backend/access/heap/heapam.c | 30 +++++--- src/test/recovery/meson.build | 1 + .../t/060_multixact_crashed_updater.pl | 75 +++++++++++++++++++ 3 files changed, 97 insertions(+), 9 deletions(-) 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 15a2a621479..98af4087faa 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3393,14 +3393,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)) { @@ -7725,8 +7736,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/test/recovery/meson.build b/src/test/recovery/meson.build index d985fac0850..172e9c7b0f9 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -50,6 +50,7 @@ tests += { 't/054_unlogged_sequence_promotion.pl', 't/055_cascade_reconnect.pl', 't/056_standby_snapshot_export.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..85f25722fd1 --- /dev/null +++ b/src/test/recovery/t/060_multixact_crashed_updater.pl @@ -0,0 +1,75 @@ +# 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();