From 6b7626dc756125bb2792668185fb1bba01090aea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= Date: Fri, 28 Nov 2025 18:09:40 +0100 Subject: [PATCH v14] ON CONFLICT: Consider indexes matching constraint index during REINDEX CONCURRENTLY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ensures that all transactions doing INSERT ON CONFLICT consider the same set of indexes during the reindex operation, avoiding spurious errors about duplicate insertions. Author: Mihail Nikalayeu Reviewed-by: Álvaro Herrera Discussion: https://postgr.es/m/CANtu0ojXmqjmEzp-=aJSxjsdE76iAsRgHBoK0QtYHimb_mEfsg@mail.gmail.com --- src/backend/optimizer/util/plancat.c | 192 ++++++++++---- src/backend/parser/parse_clause.c | 16 +- src/test/modules/injection_points/Makefile | 1 + ...ndex-concurrently-upsert-on-constraint.out | 238 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 1 + ...dex-concurrently-upsert-on-constraint.spec | 110 ++++++++ 6 files changed, 506 insertions(+), 52 deletions(-) create mode 100644 src/test/modules/injection_points/expected/reindex-concurrently-upsert-on-constraint.out create mode 100644 src/test/modules/injection_points/specs/reindex-concurrently-upsert-on-constraint.spec diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 7af9a2064e3..daf75612333 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -806,9 +806,15 @@ infer_arbiter_indexes(PlannerInfo *root) Relation relation; Oid indexOidFromConstraint = InvalidOid; List *indexList; - ListCell *l; + List *indexRelList = NIL; - /* Normalized inference attributes and inference expressions: */ + /* + * Required attributes and expressions used to match indexes to the clause + * given by the user. In the case where ON CONFLICT ON CONSTRAINT was + * given, we need to compute these things to match other indexes, to + * account for the case where the index is under REINDEX CONCURRENTLY. + */ + List *inferIndexExprs = (List *) onconflict->arbiterWhere; Bitmapset *inferAttrs = NULL; List *inferElems = NIL; @@ -841,15 +847,19 @@ infer_arbiter_indexes(PlannerInfo *root) * well as a separate list of expression items. This simplifies matching * the cataloged definition of indexes. */ - foreach(l, onconflict->arbiterElems) + foreach_ptr(InferenceElem, elem, onconflict->arbiterElems) { - InferenceElem *elem = (InferenceElem *) lfirst(l); Var *var; int attno; + /* we cannot also have a constraint name, per grammar */ + Assert(!OidIsValid(onconflict->constraint)); + if (!IsA(elem->expr, Var)) { - /* If not a plain Var, just shove it in inferElems for now */ + /* + * If not a plain Var, just shove it in inferElems for now. + */ inferElems = lappend(inferElems, elem->expr); continue; } @@ -867,45 +877,100 @@ infer_arbiter_indexes(PlannerInfo *root) } /* - * Lookup named constraint's index. This is not immediately returned - * because some additional sanity checks are required. + * Next, open all the indexes. We need this list for two things: first, + * if an ON CONSTRAINT clause was given, and that constraint's index is + * undergoing REINDEX CONCURRENTLY, then we need to consider all matches + * for that index. Second, if an attribute list was specified in the ON + * CONFLICT clause, we use the list to find the indexes whose attributes + * match that list. + */ + indexList = RelationGetIndexList(relation); + foreach_oid(indexoid, indexList) + { + Relation idxRel; + + /* + * Must open in this order to avoid deadlock. Obtain the same lock + * type that the executor will ultimately use. + */ + idxRel = index_open(indexoid, rte->rellockmode); + indexRelList = lappend(indexRelList, idxRel); + } + + /* + * If a constraint was named in the command, look up its index. We don't + * return it immediately because we need some additional sanity checks, + * and also because we need to include other indexes as arbiters to + * account for REINDEX CONCURRENTLY processing the constraint's index. */ if (onconflict->constraint != InvalidOid) { - indexOidFromConstraint = get_constraint_index(onconflict->constraint); + /* we cannot also have an explicit list of elements, per grammar */ + Assert(onconflict->arbiterElems == NIL); + indexOidFromConstraint = get_constraint_index(onconflict->constraint); if (indexOidFromConstraint == InvalidOid) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("constraint in ON CONFLICT clause has no associated index"))); + + /* + * Find the named constraint index to extract its attributes and + * predicates. + */ + foreach_ptr(RelationData, idxRel, indexRelList) + { + Form_pg_index idxForm = idxRel->rd_index; + + if (idxForm->indisready) + { + if (indexOidFromConstraint == idxForm->indexrelid) + { + /* + * Set up inferElems and inferPredExprs to match + * the constraint index, so that we can match them + * in the loop below. + */ + for (int natt = 0; natt < idxForm->indnkeyatts; natt++) + { + int attno; + + attno = idxRel->rd_index->indkey.values[natt]; + if (attno != InvalidAttrNumber) + inferAttrs = + bms_add_member(inferAttrs, + attno - FirstLowInvalidHeapAttributeNumber); + } + + /* found it */ + inferElems = RelationGetIndexExpressions(idxRel); + inferIndexExprs = RelationGetIndexPredicate(idxRel); + break; + } + } + } } /* * Using that representation, iterate through the list of indexes on the * target relation to try and find a match */ - indexList = RelationGetIndexList(relation); - - foreach(l, indexList) + foreach_ptr(RelationData, idxRel, indexRelList) { - Oid indexoid = lfirst_oid(l); - Relation idxRel; Form_pg_index idxForm; Bitmapset *indexedAttrs; List *idxExprs; List *predExprs; AttrNumber natt; - ListCell *el; + bool match; /* - * Extract info from the relation descriptor for the index. Obtain - * the same lock type that the executor will ultimately use. + * Extract info from the relation descriptor for the index. * * Let executor complain about !indimmediate case directly, because * enforcement needs to occur there anyway when an inference clause is * omitted. */ - idxRel = index_open(indexoid, rte->rellockmode); idxForm = idxRel->rd_index; /* @@ -924,7 +989,7 @@ infer_arbiter_indexes(PlannerInfo *root) * indexes at least one index that is marked valid. */ if (!idxForm->indisready) - goto next; + continue; /* * Note that we do not perform a check against indcheckxmin (like e.g. @@ -934,7 +999,7 @@ infer_arbiter_indexes(PlannerInfo *root) */ /* - * Look for match on "ON constraint_name" variant, which may not be + * Look for match on "ON constraint_name" variant, which may not be a * unique constraint. This can only be a constraint name. */ if (indexOidFromConstraint == idxForm->indexrelid) @@ -944,31 +1009,37 @@ infer_arbiter_indexes(PlannerInfo *root) (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("ON CONFLICT DO UPDATE not supported with exclusion constraints"))); + /* Consider this one a match already */ results = lappend_oid(results, idxForm->indexrelid); foundValid |= idxForm->indisvalid; - index_close(idxRel, NoLock); - break; + continue; } else if (indexOidFromConstraint != InvalidOid) { - /* No point in further work for index in named constraint case */ - goto next; + /* + * In the case of "ON constraint_name DO UPDATE" we need to skip + * non-unique candidates. + */ + if (!idxForm->indisunique && onconflict->action == ONCONFLICT_UPDATE) + continue; + } + else + { + /* + * Only considering conventional inference at this point (not + * named constraints), so index under consideration can be + * immediately skipped if it's not unique. + */ + if (!idxForm->indisunique) + continue; } - - /* - * Only considering conventional inference at this point (not named - * constraints), so index under consideration can be immediately - * skipped if it's not unique - */ - if (!idxForm->indisunique) - goto next; /* * So-called unique constraints with WITHOUT OVERLAPS are really * exclusion constraints, so skip those too. */ if (idxForm->indisexclusion) - goto next; + continue; /* Build BMS representation of plain (non expression) index attrs */ indexedAttrs = NULL; @@ -983,17 +1054,20 @@ infer_arbiter_indexes(PlannerInfo *root) /* Non-expression attributes (if any) must match */ if (!bms_equal(indexedAttrs, inferAttrs)) - goto next; + continue; /* Expression attributes (if any) must match */ idxExprs = RelationGetIndexExpressions(idxRel); if (idxExprs && varno != 1) ChangeVarNodes((Node *) idxExprs, 1, varno, 0); - foreach(el, onconflict->arbiterElems) + /* + * If arbiterElems are present, check them. (Note that if a + * constraint name was given in the command line, this list is NIL.) + */ + match = true; + foreach_ptr(InferenceElem, elem, onconflict->arbiterElems) { - InferenceElem *elem = (InferenceElem *) lfirst(el); - /* * Ensure that collation/opclass aspects of inference expression * element match. Even though this loop is primarily concerned @@ -1002,7 +1076,10 @@ infer_arbiter_indexes(PlannerInfo *root) * attributes appearing as inference elements. */ if (!infer_collation_opclass_match(elem, idxRel, idxExprs)) - goto next; + { + match = false; + break; + } /* * Plain Vars don't factor into count of expression elements, and @@ -1023,37 +1100,58 @@ infer_arbiter_indexes(PlannerInfo *root) list_member(idxExprs, elem->expr)) continue; - goto next; + match = false; + break; } + if (!match) + continue; /* - * Now that all inference elements were matched, ensure that the + * In case of inference from an attribute list, ensure that the * expression elements from inference clause are not missing any * cataloged expressions. This does the right thing when unique * indexes redundantly repeat the same attribute, or if attributes * redundantly appear multiple times within an inference clause. + * + * In case a constraint was named, ensure the candidate has an equal + * set of expressions as the named constraint's index. */ if (list_difference(idxExprs, inferElems) != NIL) - goto next; + continue; - /* - * If it's a partial index, its predicate must be implied by the ON - * CONFLICT's WHERE clause. - */ predExprs = RelationGetIndexPredicate(idxRel); if (predExprs && varno != 1) ChangeVarNodes((Node *) predExprs, 1, varno, 0); - if (!predicate_implied_by(predExprs, (List *) onconflict->arbiterWhere, false)) - goto next; + /* + * If it's a partial index and conventional inference, its predicate + * must be implied by the ON CONFLICT's WHERE clause. + */ + if (indexOidFromConstraint == InvalidOid && + !predicate_implied_by(predExprs, inferIndexExprs, false)) + continue; + /* + * If it's a partial index and named constraint predicates must be + * equal. + */ + if (indexOidFromConstraint != InvalidOid && + list_difference(predExprs, inferIndexExprs) != NIL) + continue; + + /* Consider this a match */ results = lappend_oid(results, idxForm->indexrelid); foundValid |= idxForm->indisvalid; -next: + } + + /* Close all indexes */ + foreach_ptr(RelationData, idxRel, indexRelList) + { index_close(idxRel, NoLock); } list_free(indexList); + list_free(indexRelList); table_close(relation, NoLock); /* We require at least one indisvalid index */ diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index ca26f6f61f2..bee9860c513 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -3277,11 +3277,11 @@ resolve_unique_index_expr(ParseState *pstate, InferClause *infer, * Raw grammar re-uses CREATE INDEX infrastructure for unique index * inference clause, and so will accept opclasses by name and so on. * - * Make no attempt to match ASC or DESC ordering or NULLS FIRST/NULLS - * LAST ordering, since those are not significant for inference - * purposes (any unique index matching the inference specification in - * other regards is accepted indifferently). Actively reject this as - * wrong-headed. + * Make no attempt to match ASC or DESC ordering, NULLS FIRST/NULLS + * LAST ordering or opclass options, since those are not significant + * for inference purposes (any unique index matching the inference + * specification in other regards is accepted indifferently). Actively + * reject this as wrong-headed. */ if (ielem->ordering != SORTBY_DEFAULT) ereport(ERROR, @@ -3295,6 +3295,12 @@ resolve_unique_index_expr(ParseState *pstate, InferClause *infer, errmsg("NULLS FIRST/LAST is not allowed in ON CONFLICT clause"), parser_errposition(pstate, exprLocation((Node *) infer)))); + if (ielem->opclassopts) + ereport(ERROR, + errcode(ERRCODE_INVALID_COLUMN_REFERENCE), + errmsg("operator class options are not allowed in ON CONFLICT clause"), + parser_errposition(pstate, + exprLocation((Node *) infer))); if (!ielem->expr) { diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 7b3c0c4b716..0a9716db27c 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -19,6 +19,7 @@ ISOLATION = basic \ syscache-update-pruned \ index-concurrently-upsert \ reindex-concurrently-upsert \ + reindex-concurrently-upsert-on-constraint \ index-concurrently-upsert-predicate TAP_TESTS = 1 diff --git a/src/test/modules/injection_points/expected/reindex-concurrently-upsert-on-constraint.out b/src/test/modules/injection_points/expected/reindex-concurrently-upsert-on-constraint.out new file mode 100644 index 00000000000..c1ac1f77c61 --- /dev/null +++ b/src/test/modules/injection_points/expected/reindex-concurrently-upsert-on-constraint.out @@ -0,0 +1,238 @@ +Parsed test spec with 4 sessions + +starting permutation: s3_setup_wait_before_set_dead s3_start_reindex s1_start_upsert s4_wakeup_to_set_dead s2_start_upsert s4_wakeup_s1 s4_wakeup_s2 +injection_points_attach +----------------------- + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +injection_points_set_local +-------------------------- + +(1 row) + +step s3_setup_wait_before_set_dead: + SELECT injection_points_attach('reindex-relation-concurrently-before-set-dead', 'wait'); + +injection_points_attach +----------------------- + +(1 row) + +step s3_start_reindex: + REINDEX INDEX CONCURRENTLY test.tbl_pkey; + +step s1_start_upsert: + INSERT INTO test.tbl VALUES (13, now()) ON CONFLICT ON CONSTRAINT tbl_pkey DO UPDATE SET updated_at = now(); + +step s4_wakeup_to_set_dead: + SELECT injection_points_detach('reindex-relation-concurrently-before-set-dead'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-set-dead'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step s2_start_upsert: + INSERT INTO test.tbl VALUES (13, now()) ON CONFLICT ON CONSTRAINT tbl_pkey DO UPDATE SET updated_at = now(); + +step s4_wakeup_s1: + SELECT injection_points_detach('check-exclusion-or-unique-constraint-no-conflict'); + SELECT injection_points_wakeup('check-exclusion-or-unique-constraint-no-conflict'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step s1_start_upsert: <... completed> +step s4_wakeup_s2: + SELECT injection_points_detach('exec-insert-before-insert-speculative'); + SELECT injection_points_wakeup('exec-insert-before-insert-speculative'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step s2_start_upsert: <... completed> +step s3_start_reindex: <... completed> + +starting permutation: s3_setup_wait_before_swap s3_start_reindex s1_start_upsert s4_wakeup_to_swap s2_start_upsert s4_wakeup_s2 s4_wakeup_s1 +injection_points_attach +----------------------- + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +injection_points_set_local +-------------------------- + +(1 row) + +step s3_setup_wait_before_swap: + SELECT injection_points_attach('reindex-relation-concurrently-before-swap', 'wait'); + +injection_points_attach +----------------------- + +(1 row) + +step s3_start_reindex: + REINDEX INDEX CONCURRENTLY test.tbl_pkey; + +step s1_start_upsert: + INSERT INTO test.tbl VALUES (13, now()) ON CONFLICT ON CONSTRAINT tbl_pkey DO UPDATE SET updated_at = now(); + +step s4_wakeup_to_swap: + 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 s2_start_upsert: + INSERT INTO test.tbl VALUES (13, now()) ON CONFLICT ON CONSTRAINT tbl_pkey DO UPDATE SET updated_at = now(); + +step s4_wakeup_s2: + SELECT injection_points_detach('exec-insert-before-insert-speculative'); + SELECT injection_points_wakeup('exec-insert-before-insert-speculative'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step s4_wakeup_s1: + SELECT injection_points_detach('check-exclusion-or-unique-constraint-no-conflict'); + SELECT injection_points_wakeup('check-exclusion-or-unique-constraint-no-conflict'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step s1_start_upsert: <... completed> +step s2_start_upsert: <... completed> +step s3_start_reindex: <... completed> + +starting permutation: s3_setup_wait_before_set_dead s3_start_reindex s1_start_upsert s2_start_upsert s4_wakeup_s1 s4_wakeup_to_set_dead s4_wakeup_s2 +injection_points_attach +----------------------- + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +injection_points_set_local +-------------------------- + +(1 row) + +step s3_setup_wait_before_set_dead: + SELECT injection_points_attach('reindex-relation-concurrently-before-set-dead', 'wait'); + +injection_points_attach +----------------------- + +(1 row) + +step s3_start_reindex: + REINDEX INDEX CONCURRENTLY test.tbl_pkey; + +step s1_start_upsert: + INSERT INTO test.tbl VALUES (13, now()) ON CONFLICT ON CONSTRAINT tbl_pkey DO UPDATE SET updated_at = now(); + +step s2_start_upsert: + INSERT INTO test.tbl VALUES (13, now()) ON CONFLICT ON CONSTRAINT tbl_pkey DO UPDATE SET updated_at = now(); + +step s4_wakeup_s1: + SELECT injection_points_detach('check-exclusion-or-unique-constraint-no-conflict'); + SELECT injection_points_wakeup('check-exclusion-or-unique-constraint-no-conflict'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step s1_start_upsert: <... completed> +step s4_wakeup_to_set_dead: + SELECT injection_points_detach('reindex-relation-concurrently-before-set-dead'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-set-dead'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step s4_wakeup_s2: + SELECT injection_points_detach('exec-insert-before-insert-speculative'); + SELECT injection_points_wakeup('exec-insert-before-insert-speculative'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step s2_start_upsert: <... completed> +step s3_start_reindex: <... completed> diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 485b483e3ca..0706cd3d6e9 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -51,6 +51,7 @@ tests += { 'index-concurrently-upsert', 'reindex-concurrently-upsert', 'index-concurrently-upsert-predicate', + 'reindex-concurrently-upsert-on-constraint', ], 'runningcheck': false, # see syscache-update-pruned # Some tests wait for all snapshots, so avoid parallel execution diff --git a/src/test/modules/injection_points/specs/reindex-concurrently-upsert-on-constraint.spec b/src/test/modules/injection_points/specs/reindex-concurrently-upsert-on-constraint.spec new file mode 100644 index 00000000000..8126256460c --- /dev/null +++ b/src/test/modules/injection_points/specs/reindex-concurrently-upsert-on-constraint.spec @@ -0,0 +1,110 @@ +# Test race conditions involving: +# +# - s1: UPSERT a tuple +# - s2: UPSERT the same tuple +# - s3: concurrently REINDEX the primary key +# +# - s4: operations with injection points + +setup +{ + CREATE EXTENSION injection_points; + CREATE SCHEMA test; + CREATE UNLOGGED TABLE test.tbl(i int primary key, updated_at timestamp); + ALTER TABLE test.tbl SET (parallel_workers=0); +} + +teardown +{ + DROP SCHEMA test CASCADE; + DROP EXTENSION injection_points; +} + +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('check-exclusion-or-unique-constraint-no-conflict', 'wait'); +} +step s1_start_upsert +{ + INSERT INTO test.tbl VALUES (13, now()) ON CONFLICT ON CONSTRAINT tbl_pkey DO UPDATE SET updated_at = now(); +} + +session s2 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('exec-insert-before-insert-speculative', 'wait'); +} +step s2_start_upsert +{ + INSERT INTO test.tbl VALUES (13, now()) ON CONFLICT ON CONSTRAINT tbl_pkey DO UPDATE SET updated_at = now(); +} + +session s3 +setup +{ + SELECT injection_points_set_local(); +} +step s3_setup_wait_before_set_dead +{ + SELECT injection_points_attach('reindex-relation-concurrently-before-set-dead', 'wait'); +} +step s3_setup_wait_before_swap +{ + SELECT injection_points_attach('reindex-relation-concurrently-before-swap', 'wait'); +} +step s3_start_reindex +{ + REINDEX INDEX CONCURRENTLY test.tbl_pkey; +} + +session s4 +step s4_wakeup_to_swap +{ + SELECT injection_points_detach('reindex-relation-concurrently-before-swap'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-swap'); +} +step s4_wakeup_s1 +{ + SELECT injection_points_detach('check-exclusion-or-unique-constraint-no-conflict'); + SELECT injection_points_wakeup('check-exclusion-or-unique-constraint-no-conflict'); +} +step s4_wakeup_s2 +{ + SELECT injection_points_detach('exec-insert-before-insert-speculative'); + SELECT injection_points_wakeup('exec-insert-before-insert-speculative'); +} +step s4_wakeup_to_set_dead +{ + SELECT injection_points_detach('reindex-relation-concurrently-before-set-dead'); + SELECT injection_points_wakeup('reindex-relation-concurrently-before-set-dead'); +} + +permutation + s3_setup_wait_before_set_dead + s3_start_reindex(s1_start_upsert, s2_start_upsert) + s1_start_upsert + s4_wakeup_to_set_dead + s2_start_upsert(s1_start_upsert) + s4_wakeup_s1 + s4_wakeup_s2 + +permutation + s3_setup_wait_before_swap + s3_start_reindex(s1_start_upsert, s2_start_upsert) + s1_start_upsert + s4_wakeup_to_swap + s2_start_upsert(s1_start_upsert) + s4_wakeup_s2 + s4_wakeup_s1 + +permutation + s3_setup_wait_before_set_dead + s3_start_reindex(s1_start_upsert, s2_start_upsert) + s1_start_upsert + s2_start_upsert(s1_start_upsert) + s4_wakeup_s1 + s4_wakeup_to_set_dead + s4_wakeup_s2 -- 2.47.3