On Sun, Jul 26, 2026 at 06:12:27PM +0900, Michael Paquier wrote: > The addition of the new INDEX_ option would be at least backpatchable. > I was wondering why you needed the extra flag to begin with, but I > don't immediately see how we could enforce indimmediate properly in > the definition copy without it. Having three flags related to > deferred constraints feels a bit annoying long-term, but that really > comes down to the grammar representation, what we allow in the table > definition based on the standard, and how much space we give the > backend internals to deal with the CREATE TABLE grammar for > constraints in an equivalent way. > > Side note: your patch has some useless whitespaces in index.h. I'll > study all that a bit more next week, booking a few hours.
I have looked at the patch, and here are some comments/fixes: - IS_DEFERRABLE_COPY feels confusing, and it is inconsistent with the other option names. I have settled to a simpler CREATE_DEFERRABLE, meaning indimmediate=false for the copy when the option is set. - Let's avoid a hardcoded sleep in the isolation test. With 10s, I suspect that this is going to be unstable in the buildfarm. 0.1s should not be noticeable on a fast machine, but let's just make that faster and reliable with an injection point. We just need one after the phase 2, waiting while we perform some writes that break the constraint (result would be to not break the constraint, of course, due to the indimmediate). - We can do the same test in less sessions and less steps. I was able to get down to 2 sessions. Without the fix, we report immediately a diff with the constraint breaking. Attached is my result, that I intend to backpatch. Test is for v17~. No time to edit the commit message yet, sorry, I get that it is confusing with the new flag name. :p Thanks, -- Michael
From b81fd25429315a7a58a021effc1d1d7a6975f65c Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 27 Jul 2026 16:45:51 +0900 Subject: [PATCH v3] Fix propagation of indimmediate flag in index_create_copy index_create_copy is used to create copy definitions of existing indexes. Currently, it passes 0 as constr_flags to index_create, which results in the copied index being created as immediate (indimmediate = true). For deferrable unique constraints, this means the transient index used during REINDEX CONCURRENTLY forces immediate constraint checks on concurrent inserts, causing duplicate key violations. To fix this without violating the contract of constr_flags (which should only be used when creating constraints) and without relaxing the strict assertion in index_create, we introduce a new index creation flag: INDEX_CREATE_IS_DEFERRABLE_COPY. During index_create_copy, if the old index is deferrable, we pass INDEX_CREATE_IS_DEFERRABLE_COPY in flags to index_create. In index_create, we check this flag when determining the indimmediate status of the new index, marking it as deferrable (indimmediate = false) if either INDEX_CONSTR_CREATE_DEFERRABLE is in constr_flags or INDEX_CREATE_IS_DEFERRABLE_COPY is in flags. We also add an isolation test case to verify that REINDEX CONCURRENTLY does not break concurrent transactions utilizing deferred uniqueness. --- src/include/catalog/index.h | 1 + src/backend/catalog/index.c | 16 +++++- src/backend/commands/indexcmds.c | 2 + src/test/modules/injection_points/Makefile | 1 + .../reindex_concurrently_deferred.out | 41 +++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/reindex_concurrently_deferred.spec | 50 +++++++++++++++++++ 7 files changed, 110 insertions(+), 2 deletions(-) create mode 100644 src/test/modules/injection_points/expected/reindex_concurrently_deferred.out create mode 100644 src/test/modules/injection_points/specs/reindex_concurrently_deferred.spec diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index 9aee82263478..b952ad071d31 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -72,6 +72,7 @@ extern void index_check_primary_key(Relation heapRel, #define INDEX_CREATE_PARTITIONED (1 << 5) #define INDEX_CREATE_INVALID (1 << 6) #define INDEX_CREATE_SUPPRESS_PROGRESS (1 << 7) +#define INDEX_CREATE_DEFERRABLE (1 << 8) extern Oid index_create(Relation heapRelation, const char *indexRelationName, diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 31ef84d0a166..7c1b94407a9b 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -717,6 +717,9 @@ UpdateIndexRelation(Oid indexoid, * create a partitioned index (table must be partitioned) * INDEX_CREATE_SUPPRESS_PROGRESS: * don't report progress during the index build. + * INDEX_CREATE_DEFERRABLE: + * index supports a deferrable constraint, mark it as + * non-immediate (indimmediate = false). * * constr_flags: flags passed to index_constraint_create * (only if INDEX_CREATE_ADD_CONSTRAINT is set) @@ -1051,7 +1054,8 @@ index_create(Relation heapRelation, indexInfo, collationIds, opclassIds, coloptions, isprimary, is_exclusion, - (constr_flags & INDEX_CONSTR_CREATE_DEFERRABLE) == 0, + (constr_flags & INDEX_CONSTR_CREATE_DEFERRABLE) == 0 && + (flags & INDEX_CREATE_DEFERRABLE) == 0, !concurrent && !invalid, !concurrent); @@ -1324,6 +1328,7 @@ index_create_copy(Relation heapRelation, uint16 flags, List *indexColNames = NIL; List *indexExprs = NIL; List *indexPreds = NIL; + Form_pg_index indexForm; indexRelation = index_open(oldIndexId, RowExclusiveLock); @@ -1343,6 +1348,13 @@ index_create_copy(Relation heapRelation, uint16 flags, indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(oldIndexId)); if (!HeapTupleIsValid(indexTuple)) elog(ERROR, "cache lookup failed for index %u", oldIndexId); + + indexForm = (Form_pg_index) GETSTRUCT(indexTuple); + + /* Old index is deferrable, do the same for the new index */ + if (!indexForm->indimmediate) + flags |= INDEX_CREATE_DEFERRABLE; + indclassDatum = SysCacheGetAttrNotNull(INDEXRELID, indexTuple, Anum_pg_index_indclass); indclass = (oidvector *) DatumGetPointer(indclassDatum); @@ -1477,7 +1489,7 @@ index_create_copy(Relation heapRelation, uint16 flags, stattargets, reloptionsDatum, flags, - 0, + 0, /* constr_flags */ true, /* allow table to be a system catalog? */ false, /* is_internal? */ NULL); diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 713bb5d10f19..a2936c58402d 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -4283,6 +4283,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein CommitTransactionCommand(); } + INJECTION_POINT("reindex-conc-index-built", NULL); + StartTransactionCommand(); /* diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index fac80f3a4a73..25a3ddd890d1 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -14,6 +14,7 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress ISOLATION = basic \ inplace \ + reindex_concurrently_deferred \ repack \ repack_temporal \ repack_temporal_multirange \ diff --git a/src/test/modules/injection_points/expected/reindex_concurrently_deferred.out b/src/test/modules/injection_points/expected/reindex_concurrently_deferred.out new file mode 100644 index 000000000000..39924fa24fe3 --- /dev/null +++ b/src/test/modules/injection_points/expected/reindex_concurrently_deferred.out @@ -0,0 +1,41 @@ +Parsed test spec with 2 sessions + +starting permutation: reindex check_catalog begin2 write2 write_dup resolve_dup commit2 wakeup noop1 +injection_points_attach +----------------------- + +(1 row) + +step reindex: REINDEX TABLE CONCURRENTLY reind_deferred; <waiting ...> +step check_catalog: + SELECT c.relname, i.indisunique, i.indimmediate, i.indisready, i.indisvalid + FROM pg_class c + JOIN pg_index i ON i.indexrelid = c.oid + WHERE c.relname = 'uq_val_ccnew'; + +relname |indisunique|indimmediate|indisready|indisvalid +------------+-----------+------------+----------+---------- +uq_val_ccnew|t |f |t |f +(1 row) + +step begin2: BEGIN; +step write2: INSERT INTO reind_deferred VALUES (3, 9); +step write_dup: INSERT INTO reind_deferred VALUES (4, 9); +step resolve_dup: UPDATE reind_deferred SET val = 10 WHERE id = 4; +step commit2: COMMIT; +step wakeup: + SELECT injection_points_detach('reindex-conc-index-built'); + SELECT injection_points_wakeup('reindex-conc-index-built'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step reindex: <... completed> +step noop1: diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 163b6374ebcd..aaf0536ba7e9 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -45,6 +45,7 @@ tests += { 'specs': [ 'basic', 'inplace', + 'reindex_concurrently_deferred', 'repack', 'repack_temporal', 'repack_temporal_multirange', diff --git a/src/test/modules/injection_points/specs/reindex_concurrently_deferred.spec b/src/test/modules/injection_points/specs/reindex_concurrently_deferred.spec new file mode 100644 index 000000000000..4b95e1da2a71 --- /dev/null +++ b/src/test/modules/injection_points/specs/reindex_concurrently_deferred.spec @@ -0,0 +1,50 @@ +# REINDEX CONCURRENTLY with DEFERRED constraints +# +# Verify that concurrent writes that temporarily violate a deferred unique +# constraint do not fail while REINDEX CONCURRENTLY is running. +# +# The injection point "reindex-conc-index-built" fires after the phase 2 +# of REINDEX CONCURRENTLY, when the new index has indisready = true (inserts +# are checked against it) but indisvalid = false. + +setup +{ + CREATE EXTENSION injection_points; + CREATE TABLE reind_deferred (id int, val int, + CONSTRAINT uq_val UNIQUE(val) DEFERRABLE INITIALLY DEFERRED); + INSERT INTO reind_deferred VALUES (1, 1), (2, 2); +} + +teardown +{ + DROP TABLE reind_deferred; + DROP EXTENSION injection_points; +} + +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('reindex-conc-index-built', 'wait'); +} +step reindex { REINDEX TABLE CONCURRENTLY reind_deferred; } +step noop1 { } + +session s2 +step check_catalog { + SELECT c.relname, i.indisunique, i.indimmediate, i.indisready, i.indisvalid + FROM pg_class c + JOIN pg_index i ON i.indexrelid = c.oid + WHERE c.relname = 'uq_val_ccnew'; +} +step begin2 { BEGIN; } +step write2 { INSERT INTO reind_deferred VALUES (3, 9); } +step write_dup { INSERT INTO reind_deferred VALUES (4, 9); } +step resolve_dup { UPDATE reind_deferred SET val = 10 WHERE id = 4; } +step commit2 { COMMIT; } +step wakeup { + SELECT injection_points_detach('reindex-conc-index-built'); + SELECT injection_points_wakeup('reindex-conc-index-built'); +} + +permutation reindex check_catalog begin2 write2 write_dup resolve_dup commit2 wakeup noop1 -- 2.55.0
signature.asc
Description: PGP signature
