From 67b00cae5c5c207b20cbb24fe6ccc555e2601f11 Mon Sep 17 00:00:00 2001 From: "dgrowley@gmail.com" Date: Wed, 10 Mar 2021 22:57:33 +1300 Subject: [PATCH v15 1/5] Cache PathTarget and RestrictInfo's volatility This aims to can reduce the number of times we make calls to contain_volatile_functions(). This really does not save us much with the existing set of calls to contain_volatile_functions(), however, it will save a significant number of calls in an upcoming patch which must check this during the join search. --- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/outfuncs.c | 2 ++ src/backend/optimizer/path/allpaths.c | 41 ++++++++++++----------- src/backend/optimizer/path/indxpath.c | 10 +++--- src/backend/optimizer/path/tidpath.c | 12 ++++--- src/backend/optimizer/plan/initsplan.c | 10 +++--- src/backend/optimizer/plan/planner.c | 8 ++++- src/backend/optimizer/util/orclauses.c | 11 +++--- src/backend/optimizer/util/restrictinfo.c | 1 + src/backend/optimizer/util/tlist.c | 7 ++++ src/include/nodes/pathnodes.h | 4 +++ 11 files changed, 69 insertions(+), 38 deletions(-) diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index da91cbd2b1..493a856745 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -2310,6 +2310,7 @@ _copyRestrictInfo(const RestrictInfo *from) COPY_SCALAR_FIELD(can_join); COPY_SCALAR_FIELD(pseudoconstant); COPY_SCALAR_FIELD(leakproof); + COPY_SCALAR_FIELD(has_volatile); COPY_SCALAR_FIELD(security_level); COPY_BITMAPSET_FIELD(clause_relids); COPY_BITMAPSET_FIELD(required_relids); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 6493a03ff8..73dd2255af 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -2473,6 +2473,7 @@ _outPathTarget(StringInfo str, const PathTarget *node) WRITE_FLOAT_FIELD(cost.startup, "%.2f"); WRITE_FLOAT_FIELD(cost.per_tuple, "%.2f"); WRITE_INT_FIELD(width); + WRITE_BOOL_FIELD(has_volatile_expr); } static void @@ -2497,6 +2498,7 @@ _outRestrictInfo(StringInfo str, const RestrictInfo *node) WRITE_BOOL_FIELD(can_join); WRITE_BOOL_FIELD(pseudoconstant); WRITE_BOOL_FIELD(leakproof); + WRITE_BOOL_FIELD(has_volatile); WRITE_UINT_FIELD(security_level); WRITE_BITMAPSET_FIELD(clause_relids); WRITE_BITMAPSET_FIELD(required_relids); diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index d73ac562eb..5ac993042e 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -134,7 +134,8 @@ static void check_output_expressions(Query *subquery, static void compare_tlist_datatypes(List *tlist, List *colTypes, pushdown_safety_info *safetyInfo); static bool targetIsInAllPartitionLists(TargetEntry *tle, Query *query); -static bool qual_is_pushdown_safe(Query *subquery, Index rti, Node *qual, +static bool qual_is_pushdown_safe(Query *subquery, Index rti, + RestrictInfo *rinfo, pushdown_safety_info *safetyInfo); static void subquery_push_qual(Query *subquery, RangeTblEntry *rte, Index rti, Node *qual); @@ -2177,11 +2178,12 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel, foreach(l, rel->baserestrictinfo) { RestrictInfo *rinfo = (RestrictInfo *) lfirst(l); - Node *clause = (Node *) rinfo->clause; if (!rinfo->pseudoconstant && - qual_is_pushdown_safe(subquery, rti, clause, &safetyInfo)) + qual_is_pushdown_safe(subquery, rti, rinfo, &safetyInfo)) { + Node *clause = (Node *)rinfo->clause; + /* Push it down */ subquery_push_qual(subquery, rte, rti, clause); } @@ -3390,37 +3392,39 @@ targetIsInAllPartitionLists(TargetEntry *tle, Query *query) } /* - * qual_is_pushdown_safe - is a particular qual safe to push down? + * qual_is_pushdown_safe - is a particular rinfo safe to push down? * - * qual is a restriction clause applying to the given subquery (whose RTE + * rinfo is a restriction clause applying to the given subquery (whose RTE * has index rti in the parent query). * * Conditions checked here: * - * 1. The qual must not contain any SubPlans (mainly because I'm not sure - * it will work correctly: SubLinks will already have been transformed into - * SubPlans in the qual, but not in the subquery). Note that SubLinks that - * transform to initplans are safe, and will be accepted here because what - * we'll see in the qual is just a Param referencing the initplan output. + * 1. rinfo's clause must not contain any SubPlans (mainly because it's + * unclear that it will work correctly: SubLinks will already have been + * transformed into SubPlans in the qual, but not in the subquery). Note that + * SubLinks that transform to initplans are safe, and will be accepted here + * because what we'll see in the qual is just a Param referencing the initplan + * output. * - * 2. If unsafeVolatile is set, the qual must not contain any volatile + * 2. If unsafeVolatile is set, rinfo's clause must not contain any volatile * functions. * - * 3. If unsafeLeaky is set, the qual must not contain any leaky functions - * that are passed Var nodes, and therefore might reveal values from the - * subquery as side effects. + * 3. If unsafeLeaky is set, rinfo's clause must not contain any leaky + * functions that are passed Var nodes, and therefore might reveal values from + * the subquery as side effects. * - * 4. The qual must not refer to the whole-row output of the subquery + * 4. rinfo's clause must not refer to the whole-row output of the subquery * (since there is no easy way to name that within the subquery itself). * - * 5. The qual must not refer to any subquery output columns that were + * 5. rinfo's clause must not refer to any subquery output columns that were * found to be unsafe to reference by subquery_is_pushdown_safe(). */ static bool -qual_is_pushdown_safe(Query *subquery, Index rti, Node *qual, +qual_is_pushdown_safe(Query *subquery, Index rti, RestrictInfo *rinfo, pushdown_safety_info *safetyInfo) { bool safe = true; + Node *qual = (Node *) rinfo->clause; List *vars; ListCell *vl; @@ -3429,8 +3433,7 @@ qual_is_pushdown_safe(Query *subquery, Index rti, Node *qual, return false; /* Refuse volatile quals if we found they'd be unsafe (point 2) */ - if (safetyInfo->unsafeVolatile && - contain_volatile_functions(qual)) + if (safetyInfo->unsafeVolatile && rinfo->has_volatile) return false; /* Refuse leaky quals if told to (point 3) */ diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c index ff536e6b24..8c447cf0a2 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -2502,7 +2502,7 @@ match_opclause_to_indexcol(PlannerInfo *root, */ if (match_index_to_operand(leftop, indexcol, index) && !bms_is_member(index_relid, rinfo->right_relids) && - !contain_volatile_functions(rightop)) + (!rinfo->has_volatile || !contain_volatile_functions(rightop))) { if (IndexCollMatchesExprColl(idxcollation, expr_coll) && op_in_opfamily(expr_op, opfamily)) @@ -2531,7 +2531,7 @@ match_opclause_to_indexcol(PlannerInfo *root, if (match_index_to_operand(rightop, indexcol, index) && !bms_is_member(index_relid, rinfo->left_relids) && - !contain_volatile_functions(leftop)) + (!rinfo->has_volatile || !contain_volatile_functions(leftop))) { if (IndexCollMatchesExprColl(idxcollation, expr_coll)) { @@ -2723,7 +2723,7 @@ match_saopclause_to_indexcol(PlannerInfo *root, */ if (match_index_to_operand(leftop, indexcol, index) && !bms_is_member(index_relid, right_relids) && - !contain_volatile_functions(rightop)) + (!rinfo->has_volatile || !contain_volatile_functions(rightop))) { if (IndexCollMatchesExprColl(idxcollation, expr_coll) && op_in_opfamily(expr_op, opfamily)) @@ -2805,14 +2805,14 @@ match_rowcompare_to_indexcol(PlannerInfo *root, */ if (match_index_to_operand(leftop, indexcol, index) && !bms_is_member(index_relid, pull_varnos(root, rightop)) && - !contain_volatile_functions(rightop)) + (!rinfo->has_volatile || !contain_volatile_functions(rightop))) { /* OK, indexkey is on left */ var_on_left = true; } else if (match_index_to_operand(rightop, indexcol, index) && !bms_is_member(index_relid, pull_varnos(root, leftop)) && - !contain_volatile_functions(leftop)) + (!rinfo->has_volatile || !contain_volatile_functions(leftop))) { /* indexkey is on right, so commute the operator */ expr_op = get_commutator(expr_op); diff --git a/src/backend/optimizer/path/tidpath.c b/src/backend/optimizer/path/tidpath.c index 0725d950c5..e40df11b19 100644 --- a/src/backend/optimizer/path/tidpath.c +++ b/src/backend/optimizer/path/tidpath.c @@ -84,6 +84,9 @@ IsBinaryTidClause(RestrictInfo *rinfo, RelOptInfo *rel) /* Must be an OpExpr */ if (!is_opclause(rinfo->clause)) return false; + /* Must not contain any volatile functions */ + if (rinfo->has_volatile) + return false; node = (OpExpr *) rinfo->clause; /* OpExpr must have two arguments */ @@ -111,8 +114,7 @@ IsBinaryTidClause(RestrictInfo *rinfo, RelOptInfo *rel) return false; /* The other argument must be a pseudoconstant */ - if (bms_is_member(rel->relid, other_relids) || - contain_volatile_functions(other)) + if (bms_is_member(rel->relid, other_relids)) return false; return true; /* success */ @@ -178,6 +180,9 @@ IsTidEqualAnyClause(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel) /* Must be a ScalarArrayOpExpr */ if (!(rinfo->clause && IsA(rinfo->clause, ScalarArrayOpExpr))) return false; + /* We can safely reject if it's marked as volatile */ + if (rinfo->has_volatile) + return false; node = (ScalarArrayOpExpr *) rinfo->clause; /* Operator must be tideq */ @@ -194,8 +199,7 @@ IsTidEqualAnyClause(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel) IsCTIDVar((Var *) arg1, rel)) { /* The other argument must be a pseudoconstant */ - if (bms_is_member(rel->relid, pull_varnos(root, arg2)) || - contain_volatile_functions(arg2)) + if (bms_is_member(rel->relid, pull_varnos(root, arg2))) return false; return true; /* success */ diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 02f813cebd..9914d230ed 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -2652,12 +2652,13 @@ check_mergejoinable(RestrictInfo *restrictinfo) return; if (list_length(((OpExpr *) clause)->args) != 2) return; + if (restrictinfo->has_volatile) + return; opno = ((OpExpr *) clause)->opno; leftarg = linitial(((OpExpr *) clause)->args); - if (op_mergejoinable(opno, exprType(leftarg)) && - !contain_volatile_functions((Node *) clause)) + if (op_mergejoinable(opno, exprType(leftarg))) restrictinfo->mergeopfamilies = get_mergejoin_opfamilies(opno); /* @@ -2689,11 +2690,12 @@ check_hashjoinable(RestrictInfo *restrictinfo) return; if (list_length(((OpExpr *) clause)->args) != 2) return; + if (restrictinfo->has_volatile) + return; opno = ((OpExpr *) clause)->opno; leftarg = linitial(((OpExpr *) clause)->args); - if (op_hashjoinable(opno, exprType(leftarg)) && - !contain_volatile_functions((Node *) clause)) + if (op_hashjoinable(opno, exprType(leftarg))) restrictinfo->hashjoinoperator = opno; } diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 424d25cbd5..20adb77ccc 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -5903,7 +5903,13 @@ make_sort_input_target(PlannerInfo *root, col_is_srf[i] = true; have_srf = true; } - else if (contain_volatile_functions((Node *) expr)) + + /* + * We need only check if expr is volatile if the final_target has + * any volatile functions. + */ + else if (final_target->has_volatile_expr && + contain_volatile_functions((Node *) expr)) { /* Unconditionally postpone */ postpone_col[i] = true; diff --git a/src/backend/optimizer/util/orclauses.c b/src/backend/optimizer/util/orclauses.c index d559f33826..d9f6c44079 100644 --- a/src/backend/optimizer/util/orclauses.c +++ b/src/backend/optimizer/util/orclauses.c @@ -133,17 +133,18 @@ is_safe_restriction_clause_for(RestrictInfo *rinfo, RelOptInfo *rel) { /* * We want clauses that mention the rel, and only the rel. So in - * particular pseudoconstant clauses can be rejected quickly. Then check - * the clause's Var membership. + * particular pseudoconstant clauses can be rejected quickly. Also, + * checking volatility is cheap too, so do these before checking the + * clause's Var membership. */ if (rinfo->pseudoconstant) return false; + /* We don't want extra evaluations of any volatile functions */ + if (rinfo->has_volatile) + return false; if (!bms_equal(rinfo->clause_relids, rel->relids)) return false; - /* We don't want extra evaluations of any volatile functions */ - if (contain_volatile_functions((Node *) rinfo->clause)) - return false; return true; } diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c index eb113d94c1..f1d068c2fe 100644 --- a/src/backend/optimizer/util/restrictinfo.c +++ b/src/backend/optimizer/util/restrictinfo.c @@ -137,6 +137,7 @@ make_restrictinfo_internal(PlannerInfo *root, else restrictinfo->leakproof = false; /* really, "don't know" */ + restrictinfo->has_volatile = contain_volatile_functions((Node *) clause); /* * If it's a binary opclause, set up left/right relids info. In any case * set up the total clause relids info. diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c index 89853a0630..9cf9d45347 100644 --- a/src/backend/optimizer/util/tlist.c +++ b/src/backend/optimizer/util/tlist.c @@ -623,6 +623,9 @@ make_pathtarget_from_tlist(List *tlist) i++; } + /* cache whether the tlist has any volatile functions */ + target->has_volatile_expr = contain_volatile_functions((Node *) tlist); + return target; } @@ -724,6 +727,10 @@ add_column_to_pathtarget(PathTarget *target, Expr *expr, Index sortgroupref) target->sortgrouprefs = (Index *) palloc0(nexprs * sizeof(Index)); target->sortgrouprefs[nexprs - 1] = sortgroupref; } + + /* Check for new volatile functions, unless we already have one */ + if (!target->has_volatile_expr) + target->has_volatile_expr = contain_volatile_functions((Node *) expr); } /* diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 86405a274e..4526ae4297 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -1087,6 +1087,8 @@ typedef struct PathTarget Index *sortgrouprefs; /* corresponding sort/group refnos, or 0 */ QualCost cost; /* cost of evaluating the expressions */ int width; /* estimated avg width of result tuples */ + bool has_volatile_expr; /* True if any of 'exprs' has a volatile + * function. */ } PathTarget; /* Convenience macro to get a sort/group refno from a PathTarget */ @@ -2017,6 +2019,8 @@ typedef struct RestrictInfo bool leakproof; /* true if known to contain no leaked Vars */ + bool has_volatile; /* true if clause contains a volatile func */ + Index security_level; /* see comment above */ /* The set of relids (varnos) actually referenced in the clause: */ -- 2.27.0