From 2bd62b7ac4353d8bd308a0b23885676ba61e8b68 Mon Sep 17 00:00:00 2001 From: nkey 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) --- 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