From 964e37c770e68946c9686c2d998ffc0673b986c2 Mon Sep 17 00:00:00 2001 From: "chee.wooson" Date: Thu, 30 Jul 2026 12:00:40 +0800 Subject: [PATCH v2] Fix exported snapshot xmin handoff race ProcArrayInstallImportedXmin() verifies that the source transaction is still running and then installs the imported xmin. Both steps have to be serialized with concurrent proc-array horizon computations; otherwise VACUUM can scan the importer before the xmin is installed while the source transaction is still allowed to clear its xmin concurrently. Take ProcArrayLock in exclusive mode while importing an exported snapshot. That makes the xmin handoff atomic with respect to ComputeXidHorizons(), while keeping the no-importer transaction end path unchanged. Add an injection-point TAP test that pauses VACUUM inside ComputeXidHorizons() while it holds ProcArrayLock shared, starts SET TRANSACTION SNAPSHOT concurrently, and verifies that the importer waits for ProcArrayLock before the imported snapshot is allowed to protect the deleted tuple. --- src/backend/storage/ipc/procarray.c | 13 +- src/test/modules/test_misc/meson.build | 1 + .../test_misc/t/015_export_snapshot.pl | 261 ++++++++++++++++++ 3 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 src/test/modules/test_misc/t/015_export_snapshot.pl diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 60336b31803..f5ed9c1c7a1 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -1736,6 +1736,15 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h) TransactionId xid; TransactionId xmin; +#ifdef USE_INJECTION_POINTS + { + char ip_name[64]; + + snprintf(ip_name, sizeof(ip_name), + "compute-xid-horizons-at-%d", index); + InjectionPointRun(ip_name, NULL); + } +#endif /* Fetch xid just once - see GetNewTransactionId */ xid = UINT32_ACCESS_ONCE(other_xids[index]); xmin = UINT32_ACCESS_ONCE(proc->xmin); @@ -1945,6 +1954,8 @@ GetOldestNonRemovableTransactionId(Relation rel) { ComputeXidHorizonsResult horizons; + INJECTION_POINT("get-oldest-nonremovable-txid", NULL); + ComputeXidHorizons(&horizons); switch (GlobalVisHorizonKindForRel(rel)) @@ -2488,7 +2499,7 @@ ProcArrayInstallImportedXmin(TransactionId xmin, return false; /* Get lock so source xact can't end while we're doing this */ - LWLockAcquire(ProcArrayLock, LW_SHARED); + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); /* * Find the PGPROC entry of the source transaction. (This could use diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build index ee290698b31..eb48ee35d1d 100644 --- a/src/test/modules/test_misc/meson.build +++ b/src/test/modules/test_misc/meson.build @@ -23,6 +23,7 @@ tests += { 't/012_ddlutils.pl', 't/013_temp_obj_multisession.pl', 't/014_log_statement_max_length.pl', + 't/015_export_snapshot.pl', ], # The injection points are cluster-wide, so disable installcheck 'runningcheck': false, diff --git a/src/test/modules/test_misc/t/015_export_snapshot.pl b/src/test/modules/test_misc/t/015_export_snapshot.pl new file mode 100644 index 00000000000..1cc9b482965 --- /dev/null +++ b/src/test/modules/test_misc/t/015_export_snapshot.pl @@ -0,0 +1,261 @@ +# Copyright (c) 2024-2026, PostgreSQL Global Development Group +# +# Test: reproduce the exported-snapshot xmin handoff race. +# +# Strategy (two-injection-point approach, for the exclusive-lock fix): +# 1. VACUUM attaches "get-oldest-nonremovable-txid" with injection_wait +# (PID-filtered via injection_points_set_local). +# 2. Start VACUUM. The first ComputeXidHorizons call (from on-access +# catalog pruning via GlobalVisUpdate) passes through unblocked. +# 3. VACUUM reaches GetOldestNonRemovableTransactionId -> blocked at +# "get-oldest-nonremovable-txid". +# 4. Coordinator detects the block -> GLOBALLY attaches +# "compute-xid-horizons-at-1" with injection_wait (no PID filter). +# 5. Coordinator wakes VACUUM from "get-oldest-nonremovable-txid". +# 6. VACUUM enters ComputeXidHorizons (holding ProcArrayLock shared) +# -> blocked at "compute-xid-horizons-at-1" (after scanning importer +# at index 0 with xmin=Invalid). +# 7. Coordinator detects the second block, detaches the injection point so +# later snapshots do not hit it, and starts SET TRANSACTION SNAPSHOT +# asynchronously. The fix makes ProcArrayInstallImportedXmin wait for +# ProcArrayLock exclusive instead of installing xmin while VACUUM is in +# the middle of a proc-array scan. +# 8. Coordinator verifies that the importer is waiting, then wakes +# "compute-xid-horizons-at-1". +# 9. VACUUM continues while the source transaction is still open, so its +# horizon remains low enough to preserve the deleted tuple. Afterwards +# the importer installs the snapshot. +# 10. Importer queries -> 1 row. +# +# Without the fix, SET TRANSACTION SNAPSHOT completes under a shared +# ProcArrayLock while VACUUM is paused, so the lock-wait assertion fails. +# +# Depends only on: injection_points (built-in test module) + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; +use Time::HiRes qw(usleep); + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node = PostgreSQL::Test::Cluster->new('export_race_node'); +$node->init; +$node->append_conf('postgresql.conf', + "shared_preload_libraries = 'injection_points'"); +$node->start; + +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points'); + +$node->safe_psql('postgres', qq{ + CREATE TABLE race_test (id int, data text); + INSERT INTO race_test VALUES (1, 'should_be_visible'); +}); + +# ComputeXidHorizons iterates over pgprocnos[], which contains only +# client backends that called ProcArrayAdd(). The test relies on the importer +# preceding the source transaction, with at least one entry between them. +my $imp = $node->background_psql('postgres'); # index 0 +my $fill = $node->background_psql('postgres'); # index 1 (gap) +my $src = $node->background_psql('postgres'); # index 2 (source) +my $vac = $node->background_psql('postgres'); # index 3 (VACUUM) +my $del = $node->background_psql('postgres'); # index 4 (deleter) +my $coord = $node->background_psql('postgres'); + +my $imp_pid = $imp->query("SELECT pg_backend_pid()"); +$imp_pid =~ s/\s+//g; + +$src->query("BEGIN ISOLATION LEVEL REPEATABLE READ"); +$src->query("SELECT * FROM race_test"); +my $token = $src->query("SELECT pg_export_snapshot()"); +$token =~ s/\s+//g; +diag("exported snapshot token: $token"); + +$del->query("DELETE FROM race_test WHERE id = 1"); +$del->query("COMMIT"); +diag("deleter committed"); + +# Advance the XID counter so that the horizon (latestCompletedXid + 1 when +# no backend has a valid xmin) is strictly greater than the deleter's XID. +for (my $i = 0; $i < 100; $i++) +{ + $node->safe_psql('postgres', "SELECT txid_current()"); +} +diag("100 filler XIDs consumed"); + +# No query after BEGIN: importer xmin stays Invalid until SET TRANSACTION +# SNAPSHOT, which is essential for this race. +$imp->query("BEGIN ISOLATION LEVEL REPEATABLE READ"); + +my $vac_pid = $vac->query("SELECT pg_backend_pid()"); +$vac_pid =~ s/\s+//g; +diag("VACUUM PID: $vac_pid"); + +$vac->query("SELECT injection_points_set_local()"); +$vac->query( + "SELECT injection_points_attach('get-oldest-nonremovable-txid', 'wait')"); +diag("VACUUM attached get-oldest-nonremovable-txid"); + +$vac->query_until(qr/vac_started/, + "\\echo vac_started\nVACUUM race_test;\n"); +diag("VACUUM started"); + +{ + my $blocked = 0; + for (my $i = 0; $i < 1800; $i++) + { + my $result = $coord->query( + "SELECT count(*) = 1 FROM pg_stat_activity" + . " WHERE pid = $vac_pid" + . " AND wait_event_type = 'InjectionPoint'"); + if ($result =~ /t/) + { + $blocked = 1; + last; + } + usleep(100_000); + } + die "VACUUM did not reach first injection point within 180s" + unless $blocked; +} +diag("VACUUM blocked at get-oldest-nonremovable-txid"); + +$coord->query( + "SELECT injection_points_attach('compute-xid-horizons-at-1', 'wait')"); +diag("coordinator attached compute-xid-horizons-at-1"); + +$coord->query( + "SELECT injection_points_wakeup('get-oldest-nonremovable-txid')"); +diag("woke VACUUM from get-oldest-nonremovable-txid"); + +{ + my $blocked = 0; + for (my $i = 0; $i < 1800; $i++) + { + my $result = $coord->query( + "SELECT count(*) = 1 FROM pg_stat_activity" + . " WHERE pid = $vac_pid" + . " AND wait_event_type = 'InjectionPoint'"); + if ($result =~ /t/) + { + $blocked = 1; + last; + } + usleep(100_000); + } + die "VACUUM did not reach second injection point within 180s" + unless $blocked; +} +diag("VACUUM blocked at compute-xid-horizons-at-1"); + +$coord->query( + "SELECT injection_points_detach('compute-xid-horizons-at-1')"); +diag("detached compute-xid-horizons-at-1 while VACUUM remains blocked"); + +# An exported snapshot that is never imported must retain the normal no-XID +# commit path: it should not wait for ProcArrayLock. +$fill->query("BEGIN ISOLATION LEVEL REPEATABLE READ"); +my $unused_token = $fill->query("SELECT pg_export_snapshot()"); +$unused_token =~ s/\s+//g; + +my $fill_pid = $fill->query("SELECT pg_backend_pid()"); +$fill_pid =~ s/\s+//g; +$fill->query_until(qr/unreferenced_commit_started/, + "\\echo unreferenced_commit_started\nCOMMIT;\n" + . "\\echo unreferenced_commit_done\n"); + +my $unreferenced_finished = 0; +for (my $i = 0; $i < 100; $i++) +{ + my $result = $coord->query( + "SELECT CASE" + . " WHEN state = 'idle' THEN 'done'" + . " WHEN wait_event_type = 'LWLock'" + . " AND wait_event = 'ProcArray' THEN 'blocked'" + . " ELSE 'running' END" + . " FROM pg_stat_activity WHERE pid = $fill_pid"); + if ($result =~ /done/) + { + $unreferenced_finished = 1; + last; + } + last if $result =~ /blocked/; + usleep(100_000); +} +diag("unreferenced export started COMMIT while VACUUM holds ProcArrayLock"); + +$imp->query_until(qr/import_started/, + "\\echo import_started\nSET TRANSACTION SNAPSHOT '$token';\n" + . "\\echo import_done\n"); +diag("importer started SET TRANSACTION SNAPSHOT"); + +my $importer_waiting = 0; +for (my $i = 0; $i < 100; $i++) +{ + my $result = $coord->query( + "SELECT state || '|' || COALESCE(wait_event_type, '') || '|' ||" + . " COALESCE(wait_event, '')" + . " FROM pg_stat_activity" + . " WHERE pid = $imp_pid"); + if ($result =~ /active\|LWLock\|ProcArray/) + { + $importer_waiting = 1; + last; + } + last if $result eq ''; + usleep(100_000); +} + +$coord->query( + "SELECT injection_points_detach('get-oldest-nonremovable-txid')"); +$coord->query( + "SELECT injection_points_wakeup('compute-xid-horizons-at-1')"); +diag("detached remaining injection point and woke VACUUM"); + +$imp->query_until(qr/import_done/, ""); +$src->query("COMMIT"); +$fill->query_until(qr/unreferenced_commit_done/, ""); + +ok($unreferenced_finished, + "unreferenced export clears xmin without waiting for ProcArrayLock"); +ok($importer_waiting, + "importing transaction waits for ProcArrayLock while VACUUM computes horizons"); + +{ + my $done = 0; + for (my $i = 0; $i < 1800; $i++) + { + my $result = $coord->query( + "SELECT count(*) = 1 FROM pg_stat_activity" + . " WHERE pid = $vac_pid" + . " AND state = 'idle'"); + if ($result =~ /t/) + { + $done = 1; + last; + } + usleep(100_000); + } + die "VACUUM did not finish within 180s" unless $done; +} +diag("VACUUM finished"); + +my $count = $imp->query("SELECT count(*) FROM race_test"); +$count =~ s/\s+//g; +diag("importer sees $count row(s)"); + +is($count, 1, + "imported snapshot still sees the row after concurrent VACUUM") + or diag("BUG DETECTED: export-snapshot xmin race caused " + . "premature tuple removal (expected 1 row, got $count)"); + +$imp->query("COMMIT"); + +$node->stop; +done_testing(); -- 2.43.0