From e10b5f94a1b33e713a9676589d5a1d15b99403d5 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 29 Jan 2025 20:46:20 -0500 Subject: [PATCH v1 5/8] Convert nested assignments to use Params instead of CaseTestExprs. The work in clauses.c is a little trickier than the previous steps, because we can't just use find_casetestexpr to find the CaseTestExpr to be replaced. However, execExpr.c already had logic that knew where to look for that, so we can just move that to clauses.c. Also, there's a bit in ruleutils.c that I concluded could just be dropped. The code following that can deal with the situation just as well, I think. This is the last core-backend usage of CaseTestExpr, but we still have to fix plpgsql ... --- src/backend/executor/execExpr.c | 134 ++++------------ src/backend/nodes/nodeFuncs.c | 1 + src/backend/optimizer/util/clauses.c | 220 ++++++++++++++++++++++++++- src/backend/parser/parse_target.c | 4 +- src/backend/rewrite/rewriteHandler.c | 2 + src/backend/utils/adt/ruleutils.c | 16 -- src/include/nodes/primnodes.h | 17 +++ 7 files changed, 269 insertions(+), 125 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 80709f14de..6d2b9179b2 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -87,7 +87,6 @@ static void ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *sbsref, ExprState *state, Datum *resv, bool *resnull); -static bool isAssignmentIndirectionExpr(Expr *expr); static void ExecInitCoerceToDomain(ExprEvalStep *scratch, CoerceToDomain *ctest, ExprState *state, Datum *resv, bool *resnull); @@ -1519,7 +1518,8 @@ ExecInitExprRec(Expr *node, ExprState *state, bool *nulls; int ncolumns; ListCell *l1, - *l2; + *l2, + *l3; /* find out the number of columns in the composite type */ tupDesc = lookup_rowtype_tupdesc(fstore->resulttype, -1); @@ -1547,52 +1547,43 @@ ExecInitExprRec(Expr *node, ExprState *state, ExprEvalPushStep(state, &scratch); /* evaluate new field values, store in workspace columns */ - forboth(l1, fstore->newvals, l2, fstore->fieldnums) + Assert(list_length(fstore->newvals) == list_length(fstore->fieldnums)); + Assert(list_length(fstore->newvals) == list_length(fstore->fldparams)); + forthree(l1, fstore->newvals, + l2, fstore->fieldnums, + l3, fstore->fldparams) { Expr *e = (Expr *) lfirst(l1); AttrNumber fieldnum = lfirst_int(l2); - Datum *save_innermost_caseval; - bool *save_innermost_casenull; + int fldparam = lfirst_int(l3); if (fieldnum <= 0 || fieldnum > ncolumns) elog(ERROR, "field number %d is out of range in FieldStore", fieldnum); /* - * Use the CaseTestExpr mechanism to pass down the old - * value of the field being replaced; this is needed in - * case the newval is itself a FieldStore or - * SubscriptingRef that has to obtain and modify the old - * value. It's safe to reuse the CASE mechanism because - * there cannot be a CASE between here and where the value - * would be needed, and a field assignment can't be within - * a CASE either. (So saving and restoring - * innermost_caseval is just paranoia, but let's do it - * anyway.) + * If the new field value contains a reference to the + * field's old value, put that into the appropriate + * PARAM_EXPR Param. This is needed in case the newval is + * itself a FieldStore or SubscriptingRef that has to + * obtain and modify the old value. * - * Another non-obvious point is that it's safe to use the - * field's values[]/nulls[] entries as both the caseval - * source and the result address for this subexpression. - * That's okay only because (1) both FieldStore and - * SubscriptingRef evaluate their arg or refexpr inputs - * first, and (2) any such CaseTestExpr is directly the - * arg or refexpr input. So any read of the caseval will - * occur before there's a chance to overwrite it. Also, - * if multiple entries in the newvals/fieldnums lists - * target the same field, they'll effectively be applied - * left-to-right which is what we want. + * Note: if multiple entries in the newvals/fieldnums + * lists target the same field, they'll effectively be + * applied left-to-right which is what we want. */ - save_innermost_caseval = state->innermost_caseval; - save_innermost_casenull = state->innermost_casenull; - state->innermost_caseval = &values[fieldnum - 1]; - state->innermost_casenull = &nulls[fieldnum - 1]; + if (fldparam > 0) + { + scratch.opcode = EEOP_PARAM_SET_EXPR; + scratch.d.paramset.paramid = fldparam; + scratch.d.paramset.setvalue = &values[fieldnum - 1]; + scratch.d.paramset.setnull = &nulls[fieldnum - 1]; + ExprEvalPushStep(state, &scratch); + } ExecInitExprRec(e, state, &values[fieldnum - 1], &nulls[fieldnum - 1]); - - state->innermost_caseval = save_innermost_caseval; - state->innermost_casenull = save_innermost_casenull; } /* finally, form result tuple */ @@ -3391,9 +3382,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *sbsref, if (isAssignment) { - Datum *save_innermost_caseval; - bool *save_innermost_casenull; - /* Check for unimplemented methods */ if (!methods.sbs_assign) ereport(ERROR, @@ -3406,16 +3394,12 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *sbsref, * refassgnexpr is itself a FieldStore or SubscriptingRef that needs * to obtain and modify the previous value of the array element or * slice being replaced. If so, we have to extract that value from - * the array and pass it down via the CaseTestExpr mechanism. It's - * safe to reuse the CASE mechanism because there cannot be a CASE - * between here and where the value would be needed, and an array - * assignment can't be within a CASE either. (So saving and restoring - * innermost_caseval is just paranoia, but let's do it anyway.) + * the array and pass it down via a PARAM_EXPR Param. * * Since fetching the old element might be a nontrivial expense, do it * only if the argument actually needs it. */ - if (isAssignmentIndirectionExpr(sbsref->refassgnexpr)) + if (sbsref->refassgnparam > 0) { if (!methods.sbs_fetch_old) ereport(ERROR, @@ -3426,21 +3410,18 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *sbsref, scratch->d.sbsref.subscriptfunc = methods.sbs_fetch_old; scratch->d.sbsref.state = sbsrefstate; ExprEvalPushStep(state, scratch); + /* SBSREF_OLD puts extracted value into prevvalue/prevnull */ + scratch->opcode = EEOP_PARAM_SET_EXPR; + scratch->d.paramset.paramid = sbsref->refassgnparam; + scratch->d.paramset.setvalue = &sbsrefstate->prevvalue; + scratch->d.paramset.setnull = &sbsrefstate->prevnull; + ExprEvalPushStep(state, scratch); } - /* SBSREF_OLD puts extracted value into prevvalue/prevnull */ - save_innermost_caseval = state->innermost_caseval; - save_innermost_casenull = state->innermost_casenull; - state->innermost_caseval = &sbsrefstate->prevvalue; - state->innermost_casenull = &sbsrefstate->prevnull; - /* evaluate replacement value into replacevalue/replacenull */ ExecInitExprRec(sbsref->refassgnexpr, state, &sbsrefstate->replacevalue, &sbsrefstate->replacenull); - state->innermost_caseval = save_innermost_caseval; - state->innermost_casenull = save_innermost_casenull; - /* and perform the assignment */ scratch->opcode = EEOP_SBSREF_ASSIGN; scratch->d.sbsref.subscriptfunc = methods.sbs_assign; @@ -3475,57 +3456,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *sbsref, } } -/* - * Helper for preparing SubscriptingRef expressions for evaluation: is expr - * a nested FieldStore or SubscriptingRef that needs the old element value - * passed down? - * - * (We could use this in FieldStore too, but in that case passing the old - * value is so cheap there's no need.) - * - * Note: it might seem that this needs to recurse, but in most cases it does - * not; the CaseTestExpr, if any, will be directly the arg or refexpr of the - * top-level node. Nested-assignment situations give rise to expression - * trees in which each level of assignment has its own CaseTestExpr, and the - * recursive structure appears within the newvals or refassgnexpr field. - * There is an exception, though: if the array is an array-of-domain, we will - * have a CoerceToDomain or RelabelType as the refassgnexpr, and we need to - * be able to look through that. - */ -static bool -isAssignmentIndirectionExpr(Expr *expr) -{ - if (expr == NULL) - return false; /* just paranoia */ - if (IsA(expr, FieldStore)) - { - FieldStore *fstore = (FieldStore *) expr; - - if (fstore->arg && IsA(fstore->arg, CaseTestExpr)) - return true; - } - else if (IsA(expr, SubscriptingRef)) - { - SubscriptingRef *sbsRef = (SubscriptingRef *) expr; - - if (sbsRef->refexpr && IsA(sbsRef->refexpr, CaseTestExpr)) - return true; - } - else if (IsA(expr, CoerceToDomain)) - { - CoerceToDomain *cd = (CoerceToDomain *) expr; - - return isAssignmentIndirectionExpr(cd->arg); - } - else if (IsA(expr, RelabelType)) - { - RelabelType *r = (RelabelType *) expr; - - return isAssignmentIndirectionExpr(r->arg); - } - return false; -} - /* * Prepare evaluation of a CoerceToDomain expression. */ diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c index 7bc823507f..a4e9d9ef31 100644 --- a/src/backend/nodes/nodeFuncs.c +++ b/src/backend/nodes/nodeFuncs.c @@ -3225,6 +3225,7 @@ expression_tree_mutator_impl(Node *node, MUTATE(newnode->arg, fstore->arg, Expr *); MUTATE(newnode->newvals, fstore->newvals, List *); newnode->fieldnums = list_copy(fstore->fieldnums); + newnode->fldparams = list_copy(fstore->fldparams); return (Node *) newnode; } break; diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index d638a290ec..469ef35209 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -149,6 +149,7 @@ static Node *substitute_actual_parameters_mutator(Node *node, substitute_actual_parameters_context *context); static void sql_inline_error_callback(void *arg); static bool find_casetestexpr(Node *node, CaseTestExpr **result); +static bool isAssignmentIndirectionExpr(Expr *expr, CaseTestExpr **result); static Param *generate_new_expr_param(eval_const_expressions_context *context, Oid paramtype, int32 paramtypmod, Oid paramcollation); @@ -2879,6 +2880,168 @@ eval_const_expressions_mutator(Node *node, * should never be invoked after SubPlan creation. */ return node; + + case T_SubscriptingRef: + { + SubscriptingRef *sbsref = makeNode(SubscriptingRef); + CaseTestExpr *ctexpr = NULL; + + /* + * Copy the node and const-simplify its arguments. We can't + * use ece_generic_processing() here because we may need to + * mess with case_val while processing the refassgnexpr. + */ + memcpy(sbsref, node, sizeof(SubscriptingRef)); + sbsref->refupperindexpr = (List *) + eval_const_expressions_mutator((Node *) sbsref->refupperindexpr, + context); + sbsref->reflowerindexpr = (List *) + eval_const_expressions_mutator((Node *) sbsref->reflowerindexpr, + context); + sbsref->refexpr = (Expr *) + eval_const_expressions_mutator((Node *) sbsref->refexpr, + context); + + /* + * The refassgnexpr might or might not contain a CaseTestExpr; + * if it doesn't, we don't want to uselessly consume a + * PARAM_EXPR slot. Furthermore, we have a good idea of the + * possible structure of the refassgnexpr, so apply + * specialized code to avoid being fooled by other + * CaseTestExprs. + */ + if (isAssignmentIndirectionExpr(sbsref->refassgnexpr, &ctexpr)) + { + /* + * Set up the node to substitute for the CaseTestExpr node + * contained in refassgnexpr. Normally we replace it with + * a PARAM_EXPR Param, but if we're just estimating, leave + * it alone. + * + * We must save and restore context->case_val so as not to + * affect surrounding constructs. + */ + Node *save_case_val = context->case_val; + + /* If we found a CaseTestExpr, we can't be re-simplifying */ + Assert(sbsref->refassgnparam == 0); + if (!context->estimate) + { + Param *p; + + p = generate_new_expr_param(context, + ctexpr->typeId, + ctexpr->typeMod, + ctexpr->collation); + context->case_val = (Node *) p; + sbsref->refassgnparam = p->paramid; + } + else + context->case_val = NULL; + + sbsref->refassgnexpr = (Expr *) + eval_const_expressions_mutator((Node *) sbsref->refassgnexpr, + context); + + context->case_val = save_case_val; + } + else + sbsref->refassgnexpr = (Expr *) + eval_const_expressions_mutator((Node *) sbsref->refassgnexpr, + context); + + /* + * If all arguments are Consts, we can fold to a constant. + * Treating SubscriptingRef this way assumes that subscripting + * fetch and assignment are both immutable. This constrains + * type-specific subscripting implementations; maybe we should + * relax it someday. + */ + if (ece_all_arguments_const(sbsref)) + return ece_evaluate_expr(sbsref); + + return (Node *) sbsref; + } + + case T_FieldStore: + { + FieldStore *fstore = makeNode(FieldStore); + Node *save_case_val = context->case_val; + + /* + * Copy the node and const-simplify its arguments. We can't + * use ece_generic_processing() here because we need to mess + * with case_val while processing the fields. + */ + memcpy(fstore, node, sizeof(FieldStore)); + fstore->arg = (Expr *) + eval_const_expressions_mutator((Node *) fstore->arg, + context); + + /* + * If we're re-simplifying the node, fldparams is already + * computed and we mustn't modify it, just copy it. + */ + if (fstore->fldparams != NIL) + { + context->case_val = NULL; /* No CaseTestExprs expected */ + fstore->newvals = (List *) + eval_const_expressions_mutator((Node *) fstore->newvals, + context); + fstore->fldparams = list_copy(fstore->fldparams); + } + else + { + List *newnewvals = NIL; + ListCell *lc; + + /* + * For each field value, check whether it involves a + * CaseTestExpr, and if so set up to replace that with a + * PARAM_EXPR Param. Put zeroes into the fldparams list + * for fields that don't need a Param. + */ + foreach(lc, fstore->newvals) + { + Node *newval = lfirst(lc); + CaseTestExpr *ctexpr = NULL; + int fparam; + + /* + * We have a good idea of the possible structure of + * the newval, so apply specialized code to avoid + * being fooled by other CaseTestExprs. + */ + if (isAssignmentIndirectionExpr((Expr *) newval, + &ctexpr)) + { + Param *p; + + p = generate_new_expr_param(context, + ctexpr->typeId, + ctexpr->typeMod, + ctexpr->collation); + context->case_val = (Node *) p; + fparam = p->paramid; + } + else + { + context->case_val = NULL; + fparam = 0; + } + newval = eval_const_expressions_mutator(newval, + context); + newnewvals = lappend(newnewvals, newval); + fstore->fldparams = lappend_int(fstore->fldparams, + fparam); + } + fstore->newvals = newnewvals; + } + fstore->fieldnums = list_copy(fstore->fieldnums); + context->case_val = save_case_val; + return (Node *) fstore; + } + case T_RelabelType: { RelabelType *relabel = (RelabelType *) node; @@ -3283,7 +3446,6 @@ eval_const_expressions_mutator(Node *node, else return copyObject(node); } - case T_SubscriptingRef: case T_ArrayExpr: case T_RowExpr: case T_MinMaxExpr: @@ -3293,11 +3455,6 @@ eval_const_expressions_mutator(Node *node, * known to be immutable, and for which we need no smarts * beyond "simplify if all inputs are constants". * - * Treating SubscriptingRef this way assumes that subscripting - * fetch and assignment are both immutable. This constrains - * type-specific subscripting implementations; maybe we should - * relax it someday. - * * Treating MinMaxExpr this way amounts to assuming that the * btree comparison function it calls is immutable; see the * reasoning in contain_mutable_functions_walker. @@ -5142,6 +5299,57 @@ find_casetestexpr(Node *node, CaseTestExpr **result) return expression_tree_walker(node, find_casetestexpr, result); } +/* + * Locate the CaseTestExpr, if any, within a nested FieldStore or + * SubscriptingRef assignment source expression. + * + * This has basically the same charter as find_casetestexpr, but we have + * specific knowledge of where the relevant CaseTestExpr could be, and + * we want to ignore any others so as not to generate useless Params. + * + * Note: it might seem that this needs to recurse, but in most cases it does + * not; the CaseTestExpr, if any, will be directly the arg or refexpr of the + * top-level node. Nested-assignment situations give rise to expression + * trees in which each level of assignment has its own CaseTestExpr, and the + * recursive structure appears within the newvals or refassgnexpr field. + * There is an exception, though: if an array is an array-of-domain, we will + * have a CoerceToDomain as the refassgnexpr, and we need to be able to look + * through that. + */ +static bool +isAssignmentIndirectionExpr(Expr *expr, CaseTestExpr **result) +{ + if (expr == NULL) + return false; + if (IsA(expr, FieldStore)) + { + FieldStore *fstore = (FieldStore *) expr; + + if (fstore->arg && IsA(fstore->arg, CaseTestExpr)) + { + *result = (CaseTestExpr *) fstore->arg; + return true; + } + } + else if (IsA(expr, SubscriptingRef)) + { + SubscriptingRef *sbsRef = (SubscriptingRef *) expr; + + if (sbsRef->refexpr && IsA(sbsRef->refexpr, CaseTestExpr)) + { + *result = (CaseTestExpr *) sbsRef->refexpr; + return true; + } + } + else if (IsA(expr, CoerceToDomain)) + { + CoerceToDomain *cd = (CoerceToDomain *) expr; + + return isAssignmentIndirectionExpr(cd->arg, result); + } + return false; +} + /* * Generate a new PARAM_EXPR Param node that will not conflict with any other. * diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c index 4aba0d9d4d..b01138b4b7 100644 --- a/src/backend/parser/parse_target.c +++ b/src/backend/parser/parse_target.c @@ -707,7 +707,9 @@ transformAssignmentIndirection(ParseState *pstate, * to do so because the only nodes that will be above the CaseTestExpr * in the finished expression will be FieldStore and SubscriptingRef * nodes. (There could be other stuff in the tree, but it will be - * within other child fields of those node types.) + * within other child fields of those node types.) The planner will + * replace the CaseTestExpr with a PARAM_EXPR Param node to remove + * ambiguity before it performs any expression optimization. */ CaseTestExpr *ctest = makeNode(CaseTestExpr); diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 847edcfa90..bca7b45b98 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -1150,6 +1150,8 @@ process_matched_tle(TargetEntry *src_tle, fstore->fieldnums = list_concat_copy(((FieldStore *) prior_expr)->fieldnums, ((FieldStore *) src_expr)->fieldnums); + /* fldparams has not been computed yet */ + Assert(fstore->fldparams == NIL); } else { diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 59a4cde467..9186ebe7d9 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -9248,22 +9248,6 @@ get_rule_expr(Node *node, deparse_context *context, SubscriptingRef *sbsref = (SubscriptingRef *) node; bool need_parens; - /* - * If the argument is a CaseTestExpr, we must be inside a - * FieldStore, ie, we are assigning to an element of an array - * within a composite column. Since we already punted on - * displaying the FieldStore's target information, just punt - * here too, and display only the assignment source - * expression. - */ - if (IsA(sbsref->refexpr, CaseTestExpr)) - { - Assert(sbsref->refassgnexpr); - get_rule_expr((Node *) sbsref->refassgnexpr, - context, showimplicit); - break; - } - /* * Parenthesize the argument unless it's a simple Var or a * FieldSelect. (In particular, if it's another diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index aad1fc2897..e01ffd37c8 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -698,6 +698,11 @@ typedef struct MergeSupportFunc * subscripting logic. Likewise, reftypmod and refcollid will match the * container's properties in a store, but could be different in a fetch. * + * In a store, it might be necessary for the refassgnexpr to reference the + * old value of the element being replaced (this happens with nested container + * types). If so, the parser will generate a CaseTestExpr that represents + * the old value, which the planner will replace with a PARAM_EXPR Param. + * * Any internal state data is ignored for the query jumbling. * * Note: for the cases where a container is returned, if refexpr yields a R/W @@ -729,6 +734,8 @@ typedef struct SubscriptingRef Expr *refexpr; /* expression for the source value, or NULL if fetch */ Expr *refassgnexpr; + /* ID of PARAM_EXPR Param for old value within refassgnexpr; 0 if none */ + int refassgnparam pg_node_attr(equal_ignore, query_jumble_ignore); } SubscriptingRef; /* @@ -1178,6 +1185,14 @@ typedef struct FieldSelect * fields. The parser only generates FieldStores with single-element lists, * but the planner will collapse multiple updates of the same base column * into one FieldStore. + * + * In nested-assignment cases, such as assigning to an element of an array + * that is a field in the given tuple, an element of the newvals list can + * itself be a FieldStore or SubscriptingRef that has to operate on the old + * value of the field. The old value will be represented by a CaseTestExpr + * in the output of the parser. The planner will replace that with a + * PARAM_EXPR Param, to ensure that no confusion arises during expression + * optimization. * ---------------- */ @@ -1188,6 +1203,8 @@ typedef struct FieldStore List *newvals; /* new value(s) for field(s) */ /* integer list of field attnums */ List *fieldnums pg_node_attr(query_jumble_ignore); + /* integer list of Param IDs, or 0 if not needed (filled by planner) */ + List *fldparams pg_node_attr(equal_ignore, query_jumble_ignore); /* type of result (same as type of arg) */ Oid resulttype pg_node_attr(query_jumble_ignore); /* Like RowExpr, we deliberately omit a typmod and collation here */ -- 2.43.5