From 7c97dbb4a9b7493559d8f3d09a5943a77dd2abae Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Mon, 17 Aug 2026 21:30:44 +0900 Subject: [PATCH v2] Fix RI fast-path race with REINDEX CONCURRENTLY The RI fast path reads pg_constraint.conindid before taking RowShareLock on the referenced table. REINDEX CONCURRENTLY can repoint the constraint and mark the old index dead, or drop it, between those operations. A backend in that window does not yet hold a relation lock, so it is not covered by REINDEX CONCURRENTLY's waits for lockers. Opening an index that has already been dropped produces "could not open relation with OID". Opening one that has only been marked dead can produce wrong answers: the index is no longer maintained or vacuumed, so a scan can miss a referenced row or follow a stale entry to a reused heap line pointer. After locking the referenced table, reload the constraint and use its current conindid. LockRelationOid() processes invalidation messages after acquiring the lock, so the reload sees a committed index swap. If the lock was already held, REINDEX CONCURRENTLY cannot mark the old index dead or drop it until the transaction releases that lock, so continuing to use the old conindid is safe. Do this at both RI fast-path call sites. Add injection-point coverage for old indexes that have either been dropped or marked dead. Disclosure: the original patch was developed with Claude Opus 5. Author: Mihail Nikalayeu Discussion: https://postgr.es/m/CADzfLwUJiVuv69uwuF5z4TrMhNkVwQUXW03q+uVNwmYFLtjEhw@mail.gmail.com Backpatch-through: 19 --- src/backend/commands/indexcmds.c | 1 + src/backend/utils/adt/ri_triggers.c | 20 ++ src/test/modules/injection_points/Makefile | 1 + .../expected/ri_fastpath_reindex.out | 171 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/ri_fastpath_reindex.spec | 108 +++++++++++ 6 files changed, 302 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 5d62fa7605b..5a0312fe772 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -4539,6 +4539,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 6f201ef0f31..f83df4d543a 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -49,6 +49,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" @@ -2824,7 +2825,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); @@ -4426,7 +4433,20 @@ 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: + * LockRelationOid() processes invalidation messages after acquiring the + * lock, so 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..275b9a6d085 --- /dev/null +++ b/src/test/modules/injection_points/expected/ri_fastpath_reindex.out @@ -0,0 +1,171 @@ +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: 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..3c2b8dc1ccb --- /dev/null +++ b/src/test/modules/injection_points/specs/ri_fastpath_reindex.spec @@ -0,0 +1,108 @@ +# 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. Cover 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; } +# 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 +# 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.47.3