Hello, everyone! I've (or AI?) found another fast-path/foreign-key - related problem which the stress-test framework I am prototyping detected. I think this's okay to send it to that thread; if not, please tell me, and I'll create another one.
It was all built by Claude; sorry, I have no cycles to carefully validate all details (there are many findings like this), but it looks valid - it reproduces the error and even data corruption with just two new injection points. I think Amit will be able to understand it all clearly and easily as the author. If not for some reason, please tell me, and I'll handle it myself later. That's a strange time to live :) Best regards, Mikhail.
From 2bd62b7ac4353d8bd308a0b23885676ba61e8b68 Mon Sep 17 00:00:00 2001 From: nkey <[email protected]> Date: Sat, 1 Aug 2026 22:23:30 +0200 Subject: [PATCH v1 1/3] Reproducer: RI fast path opens an index a concurrent REINDEX dropped The RI fast path looks up the constraint, takes RowShareLock on the referenced table, and opens the index conindid names. Reading conindid before that lock is not safe. REINDEX CONCURRENTLY repoints the constraint at a new index and then drops the old one, and it waits only for backends holding a lock on the referenced table; a backend that has read the constraint but not yet taken that lock is not one of them. It then opens an index that is already gone, and the write fails with "could not open relation with OID". This commit only demonstrates the problem, it does not fix it. It adds the injection point the test needs, at the point where conindid has been read and the referenced table is not locked yet. There is no way to park a backend in that window without one: anything holding a conflicting lock on the referenced table would block the rebuild as well, so the two would deadlock rather than race. The test pins the rebuild first, at the swap, once its own waiting is behind it, and only then pauses a writer in that window. The order matters: REINDEX CONCURRENTLY waits for older snapshots, so a writer paused mid-statement would block the rebuild rather than race it. It checks that the write completes, that the row really was updated, and that the constraint still rejects a row with no referenced key -- a fix that skipped the check would pass the first of those and fail the last. Without a fix the test fails with ERROR: could not open relation with OID 16399 and the row is left unchanged. Found by the CONCURRENTLY stress suite, whose foreign key scenario had about one run in three fail this way. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- src/backend/utils/adt/ri_triggers.c | 13 ++ src/test/modules/test_misc/meson.build | 1 + .../test_misc/t/015_ri_fastpath_reindex.pl | 148 ++++++++++++++++++ 3 files changed, 162 insertions(+) create mode 100644 src/test/modules/test_misc/t/015_ri_fastpath_reindex.pl diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 627a9fb38ea..17c230c9d32 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -48,6 +48,7 @@ #include "utils/fmgroids.h" #include "utils/guc.h" #include "utils/hsearch.h" +#include "utils/injection_point.h" #include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" @@ -2823,6 +2824,12 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo, CommandCounterIncrement(); snapshot = RegisterSnapshot(GetTransactionSnapshot()); + /* + * conindid has been read but the referenced table is not locked yet, + * which is the window a concurrent rebuild of that index gets into. + */ + INJECTION_POINT("ri-before-pk-lock", NULL); + pk_rel = table_open(riinfo->pk_relid, RowShareLock); idx_rel = index_open(riinfo->conindid, AccessShareLock); @@ -4385,6 +4392,12 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) * We don't release these locks until end of transaction, matching SPI * behavior. */ + /* + * conindid has been read but the referenced table is not locked yet, + * which is the window a concurrent rebuild of that index gets into. + */ + INJECTION_POINT("ri-before-pk-lock", NULL); + entry->pk_rel = table_open(riinfo->pk_relid, RowShareLock); entry->idx_rel = index_open(riinfo->conindid, AccessShareLock); entry->pk_slot = table_slot_create(entry->pk_rel, NULL); diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build index ee290698b31..deed584125f 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_ri_fastpath_reindex.pl', ], # The injection points are cluster-wide, so disable installcheck 'runningcheck': false, diff --git a/src/test/modules/test_misc/t/015_ri_fastpath_reindex.pl b/src/test/modules/test_misc/t/015_ri_fastpath_reindex.pl new file mode 100644 index 00000000000..142be4560e1 --- /dev/null +++ b/src/test/modules/test_misc/t/015_ri_fastpath_reindex.pl @@ -0,0 +1,148 @@ + +# Copyright (c) 2026, PostgreSQL Global Development Group + +# A foreign key check racing a rebuild of the index it resolves through. +# +# The RI fast path looks up the constraint, takes RowShareLock on the +# referenced table, and opens the index named by conindid. Reading +# conindid before taking that lock is not safe: REINDEX CONCURRENTLY +# repoints the constraint at a new index and drops the old one, and it +# waits only for backends that hold a lock on the referenced table. A +# backend that has read the constraint but not yet taken that lock is not +# one of them, so the index it is about to open can be gone by then. +# +# The rebuild is pinned first, once its own waiting is behind it. Doing +# it the other way round does not work: REINDEX CONCURRENTLY waits for +# older snapshots, so a writer paused mid-statement would block the +# rebuild rather than race it. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node = PostgreSQL::Test::Cluster->new('node'); +$node->init; +$node->start; + +if (!$node->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points'); + +$node->safe_psql( + 'postgres', q[ + CREATE TABLE pk (id int PRIMARY KEY); + INSERT INTO pk SELECT g FROM generate_series(1, 100) g; + CREATE TABLE fk (id int PRIMARY KEY, pid int REFERENCES pk(id)); + INSERT INTO fk SELECT g, g FROM generate_series(1, 100) g; +]); + +my $before = $node->safe_psql('postgres', + q[SELECT conindid FROM pg_constraint WHERE conname = 'fk_pid_fkey']); + +# The rebuild goes first, and stops at the swap: from here on it wants +# nothing from other backends until it takes its own locks. +my $rebuild = $node->background_psql('postgres', on_error_stop => 0); +$rebuild->query_safe( + q[ + SELECT injection_points_set_local(); + SELECT injection_points_attach('reindex-relation-concurrently-before-swap', 'wait'); +]); +$rebuild->query_until( + qr/rebuilding/, q[ +\echo rebuilding +REINDEX INDEX CONCURRENTLY pk_pkey; +]); + +ok( $node->poll_query_until( + 'postgres', q[ + SELECT count(*) > 0 FROM pg_stat_activity + WHERE wait_event = 'reindex-relation-concurrently-before-swap']), + 'the rebuild is past its waiting'); + +# Now a writer reads the constraint and stops before it locks the +# referenced table. It holds no lock there, so the rebuild below never +# waits for it. +my $writer = $node->background_psql('postgres', on_error_stop => 0); +$writer->query_safe( + q[ + SELECT injection_points_set_local(); + SELECT injection_points_attach('ri-before-pk-lock', 'wait'); +]); +$writer->query_until( + qr/writing/, q[ +\echo writing +UPDATE fk SET pid = 42 WHERE id = 1; +]); + +ok( $node->poll_query_until( + 'postgres', q[ + SELECT count(*) > 0 FROM pg_stat_activity + WHERE wait_event = 'ri-before-pk-lock']), + 'the foreign key check has read the constraint'); + +# Let the rebuild finish. It repoints the constraint and drops the index +# the writer read out of it. +$node->safe_psql('postgres', + q[SELECT injection_points_wakeup('reindex-relation-concurrently-before-swap')] +); +$node->safe_psql('postgres', + q[SELECT injection_points_detach('reindex-relation-concurrently-before-swap')] +); + +ok( $node->poll_query_until( + 'postgres', + "SELECT count(*) = 0 FROM pg_class WHERE oid = $before"), + 'the index the check read has been dropped'); + +isnt( + $node->safe_psql('postgres', + q[SELECT conindid FROM pg_constraint WHERE conname = 'fk_pid_fkey']), + $before, + 'the constraint names a different index now'); + +# The check has to resolve the constraint to the index it names now. +$node->safe_psql('postgres', + q[SELECT injection_points_wakeup('ri-before-pk-lock')]); +$node->safe_psql('postgres', + q[SELECT injection_points_detach('ri-before-pk-lock')]); + +my $banner = 'done_marker'; +$writer->{stdin} .= "\\echo $banner\n\\warn $banner\n"; +pump_until($writer->{run}, $writer->{timeout}, \$writer->{stdout}, + qr/$banner/); +pump_until($writer->{run}, $writer->{timeout}, \$writer->{stderr}, + qr/$banner/); +my $err = $writer->{stderr}; +$err =~ s/$banner//g; +$err =~ s/\s+/ /g; +$err =~ s/^\s+|\s+$//g; + +is($err, '', 'the foreign key check survived the rebuild'); + +$writer->quit; +$rebuild->quit; + +is($node->safe_psql('postgres', 'SELECT pid FROM fk WHERE id = 1'), + '42', 'the row was updated'); + +# The constraint must still be enforced, not merely not crashing. +my (undef, undef, $viol) = $node->psql('postgres', + 'INSERT INTO fk VALUES (999, 12345);', on_error_stop => 0); +like( + $viol, + qr/violates foreign key constraint/, + 'the constraint is still enforced'); + +$node->stop; +done_testing(); -- 2.54.0.windows.1
From b97fa3c645e4e9b0b74064beb06b326fe98f9160 Mon Sep 17 00:00:00 2001 From: nkey <[email protected]> Date: Sat, 1 Aug 2026 22:49:46 +0200 Subject: [PATCH v1 3/3] Re-read conindid under the referenced table's lock in the RI fast path The RI fast path looks up the constraint, takes RowShareLock on the referenced table, and opens the index conindid names. Reading conindid before that lock is not safe. REINDEX CONCURRENTLY repoints the constraint at a new index and then drops the old one, and it waits only for backends holding a lock on the referenced table; a backend that has read the constraint but not yet taken that lock is not one of them. It then opens an index that is already gone, and the write fails with "could not open relation with OID". An interrupted rebuild leaves the old index dead rather than dropped, and opening that one is worse: it is no longer vacuumed, so its entries can point at line pointers the heap has handed out again, and the check reports a referenced row that does not exist. Re-read the constraint once the table is locked. Locking it is what makes the value stable: index_drop() removes the old index only after the swap that repointed conindid has committed, and only after waiting for the table's lockers, so afterwards we either see the new index or an old one that cannot go away until this transaction ends. Both reproducers added earlier now pass: the write completes and the row is updated in 015, and in 016 the write is rejected and leaves no foreign key row without a referenced row. A fix that skipped the check would pass the first of those and fail the second. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- src/backend/utils/adt/ri_triggers.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 17c230c9d32..4abdcca59df 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -2831,6 +2831,10 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo, INJECTION_POINT("ri-before-pk-lock", NULL); pk_rel = table_open(riinfo->pk_relid, RowShareLock); + + /* Re-read the constraint under that lock; see ri_FastPathGetEntry(). */ + riinfo = ri_LoadConstraintInfo(riinfo->constraint_id); + idx_rel = index_open(riinfo->conindid, AccessShareLock); slot = table_slot_create(pk_rel, NULL); @@ -4399,6 +4403,21 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) INJECTION_POINT("ri-before-pk-lock", NULL); entry->pk_rel = table_open(riinfo->pk_relid, RowShareLock); + + /* + * Re-read the constraint now that the PK table is locked, because + * conindid may have been read before that lock was taken and REINDEX + * CONCURRENTLY moves a constraint to a new index. Locking the PK + * table is what makes the value we read here stable: index_drop() + * removes the old index only after the swap that repointed conindid + * has committed, and only after waiting for the lockers of the table, + * so we either see the new index or an old one that cannot go away + * until this transaction ends. Without this we could open an index + * that has already been dropped, or scan one that has been marked + * dead and so no longer receives new rows. + */ + riinfo = ri_LoadConstraintInfo(riinfo->constraint_id); + entry->idx_rel = index_open(riinfo->conindid, AccessShareLock); entry->pk_slot = table_slot_create(entry->pk_rel, NULL); -- 2.54.0.windows.1
From bed7d7ab10d28856be311a3d02d94b0de96bd711 Mon Sep 17 00:00:00 2001 From: nkey <[email protected]> Date: Sat, 1 Aug 2026 22:45:41 +0200 Subject: [PATCH v1 2/3] Reproducer: RI fast path accepts a row whose referenced key is gone Same window as the previous commit, worse outcome. The index the check opens has not been dropped here, only marked dead, which is what an interrupted REINDEX CONCURRENTLY leaves behind. A dead index is left out of RelationGetIndexList(), so VACUUM never cleans it, and its entries keep pointing at line pointers the heap is free to hand out again. The check finds a stale entry for a key that was deleted, fetches whatever tuple took the slot over, and reports the referenced row as present -- a btree scan does not recheck the key against the heap tuple, and ri_FastPathProbeOne() only rechecks when the tuple was concurrently updated. The write is accepted and a row with no referenced key is committed. Nothing errors out, and the orphan outlives the race. This commit only demonstrates the problem, it does not fix it. It adds an injection point at the start of phase 6 of REINDEX CONCURRENTLY, where the old indexes are dead and committed as such. Attached as 'error' it leaves the state an interrupted rebuild leaves. The test needs the rebuild gone rather than paused: it holds a session-level ShareUpdateExclusiveLock on the table from start to drop, and VACUUM wants that lock too, so no vacuum can interleave while it is in flight. The preconditions are asserted rather than assumed -- that the old index is dead but still there, that the constraint has moved, that the new row really took the freed slot, and that the referenced key is really absent -- so the test fails at the first one that stops holding instead of quietly testing nothing. Without a fix the write is accepted and SELECT count(*) FROM fk f WHERE NOT EXISTS (SELECT 1 FROM pk WHERE pk.id = f.pid) returns 1. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- src/backend/commands/indexcmds.c | 8 + src/test/modules/test_misc/meson.build | 1 + .../t/016_ri_fastpath_stale_index.pl | 186 ++++++++++++++++++ 3 files changed, 195 insertions(+) create mode 100644 src/test/modules/test_misc/t/016_ri_fastpath_stale_index.pl diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 3790b8e1252..4ee545697c1 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -4523,6 +4523,14 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein * Drop the old indexes. */ + /* + * The old indexes are dead and committed as such, and the session locks + * are still held. Erroring out here leaves behind what an interrupted + * rebuild leaves behind: an index that is no longer live, so no longer + * vacuumed, and that outlives this command. + */ + INJECTION_POINT("reindex-relation-concurrently-before-drop", NULL); + pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_5); WaitForLockersMultiple(lockTags, AccessExclusiveLock, true); diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build index deed584125f..6aa026556a8 100644 --- a/src/test/modules/test_misc/meson.build +++ b/src/test/modules/test_misc/meson.build @@ -24,6 +24,7 @@ tests += { 't/013_temp_obj_multisession.pl', 't/014_log_statement_max_length.pl', 't/015_ri_fastpath_reindex.pl', + 't/016_ri_fastpath_stale_index.pl', ], # The injection points are cluster-wide, so disable installcheck 'runningcheck': false, diff --git a/src/test/modules/test_misc/t/016_ri_fastpath_stale_index.pl b/src/test/modules/test_misc/t/016_ri_fastpath_stale_index.pl new file mode 100644 index 00000000000..26c950707a2 --- /dev/null +++ b/src/test/modules/test_misc/t/016_ri_fastpath_stale_index.pl @@ -0,0 +1,186 @@ + +# Copyright (c) 2026, PostgreSQL Global Development Group + +# A foreign key check that accepts a row whose referenced key does not +# exist, by resolving the constraint to an index that is no longer live. +# +# Same window as 015_ri_fastpath_reindex.pl: the RI fast path reads +# conindid before it locks the referenced table, so a concurrent rebuild +# can move the constraint elsewhere in between. Here the index the check +# opens has not been dropped, only marked dead, which is what an +# interrupted REINDEX CONCURRENTLY leaves behind. +# +# A dead index is left out of RelationGetIndexList(), so VACUUM never +# cleans it. Its entries keep pointing at line pointers that the heap is +# free to hand out again. The check then finds a stale entry for a key +# that was deleted, fetches the tuple that took the slot over, and -- a +# btree scan does not recheck the key against the heap tuple -- reports +# the referenced row as present. The write is accepted and a row with no +# referenced key is committed. +# +# The rebuild has to be out of the way before the vacuum: it holds a +# session-level ShareUpdateExclusiveLock on the table from the moment it +# starts until the old indexes are dropped, and VACUUM wants that lock +# too. So the rebuild errors out after the old index is dead, which +# releases the lock and leaves the index behind. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node = PostgreSQL::Test::Cluster->new('node'); +$node->init; +$node->start; + +if (!$node->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points'); + +# One page worth of rows, so the slot freed below is the only one a new +# row can take. Nothing references 42, so it can be deleted. +$node->safe_psql( + 'postgres', q[ + CREATE TABLE pk (id int PRIMARY KEY); + INSERT INTO pk SELECT g FROM generate_series(1, 10) g; + INSERT INTO pk VALUES (42); + CREATE TABLE fk (id int PRIMARY KEY, pid int REFERENCES pk(id)); + INSERT INTO fk SELECT g, g FROM generate_series(1, 10) g; +]); + +my $slot = $node->safe_psql('postgres', + q[SELECT ctid FROM pk WHERE id = 42]); + +# Delete it, but do not vacuum: the entry for 42 stays in the index the +# constraint names right now. +$node->safe_psql('postgres', q[DELETE FROM pk WHERE id = 42]); + +my $before = $node->safe_psql('postgres', + q[SELECT conindid FROM pg_constraint WHERE conname = 'fk_pid_fkey']); + +# The rebuild goes first and stops at the swap, past its own waiting. It +# is also told to fail once the old index is dead. +my $rebuild = $node->background_psql('postgres', on_error_stop => 0); +$rebuild->query_safe( + q[ + SELECT injection_points_set_local(); + SELECT injection_points_attach('reindex-relation-concurrently-before-swap', 'wait'); + SELECT injection_points_attach('reindex-relation-concurrently-before-drop', 'error'); +]); +$rebuild->query_until( + qr/rebuilding/, q[ +\echo rebuilding +REINDEX INDEX CONCURRENTLY pk_pkey; +]); + +ok( $node->poll_query_until( + 'postgres', q[ + SELECT count(*) > 0 FROM pg_stat_activity + WHERE wait_event = 'reindex-relation-concurrently-before-swap']), + 'the rebuild is past its waiting'); + +# The writer reads the constraint and stops before it locks the +# referenced table. It holds no lock there, so the rebuild below never +# waits for it. Its snapshot is younger than the delete above, so it +# does not hold the vacuum back either. +my $writer = $node->background_psql('postgres', on_error_stop => 0); +$writer->query_safe( + q[ + SELECT injection_points_set_local(); + SELECT injection_points_attach('ri-before-pk-lock', 'wait'); +]); +$writer->query_until( + qr/writing/, q[ +\echo writing +UPDATE fk SET pid = 42 WHERE id = 1; +]); + +ok( $node->poll_query_until( + 'postgres', q[ + SELECT count(*) > 0 FROM pg_stat_activity + WHERE wait_event = 'ri-before-pk-lock']), + 'the foreign key check has read the constraint'); + +# Let the rebuild swap the constraint over and mark the old index dead, +# then fail. Its locks go away with it. +$node->safe_psql('postgres', + q[SELECT injection_points_wakeup('reindex-relation-concurrently-before-swap')] +); +$node->safe_psql('postgres', + q[SELECT injection_points_detach('reindex-relation-concurrently-before-swap')] +); + +ok( $node->poll_query_until( + 'postgres', + "SELECT NOT indislive FROM pg_index WHERE indexrelid = $before"), + 'the index the check read is dead but still there'); + +isnt( + $node->safe_psql('postgres', + q[SELECT conindid FROM pg_constraint WHERE conname = 'fk_pid_fkey']), + $before, + 'the constraint names a different index now'); + +$node->safe_psql('postgres', + q[SELECT injection_points_detach('reindex-relation-concurrently-before-drop')] +); + +# The vacuum frees the slot the deleted row held. It cleans the live +# index only, so the dead one keeps its entry for 42. +$node->safe_psql('postgres', 'VACUUM pk'); +$node->safe_psql('postgres', 'INSERT INTO pk VALUES (999)'); + +is( $node->safe_psql('postgres', q[SELECT ctid FROM pk WHERE id = 999]), + $slot, + 'the new row took the slot the deleted row held'); + +is( $node->safe_psql( + 'postgres', "SELECT count(*) FROM pk WHERE id = 42"), + '0', + 'there is no row with the key the writer is about to reference'); + +# The check has to resolve the constraint to the index it names now. The +# dead one says 42 is there; it is not. +$node->safe_psql('postgres', + q[SELECT injection_points_wakeup('ri-before-pk-lock')]); +$node->safe_psql('postgres', + q[SELECT injection_points_detach('ri-before-pk-lock')]); + +my $banner = 'done_marker'; +$writer->{stdin} .= "\\echo $banner\n\\warn $banner\n"; +pump_until($writer->{run}, $writer->{timeout}, \$writer->{stdout}, + qr/$banner/); +pump_until($writer->{run}, $writer->{timeout}, \$writer->{stderr}, + qr/$banner/); +my $err = $writer->{stderr}; +$err =~ s/$banner//g; +$err =~ s/\s+/ /g; +$err =~ s/^\s+|\s+$//g; + +$writer->quit; +$rebuild->quit; + +like( + $err, + qr/violates foreign key constraint/, + 'the write is rejected, the referenced key does not exist'); + +is( $node->safe_psql( + 'postgres', q[ + SELECT count(*) FROM fk f + WHERE NOT EXISTS (SELECT 1 FROM pk WHERE pk.id = f.pid)]), + '0', + 'no foreign key row without a referenced row'); + +$node->stop; +done_testing(); -- 2.54.0.windows.1
