From 0a0b3b5170dfbade51021d9c146a9586bbe75a37 Mon Sep 17 00:00:00 2001 From: "tender.wang" Date: Thu, 29 Feb 2024 15:01:38 +0800 Subject: [PATCH v2] Fix parallel index build failed. We will check whether it is safe to build btree index parallelly. But the index expression or predicate which we check is modified not raw expression or predicate. For example, function in expression may be optimized to const. This optimization will miss function PARALLEL SAFE option, which will make planner choose to build index parallelly. If function includes EXCEPTION, that will report error. Function containing an EXCEPTION clause effectively forms a subtransaction. Now we cannot start subtransactions during a parallel operation For whether to parallely building index, we need the raw index expression. The RelationGetIndexExpressions() actually returns optimized expression not raw expression. We choose to add a bool argument to RelationGetIndexExpressions() not to write a new function which just return index raw expression. --- src/backend/access/spgist/spgutils.c | 2 +- src/backend/catalog/index.c | 4 ++-- src/backend/commands/matview.c | 2 +- src/backend/commands/tablecmds.c | 4 ++-- src/backend/executor/execIndexing.c | 2 +- src/backend/optimizer/plan/planner.c | 9 +++++++-- src/backend/optimizer/util/plancat.c | 8 ++++---- src/backend/parser/parse_utilcmd.c | 4 ++-- src/backend/utils/cache/relcache.c | 12 ++++++++++-- src/include/utils/relcache.h | 4 ++-- src/test/regress/expected/btree_index.out | 17 +++++++++++++++++ src/test/regress/sql/btree_index.sql | 21 +++++++++++++++++++++ 12 files changed, 70 insertions(+), 19 deletions(-) diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c index 5b5e6e82d3..7acdc3a416 100644 --- a/src/backend/access/spgist/spgutils.c +++ b/src/backend/access/spgist/spgutils.c @@ -137,7 +137,7 @@ GetIndexInputType(Relation index, AttrNumber indexcol) if (index->rd_indexprs) indexprs = index->rd_indexprs; else - indexprs = RelationGetIndexExpressions(index); + indexprs = RelationGetIndexExpressions(index, true); indexpr_item = list_head(indexprs); for (int i = 1; i <= index->rd_index->indnkeyatts; i++) { diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 4b88a9cb87..d49b86a7c8 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2455,8 +2455,8 @@ BuildIndexInfo(Relation index) ii = makeIndexInfo(indexStruct->indnatts, indexStruct->indnkeyatts, index->rd_rel->relam, - RelationGetIndexExpressions(index), - RelationGetIndexPredicate(index), + RelationGetIndexExpressions(index, true), + RelationGetIndexPredicate(index, true), indexStruct->indisunique, indexStruct->indnullsnotdistinct, indexStruct->indisready, diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c index 59920ced83..847f34b021 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -898,7 +898,7 @@ is_usable_unique_index(Relation indexRel) indexStruct->indimmediate && indexRel->rd_rel->relam == BTREE_AM_OID && indexStruct->indisvalid && - RelationGetIndexPredicate(indexRel) == NIL && + RelationGetIndexPredicate(indexRel, true) == NIL && indexStruct->indnatts > 0) { /* diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f798794556..8db9790315 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -17172,13 +17172,13 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode errmsg("cannot use non-immediate index \"%s\" as replica identity", RelationGetRelationName(indexRel)))); /* Expression indexes aren't supported. */ - if (RelationGetIndexExpressions(indexRel) != NIL) + if (RelationGetIndexExpressions(indexRel, true) != NIL) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot use expression index \"%s\" as replica identity", RelationGetRelationName(indexRel)))); /* Predicate indexes aren't supported. */ - if (RelationGetIndexPredicate(indexRel) != NIL) + if (RelationGetIndexPredicate(indexRel, true) != NIL) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot use partial index \"%s\" as replica identity", diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index 9f05b3654c..ad6e5ab23a 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -1045,7 +1045,7 @@ index_unchanged_by_update(ResultRelInfo *resultRelInfo, EState *estate, * If we find any matching Vars, don't pass hint for index. Otherwise * pass hint. */ - idxExprs = RelationGetIndexExpressions(indexRelation); + idxExprs = RelationGetIndexExpressions(indexRelation, true); hasexpression = index_expression_changed_walker((Node *) idxExprs, allUpdatedCols); list_free(idxExprs); diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index be4e182869..3951d1924e 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -6674,10 +6674,15 @@ plan_create_index_workers(Oid tableOid, Oid indexOid) * Currently, parallel workers can't access the leader's temporary tables. * Furthermore, any index predicate or index expressions must be parallel * safe. + * + * We would check the raw index expression not optimized expression. + * ReationGetIndexExpression() will optimize the index expression even though + * user provide PARALLEL UNSAFE label to the expression. + * It will report error in some cases. So we pass FALSE to function. */ if (heap->rd_rel->relpersistence == RELPERSISTENCE_TEMP || - !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index)) || - !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index))) + !is_parallel_safe(root, (Node *) RelationGetIndexExpressions(index, false)) || + !is_parallel_safe(root, (Node *) RelationGetIndexPredicate(index, false))) { parallel_workers = 0; goto done; diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index b933eefa64..0a503269e1 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -434,8 +434,8 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent, * correct varno for the parent relation, so that they match up * correctly against qual clauses. */ - info->indexprs = RelationGetIndexExpressions(indexRelation); - info->indpred = RelationGetIndexPredicate(indexRelation); + info->indexprs = RelationGetIndexExpressions(indexRelation, true); + info->indpred = RelationGetIndexPredicate(indexRelation, true); if (info->indexprs && varno != 1) ChangeVarNodes((Node *) info->indexprs, 1, varno, 0); if (info->indpred && varno != 1) @@ -847,7 +847,7 @@ infer_arbiter_indexes(PlannerInfo *root) goto next; /* Expression attributes (if any) must match */ - idxExprs = RelationGetIndexExpressions(idxRel); + idxExprs = RelationGetIndexExpressions(idxRel, true); foreach(el, onconflict->arbiterElems) { InferenceElem *elem = (InferenceElem *) lfirst(el); @@ -898,7 +898,7 @@ infer_arbiter_indexes(PlannerInfo *root) * If it's a partial index, its predicate must be implied by the ON * CONFLICT's WHERE clause. */ - predExprs = RelationGetIndexPredicate(idxRel); + predExprs = RelationGetIndexPredicate(idxRel, true); if (!predicate_implied_by(predExprs, (List *) onconflict->arbiterWhere, false)) goto next; diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index c7efd8d8ce..07dab10a4f 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -2422,14 +2422,14 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) errdetail("Cannot create a primary key or unique constraint using such an index."), parser_errposition(cxt->pstate, constraint->location))); - if (RelationGetIndexExpressions(index_rel) != NIL) + if (RelationGetIndexExpressions(index_rel, true) != NIL) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("index \"%s\" contains expressions", index_name), errdetail("Cannot create a primary key or unique constraint using such an index."), parser_errposition(cxt->pstate, constraint->location))); - if (RelationGetIndexPredicate(index_rel) != NIL) + if (RelationGetIndexPredicate(index_rel, true) != NIL) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("\"%s\" is a partial index", index_name), diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 50acae4529..002c8b25f2 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -5008,7 +5008,7 @@ RelationGetReplicaIndex(Relation relation) * disappear due to relcache invalidation.) */ List * -RelationGetIndexExpressions(Relation relation) +RelationGetIndexExpressions(Relation relation, bool get_raw_expr) { List *result; Datum exprsDatum; @@ -5039,6 +5039,10 @@ RelationGetIndexExpressions(Relation relation) result = (List *) stringToNode(exprsString); pfree(exprsString); + /* Return, if we need raw index expression */ + if (!get_raw_expr) + return result; + /* * Run the expressions through eval_const_expressions. This is not just an * optimization, but is necessary, because the planner will be comparing @@ -5121,7 +5125,7 @@ RelationGetDummyIndexExpressions(Relation relation) * disappear due to relcache invalidation.) */ List * -RelationGetIndexPredicate(Relation relation) +RelationGetIndexPredicate(Relation relation, bool get_raw_expr) { List *result; Datum predDatum; @@ -5152,6 +5156,10 @@ RelationGetIndexPredicate(Relation relation) result = (List *) stringToNode(predString); pfree(predString); + /* Return, if we need a raw index predicate */ + if (!get_raw_expr) + return result; + /* * Run the expression through const-simplification and canonicalization. * This is not just an optimization, but is necessary, because the planner diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h index 18c32ea700..f1c7b0d2e5 100644 --- a/src/include/utils/relcache.h +++ b/src/include/utils/relcache.h @@ -48,9 +48,9 @@ extern List *RelationGetIndexList(Relation relation); extern List *RelationGetStatExtList(Relation relation); extern Oid RelationGetPrimaryKeyIndex(Relation relation); extern Oid RelationGetReplicaIndex(Relation relation); -extern List *RelationGetIndexExpressions(Relation relation); +extern List *RelationGetIndexExpressions(Relation relation, bool get_raw_expr); extern List *RelationGetDummyIndexExpressions(Relation relation); -extern List *RelationGetIndexPredicate(Relation relation); +extern List *RelationGetIndexPredicate(Relation relation, bool get_raw_expr); extern bytea **RelationGetIndexAttOptions(Relation relation, bool copy); /* diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out index 8311a03c3d..29a2903d80 100644 --- a/src/test/regress/expected/btree_index.out +++ b/src/test/regress/expected/btree_index.out @@ -434,3 +434,20 @@ ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100); ERROR: ALTER action ALTER COLUMN ... SET cannot be performed on relation "btree_part_idx" DETAIL: This operation is not supported for partitioned indexes. DROP TABLE btree_part; +-- Test index expression includes parallel unsafe function +CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE +AS $$ +BEGIN + RETURN 0; +EXCEPTION WHEN OTHERS THEN + RETURN 1; +END$$ LANGUAGE plpgsql; +CREATE TABLE btree_para_bld(i int); +ALTER TABLE btree_para_bld SET (parallel_workers = 4); +SET max_parallel_maintenance_workers TO 4; +CREATE INDEX ON btree_para_bld((i + para_unsafe_f())); +-- Test index predicate includes parallel unsafe function +CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f(); +RESET max_parallel_maintenance_workers; +DROP TABLE btree_para_bld; +DROP FUNCTION para_unsafe_f; diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql index ef84354234..a054d106fb 100644 --- a/src/test/regress/sql/btree_index.sql +++ b/src/test/regress/sql/btree_index.sql @@ -267,3 +267,24 @@ CREATE TABLE btree_part (id int4) PARTITION BY RANGE (id); CREATE INDEX btree_part_idx ON btree_part(id); ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100); DROP TABLE btree_part; + +-- Test index expression includes parallel unsafe function +CREATE FUNCTION para_unsafe_f() RETURNS int IMMUTABLE PARALLEL UNSAFE +AS $$ +BEGIN + RETURN 0; +EXCEPTION WHEN OTHERS THEN + RETURN 1; +END$$ LANGUAGE plpgsql; + +CREATE TABLE btree_para_bld(i int); +ALTER TABLE btree_para_bld SET (parallel_workers = 4); +SET max_parallel_maintenance_workers TO 4; +CREATE INDEX ON btree_para_bld((i + para_unsafe_f())); + +-- Test index predicate includes parallel unsafe function +CREATE INDEX ON btree_para_bld(i) WHERE i > para_unsafe_f(); + +RESET max_parallel_maintenance_workers; +DROP TABLE btree_para_bld; +DROP FUNCTION para_unsafe_f; -- 2.25.1