From 561e06110de17c3a3d95ea137e57b10c29657f30 Mon Sep 17 00:00:00 2001 From: nkey Date: Tue, 31 Dec 2024 14:09:52 +0100 Subject: [PATCH v15 01/12] This is https://commitfest.postgresql.org/50/5160/ merged in single commit. it is required for stability of stress tests. --- src/backend/commands/indexcmds.c | 4 +- src/backend/executor/execIndexing.c | 3 + src/backend/executor/execPartition.c | 119 +++++++++++++++++++--- src/backend/executor/nodeModifyTable.c | 2 + src/backend/optimizer/util/plancat.c | 135 ++++++++++++++++++------- src/backend/utils/time/snapmgr.c | 2 + 6 files changed, 216 insertions(+), 49 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index f8d3ea820e1..47c509ceb3e 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1796,6 +1796,7 @@ DefineIndex(Oid tableId, * before the reference snap was taken, we have to wait out any * transactions that might have older snapshots. */ + INJECTION_POINT("define_index_before_set_valid"); pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_3); WaitForOlderSnapshots(limitXmin, true); @@ -4201,7 +4202,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein * the same time to make sure we only get constraint violations from the * indexes with the correct names. */ - + INJECTION_POINT("reindex_relation_concurrently_before_swap"); StartTransactionCommand(); /* @@ -4280,6 +4281,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein * index_drop() for more details. */ + INJECTION_POINT("reindex_relation_concurrently_before_set_dead"); pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_4); WaitForLockersMultiple(lockTags, AccessExclusiveLock, true); diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index 742f3f8c08d..f2a74b76465 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -117,6 +117,7 @@ #include "utils/multirangetypes.h" #include "utils/rangetypes.h" #include "utils/snapmgr.h" +#include "utils/injection_point.h" /* waitMode argument to check_exclusion_or_unique_constraint() */ typedef enum @@ -943,6 +944,8 @@ retry: econtext->ecxt_scantuple = save_scantuple; ExecDropSingleTupleTableSlot(existing_slot); + if (!conflict) + INJECTION_POINT("check_exclusion_or_unique_constraint_no_conflict"); return !conflict; } diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 432eeaf9034..44c0e8ed285 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -487,6 +487,48 @@ ExecFindPartition(ModifyTableState *mtstate, return rri; } +/* + * IsIndexCompatibleAsArbiter + * Checks if the indexes are identical in terms of being used + * as arbiters for the INSERT ON CONFLICT operation by comparing + * them to the provided arbiter index. + * + * Returns the true if indexes are compatible. + */ +static bool +IsIndexCompatibleAsArbiter(Relation arbiterIndexRelation, + IndexInfo *arbiterIndexInfo, + Relation indexRelation, + IndexInfo *indexInfo) +{ + int i; + + if (arbiterIndexInfo->ii_Unique != indexInfo->ii_Unique) + return false; + /* it is not supported for cases of exclusion constraints. */ + if (arbiterIndexInfo->ii_ExclusionOps != NULL || indexInfo->ii_ExclusionOps != NULL) + return false; + if (arbiterIndexRelation->rd_index->indnkeyatts != indexRelation->rd_index->indnkeyatts) + return false; + + for (i = 0; i < indexRelation->rd_index->indnkeyatts; i++) + { + int arbiterAttoNo = arbiterIndexRelation->rd_index->indkey.values[i]; + int attoNo = indexRelation->rd_index->indkey.values[i]; + if (arbiterAttoNo != attoNo) + return false; + } + + if (list_difference(RelationGetIndexExpressions(arbiterIndexRelation), + RelationGetIndexExpressions(indexRelation)) != NIL) + return false; + + if (list_difference(RelationGetIndexPredicate(arbiterIndexRelation), + RelationGetIndexPredicate(indexRelation)) != NIL) + return false; + return true; +} + /* * ExecInitPartitionInfo * Lock the partition and initialize ResultRelInfo. Also setup other @@ -697,6 +739,8 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, if (rootResultRelInfo->ri_onConflictArbiterIndexes != NIL) { List *childIdxs; + List *nonAncestorIdxs = NIL; + int i, j, additional_arbiters = 0; childIdxs = RelationGetIndexList(leaf_part_rri->ri_RelationDesc); @@ -707,23 +751,74 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, ListCell *lc2; ancestors = get_partition_ancestors(childIdx); - foreach(lc2, rootResultRelInfo->ri_onConflictArbiterIndexes) + if (ancestors) { - if (list_member_oid(ancestors, lfirst_oid(lc2))) - arbiterIndexes = lappend_oid(arbiterIndexes, childIdx); + foreach(lc2, rootResultRelInfo->ri_onConflictArbiterIndexes) + { + if (list_member_oid(ancestors, lfirst_oid(lc2))) + arbiterIndexes = lappend_oid(arbiterIndexes, childIdx); + } } + else /* No ancestor was found for that index. Save it for rechecking later. */ + nonAncestorIdxs = lappend_oid(nonAncestorIdxs, childIdx); list_free(ancestors); } + + /* + * If any non-ancestor indexes are found, we need to compare them with other + * indexes of the relation that will be used as arbiters. This is necessary + * when a partitioned index is processed by REINDEX CONCURRENTLY. Both indexes + * must be considered as arbiters to ensure that all concurrent transactions + * use the same set of arbiters. + */ + if (nonAncestorIdxs) + { + for (i = 0; i < leaf_part_rri->ri_NumIndices; i++) + { + if (list_member_oid(nonAncestorIdxs, leaf_part_rri->ri_IndexRelationDescs[i]->rd_index->indexrelid)) + { + Relation nonAncestorIndexRelation = leaf_part_rri->ri_IndexRelationDescs[i]; + IndexInfo *nonAncestorIndexInfo = leaf_part_rri->ri_IndexRelationInfo[i]; + Assert(!list_member_oid(arbiterIndexes, nonAncestorIndexRelation->rd_index->indexrelid)); + + /* It is too early to us non-ready indexes as arbiters */ + if (!nonAncestorIndexInfo->ii_ReadyForInserts) + continue; + + for (j = 0; j < leaf_part_rri->ri_NumIndices; j++) + { + if (list_member_oid(arbiterIndexes, + leaf_part_rri->ri_IndexRelationDescs[j]->rd_index->indexrelid)) + { + Relation arbiterIndexRelation = leaf_part_rri->ri_IndexRelationDescs[j]; + IndexInfo *arbiterIndexInfo = leaf_part_rri->ri_IndexRelationInfo[j]; + + /* If non-ancestor index are compatible to arbiter - use it as arbiter too. */ + if (IsIndexCompatibleAsArbiter(arbiterIndexRelation, arbiterIndexInfo, + nonAncestorIndexRelation, nonAncestorIndexInfo)) + { + arbiterIndexes = lappend_oid(arbiterIndexes, + nonAncestorIndexRelation->rd_index->indexrelid); + additional_arbiters++; + } + } + } + } + } + } + list_free(nonAncestorIdxs); + + /* + * If the resulting lists are of inequal length, something is wrong. + * (This shouldn't happen, since arbiter index selection should not + * pick up a non-ready index.) + * + * But we need to consider an additional arbiter indexes also. + */ + if (list_length(rootResultRelInfo->ri_onConflictArbiterIndexes) != + list_length(arbiterIndexes) - additional_arbiters) + elog(ERROR, "invalid arbiter index list"); } - - /* - * If the resulting lists are of inequal length, something is wrong. - * (This shouldn't happen, since arbiter index selection should not - * pick up an invalid index.) - */ - if (list_length(rootResultRelInfo->ri_onConflictArbiterIndexes) != - list_length(arbiterIndexes)) - elog(ERROR, "invalid arbiter index list"); leaf_part_rri->ri_onConflictArbiterIndexes = arbiterIndexes; /* diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index b0fe50075ad..d5ad73f6f69 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -69,6 +69,7 @@ #include "utils/datum.h" #include "utils/rel.h" #include "utils/snapmgr.h" +#include "utils/injection_point.h" typedef struct MTTargetRelLookup @@ -1158,6 +1159,7 @@ ExecInsert(ModifyTableContext *context, return NULL; } } + INJECTION_POINT("exec_insert_before_insert_speculative"); /* * Before we start insertion proper, acquire our "speculative diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 71abb01f655..af7586a428f 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -714,12 +714,14 @@ infer_arbiter_indexes(PlannerInfo *root) List *indexList; ListCell *l; - /* Normalized inference attributes and inference expressions: */ - Bitmapset *inferAttrs = NULL; - List *inferElems = NIL; + /* Normalized required attributes and expressions: */ + Bitmapset *requiredArbiterAttrs = NULL; + List *requiredArbiterElems = NIL; + List *requiredIndexPredExprs = (List *) onconflict->arbiterWhere; /* Results */ List *results = NIL; + bool foundValid = false; /* * Quickly return NIL for ON CONFLICT DO NOTHING without an inference @@ -754,8 +756,8 @@ infer_arbiter_indexes(PlannerInfo *root) if (!IsA(elem->expr, Var)) { - /* If not a plain Var, just shove it in inferElems for now */ - inferElems = lappend(inferElems, elem->expr); + /* If not a plain Var, just shove it in requiredArbiterElems for now */ + requiredArbiterElems = lappend(requiredArbiterElems, elem->expr); continue; } @@ -767,30 +769,76 @@ infer_arbiter_indexes(PlannerInfo *root) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("whole row unique index inference specifications are not supported"))); - inferAttrs = bms_add_member(inferAttrs, + requiredArbiterAttrs = bms_add_member(requiredArbiterAttrs, attno - FirstLowInvalidHeapAttributeNumber); } + indexList = RelationGetIndexList(relation); + /* * Lookup named constraint's index. This is not immediately returned - * because some additional sanity checks are required. + * because some additional sanity checks are required. Additionally, we + * need to process other indexes as potential arbiters to account for + * cases where REINDEX CONCURRENTLY is processing an index used as a + * named constraint. */ if (onconflict->constraint != InvalidOid) { 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"))); + errmsg("constraint in ON CONFLICT clause has no associated index"))); + } + + /* + * Find the named constraint index to extract its attributes and predicates. + * We open all indexes in the loop to avoid deadlock of changed order of locks. + * */ + foreach(l, indexList) + { + Oid indexoid = lfirst_oid(l); + Relation idxRel; + Form_pg_index idxForm; + AttrNumber natt; + + idxRel = index_open(indexoid, rte->rellockmode); + idxForm = idxRel->rd_index; + + if (idxForm->indisready) + { + if (indexOidFromConstraint == idxForm->indexrelid) + { + /* + * Prepare requirements for other indexes to be used as arbiter together + * with indexOidFromConstraint. It is required to involve both equals indexes + * in case of REINDEX CONCURRENTLY. + */ + for (natt = 0; natt < idxForm->indnkeyatts; natt++) + { + int attno = idxRel->rd_index->indkey.values[natt]; + + if (attno != 0) + requiredArbiterAttrs = bms_add_member(requiredArbiterAttrs, + attno - FirstLowInvalidHeapAttributeNumber); + } + requiredArbiterElems = RelationGetIndexExpressions(idxRel); + requiredIndexPredExprs = RelationGetIndexPredicate(idxRel); + /* We are done, so, quite the loop. */ + index_close(idxRel, NoLock); + break; + } + } + index_close(idxRel, NoLock); + } } /* * 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) { Oid indexoid = lfirst_oid(l); @@ -813,7 +861,13 @@ infer_arbiter_indexes(PlannerInfo *root) idxRel = index_open(indexoid, rte->rellockmode); idxForm = idxRel->rd_index; - if (!idxForm->indisvalid) + /* + * We need to consider both indisvalid and indisready indexes because + * them may become indisvalid before execution phase. It is required + * to keep set of indexes used as arbiter to be the same for all + * concurrent transactions. + */ + if (!idxForm->indisready) goto next; /* @@ -833,27 +887,23 @@ infer_arbiter_indexes(PlannerInfo *root) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("ON CONFLICT DO UPDATE not supported with exclusion constraints"))); - - results = lappend_oid(results, idxForm->indexrelid); - list_free(indexList); - index_close(idxRel, NoLock); - table_close(relation, NoLock); - return results; + goto found; } 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) + goto next; + } 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) + goto next; } - /* - * 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. @@ -873,7 +923,7 @@ infer_arbiter_indexes(PlannerInfo *root) } /* Non-expression attributes (if any) must match */ - if (!bms_equal(indexedAttrs, inferAttrs)) + if (!bms_equal(indexedAttrs, requiredArbiterAttrs)) goto next; /* Expression attributes (if any) must match */ @@ -881,6 +931,10 @@ infer_arbiter_indexes(PlannerInfo *root) if (idxExprs && varno != 1) ChangeVarNodes((Node *) idxExprs, 1, varno, 0); + /* + * If arbiterElems are present, check them. If name >constraint is + * present arbiterElems == NIL. + */ foreach(el, onconflict->arbiterElems) { InferenceElem *elem = (InferenceElem *) lfirst(el); @@ -918,27 +972,35 @@ infer_arbiter_indexes(PlannerInfo *root) } /* - * Now that all inference elements were matched, ensure that the + * In case of the conventional inference involved 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 the case of named constraint ensure candidate has equal set + * of expressions as the named constraint index. */ - if (list_difference(idxExprs, inferElems) != NIL) + if (list_difference(idxExprs, requiredArbiterElems) != NIL) goto next; - /* - * 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)) + /* + * 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, requiredIndexPredExprs, false)) + goto next; + /* If it's a partial index and named constraint predicates must be equal. */ + if (indexOidFromConstraint != InvalidOid && list_difference(predExprs, requiredIndexPredExprs) != NIL) goto next; +found: results = lappend_oid(results, idxForm->indexrelid); + foundValid |= idxForm->indisvalid; next: index_close(idxRel, NoLock); } @@ -946,7 +1008,8 @@ next: list_free(indexList); table_close(relation, NoLock); - if (results == NIL) + /* It is required to have at least one indisvalid index during the planning. */ + if (results == NIL || !foundValid) ereport(ERROR, (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), errmsg("there is no unique or exclusion constraint matching the ON CONFLICT specification"))); diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 8f1508b1ee2..3d018c3a1e8 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -64,6 +64,7 @@ #include "utils/resowner.h" #include "utils/snapmgr.h" #include "utils/syscache.h" +#include "utils/injection_point.h" /* @@ -388,6 +389,7 @@ InvalidateCatalogSnapshot(void) pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node); CatalogSnapshot = NULL; SnapshotResetXmin(); + INJECTION_POINT("invalidate_catalog_snapshot_end"); } } -- 2.43.0