From 5e11c3978c898a37b0bee8b52051b64468adb0b8 Mon Sep 17 00:00:00 2001 From: Mikhail Nikalayeu Date: Wed, 5 Aug 2026 01:12:14 +0200 Subject: [PATCH v1] Re-read the FK constraint after locking the referenced table The RI fast path resolves a foreign key check to a physical index: it reads pg_constraint.conindid, takes RowShareLock on the referenced table, and opens that index by OID. conindid is read before the lock, which is not safe. REINDEX CONCURRENTLY holds only ShareUpdateExclusiveLock on the referenced table, so it does not conflict, and the waits that make it safe -- WaitForLockersMultiple() before marking the old index dead and again before dropping it -- cover only backends that already hold a lock on that table. A backend that has read the constraint but has not yet taken RowShareLock is invisible to both. The RI constraint cache does not help here: InvalidateConstraintCacheCallBack() only clears the valid flag, so a pointer obtained earlier keeps serving the stale conindid. Two outcomes follow. If the swap, the set-dead and the drop all complete within the window, index_open() fails with "could not open relation with OID". If only the set-dead completes, the index is still there to open but has indislive = false, so it no longer receives new rows and the check can miss a referenced row that does exist, reporting a foreign key violation that is not one. The SPI path was never exposed to this: its query names no index, so the planner picks one under the lock. Fix by re-reading the constraint through ri_LoadConstraintInfo() once the referenced table is locked, at both fast path call sites -- the batched one in ri_FastPathGetEntry() and the per-row one in ri_FastPathCheck(). That is enough because REINDEX CONCURRENTLY commits the swap that repoints conindid before it waits for lockers, and marks the old index dead and later drops it only after such a wait. So under the lock we either observe the new conindid, or we observe an old index whose state cannot change until our transaction ends. ri_LoadConstraintInfo() refills the cache entry in place, so a caller still holding the same pointer sees the refreshed value too. conindid is the only part of RI_ConstraintInfo exposed this way; the rest cannot change without a lock that conflicts with RowShareLock. Add an isolation test with three permutations, covering both call sites and both outcomes. The dead index case needs a new injection point, "reindex-relation-concurrently-before-drop", to hold REINDEX CONCURRENTLY between phases 5 and 6, which is the window where the old index is dead but not yet dropped. Co-Authored-By: Claude Opus 5 (1M context) --- src/backend/commands/indexcmds.c | 1 + src/backend/utils/adt/ri_triggers.c | 19 ++ src/test/modules/injection_points/Makefile | 1 + .../expected/ri_fastpath_reindex.out | 244 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/ri_fastpath_reindex.spec | 114 ++++++++ 6 files changed, 380 insertions(+) create mode 100644 src/test/modules/injection_points/expected/ri_fastpath_reindex.out create mode 100644 src/test/modules/injection_points/specs/ri_fastpath_reindex.spec diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 3790b8e1252..97f2d2a60a2 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -4523,6 +4523,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein * Drop the old indexes. */ + 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/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 627a9fb38ea..5759b1d8839 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,7 +2824,13 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo, CommandCounterIncrement(); snapshot = RegisterSnapshot(GetTransactionSnapshot()); + 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); @@ -4385,7 +4392,19 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) * We don't release these locks until end of transaction, matching SPI * behavior. */ + + INJECTION_POINT("ri-before-pk-lock", NULL); + entry->pk_rel = table_open(riinfo->pk_relid, RowShareLock); + + /* + * conindid may have been read before we took that lock, and REINDEX + * CONCURRENTLY moves a constraint to a new index. Re-read it now: + * under the lock we either see the new index, or an old one that + * cannot be marked dead or dropped until this transaction ends. + */ + riinfo = ri_LoadConstraintInfo(riinfo->constraint_id); + entry->idx_rel = index_open(riinfo->conindid, AccessShareLock); entry->pk_slot = table_slot_create(entry->pk_rel, NULL); diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 25a3ddd890d..3b136adf126 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -19,6 +19,7 @@ ISOLATION = basic \ repack_temporal \ repack_temporal_multirange \ repack_toast \ + ri_fastpath_reindex \ syscache-update-pruned \ wait_cleanup \ heap_lock_update diff --git a/src/test/modules/injection_points/expected/ri_fastpath_reindex.out b/src/test/modules/injection_points/expected/ri_fastpath_reindex.out new file mode 100644 index 00000000000..dedc1f96d21 --- /dev/null +++ b/src/test/modules/injection_points/expected/ri_fastpath_reindex.out @@ -0,0 +1,244 @@ +Parsed test spec with 3 sessions + +starting permutation: reindex upd wake_reindex reindexed swapped wake_check checked rows orphan +injection_points_attach +----------------------- + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step reindex: REINDEX INDEX CONCURRENTLY ri_pk_pkey; +step upd: UPDATE ri_fk SET pid = 42 WHERE id = 1; +step wake_reindex: + SELECT injection_points_detach('reindex-relation-concurrently-before-swap'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-swap'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step reindex: <... completed> +step reindexed: +step swapped: + SELECT count(*) = 0 AS old_index_dropped + FROM pg_class WHERE oid = (SELECT conindid FROM ri_old_index); + SELECT conindid <> (SELECT conindid FROM ri_old_index) AS constraint_moved + FROM pg_constraint WHERE conname = 'ri_fk_pid_fkey'; + +old_index_dropped +----------------- +t +(1 row) + +constraint_moved +---------------- +t +(1 row) + +step wake_check: + SELECT injection_points_detach('ri-before-pk-lock'); + SELECT injection_points_wakeup('ri-before-pk-lock'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step upd: <... completed> +step checked: +step rows: SELECT id, pid FROM ri_fk WHERE id IN (1, 2) ORDER BY id; +id|pid +--+--- + 1| 42 + 2| 2 +(2 rows) + +step orphan: INSERT INTO ri_fk VALUES (999, 12345); +ERROR: insert or update on table "ri_fk" violates foreign key constraint "ri_fk_pid_fkey" + +starting permutation: reindex sub sub_upd wake_reindex reindexed swapped wake_check checked sub_commit rows orphan +injection_points_attach +----------------------- + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step reindex: REINDEX INDEX CONCURRENTLY ri_pk_pkey; +step sub: BEGIN; SAVEPOINT s; +step sub_upd: UPDATE ri_fk SET pid = 43 WHERE id = 2; +step wake_reindex: + SELECT injection_points_detach('reindex-relation-concurrently-before-swap'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-swap'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step reindex: <... completed> +step reindexed: +step swapped: + SELECT count(*) = 0 AS old_index_dropped + FROM pg_class WHERE oid = (SELECT conindid FROM ri_old_index); + SELECT conindid <> (SELECT conindid FROM ri_old_index) AS constraint_moved + FROM pg_constraint WHERE conname = 'ri_fk_pid_fkey'; + +old_index_dropped +----------------- +t +(1 row) + +constraint_moved +---------------- +t +(1 row) + +step wake_check: + SELECT injection_points_detach('ri-before-pk-lock'); + SELECT injection_points_wakeup('ri-before-pk-lock'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step sub_upd: <... completed> +step checked: +step sub_commit: COMMIT; +step rows: SELECT id, pid FROM ri_fk WHERE id IN (1, 2) ORDER BY id; +id|pid +--+--- + 1| 1 + 2| 43 +(2 rows) + +step orphan: INSERT INTO ri_fk VALUES (999, 12345); +ERROR: insert or update on table "ri_fk" violates foreign key constraint "ri_fk_pid_fkey" + +starting permutation: park_drop reindex upd_new wake_reindex await_drop dead wake_check checked wake_drop reindexed rows orphan +injection_points_attach +----------------------- + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step park_drop: + SELECT injection_points_attach('reindex-relation-concurrently-before-drop', 'wait'); + +injection_points_attach +----------------------- + +(1 row) + +step reindex: REINDEX INDEX CONCURRENTLY ri_pk_pkey; +step upd_new: UPDATE ri_fk SET pid = 500 WHERE id = 1; +step wake_reindex: + SELECT injection_points_detach('reindex-relation-concurrently-before-swap'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-swap'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step await_drop: + DO $$ + BEGIN + LOOP + PERFORM 1 FROM pg_stat_activity + WHERE wait_event = 'reindex-relation-concurrently-before-drop'; + EXIT WHEN FOUND; + PERFORM pg_sleep(.1); + END LOOP; + END + $$; + +step dead: + SELECT indisvalid, indisready, indislive + FROM pg_index WHERE indexrelid = (SELECT conindid FROM ri_old_index); + INSERT INTO ri_pk VALUES (500); + +indisvalid|indisready|indislive +----------+----------+--------- +f |f |f +(1 row) + +step wake_check: + SELECT injection_points_detach('ri-before-pk-lock'); + SELECT injection_points_wakeup('ri-before-pk-lock'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step upd_new: <... completed> +step checked: +step wake_drop: + SELECT injection_points_detach('reindex-relation-concurrently-before-drop'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-drop'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step reindex: <... completed> +step reindexed: +step rows: SELECT id, pid FROM ri_fk WHERE id IN (1, 2) ORDER BY id; +id|pid +--+--- + 1|500 + 2| 2 +(2 rows) + +step orphan: INSERT INTO ri_fk VALUES (999, 12345); +ERROR: insert or update on table "ri_fk" violates foreign key constraint "ri_fk_pid_fkey" diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index aaf0536ba7e..aff516b901a 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -50,6 +50,7 @@ tests += { 'repack_temporal', 'repack_temporal_multirange', 'repack_toast', + 'ri_fastpath_reindex', 'syscache-update-pruned', 'wait_cleanup', 'heap_lock_update', diff --git a/src/test/modules/injection_points/specs/ri_fastpath_reindex.spec b/src/test/modules/injection_points/specs/ri_fastpath_reindex.spec new file mode 100644 index 00000000000..f64b36053b6 --- /dev/null +++ b/src/test/modules/injection_points/specs/ri_fastpath_reindex.spec @@ -0,0 +1,114 @@ +# A foreign key check racing a rebuild of the index it resolves through. +# +# The RI fast path reads conindid before it locks the referenced table, so a +# concurrent REINDEX CONCURRENTLY can repoint the constraint and drop that +# index in between. Both fast path call sites are covered, and both outcomes: +# an index that is already gone, and one that is only marked dead and so no +# longer receives new rows. + +setup +{ + CREATE EXTENSION injection_points; + CREATE TABLE ri_pk (id int PRIMARY KEY); + INSERT INTO ri_pk SELECT g FROM generate_series(1, 100) g; + CREATE TABLE ri_fk (id int PRIMARY KEY, pid int REFERENCES ri_pk(id)); + INSERT INTO ri_fk SELECT g, g FROM generate_series(1, 100) g; + CREATE TABLE ri_old_index AS + SELECT conindid FROM pg_constraint WHERE conname = 'ri_fk_pid_fkey'; +} + +teardown +{ + DROP TABLE ri_fk, ri_pk, ri_old_index; + DROP EXTENSION injection_points; +} + +# The rebuild, stopped just before it repoints the constraint. +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('reindex-relation-concurrently-before-swap', 'wait'); +} +# Stops it again, after the old index is dead and before it is dropped. +step park_drop +{ + SELECT injection_points_attach('reindex-relation-concurrently-before-drop', 'wait'); +} +step reindex { REINDEX INDEX CONCURRENTLY ri_pk_pkey; } +# Forces the rebuild to have finished before anything else runs. +step reindexed { } + +# The writer, stopped after it read conindid and before it locks ri_pk. +session s2 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('ri-before-pk-lock', 'wait'); +} +step upd { UPDATE ri_fk SET pid = 42 WHERE id = 1; } +# 500 is added while the check is parked, so only a maintained index has it. +step upd_new { UPDATE ri_fk SET pid = 500 WHERE id = 1; } +step sub { BEGIN; SAVEPOINT s; } +step sub_upd { UPDATE ri_fk SET pid = 43 WHERE id = 2; } +step sub_commit { COMMIT; } +# Forces the check to have finished before anything else runs. +step checked { } + +session s3 +step wake_reindex +{ + SELECT injection_points_detach('reindex-relation-concurrently-before-swap'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-swap'); +} +step swapped +{ + SELECT count(*) = 0 AS old_index_dropped + FROM pg_class WHERE oid = (SELECT conindid FROM ri_old_index); + SELECT conindid <> (SELECT conindid FROM ri_old_index) AS constraint_moved + FROM pg_constraint WHERE conname = 'ri_fk_pid_fkey'; +} +step wake_check +{ + SELECT injection_points_detach('ri-before-pk-lock'); + SELECT injection_points_wakeup('ri-before-pk-lock'); +} +# Waking a session does not mean it has moved on: it can still be reported as +# waiting on the point it was woken from. Wait for the name of the next point +# instead, which only appears once the old index is dead. The empty step trick +# used above does not work here, since the rebuild parks again rather than +# finishing. +step await_drop +{ + DO $$ + BEGIN + LOOP + PERFORM 1 FROM pg_stat_activity + WHERE wait_event = 'reindex-relation-concurrently-before-drop'; + EXIT WHEN FOUND; + PERFORM pg_sleep(.1); + END LOOP; + END + $$; +} +step dead +{ + SELECT indisvalid, indisready, indislive + FROM pg_index WHERE indexrelid = (SELECT conindid FROM ri_old_index); + INSERT INTO ri_pk VALUES (500); +} +step wake_drop +{ + SELECT injection_points_detach('reindex-relation-concurrently-before-drop'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-drop'); +} +step rows { SELECT id, pid FROM ri_fk WHERE id IN (1, 2) ORDER BY id; } +# The constraint must still be enforced, not merely not crashing. +step orphan { INSERT INTO ri_fk VALUES (999, 12345); } + +# Batched call site. +permutation reindex upd wake_reindex reindexed swapped wake_check checked rows orphan +# Per-row call site: a subtransaction turns batching off. +permutation reindex sub sub_upd wake_reindex reindexed swapped wake_check checked sub_commit rows orphan +# Dead index rather than dropped one. +permutation park_drop reindex upd_new wake_reindex await_drop dead wake_check checked wake_drop reindexed rows orphan -- 2.43.0