From 5a01fc722d63eeff63f764349c24c45aceb8590e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 29 Jan 2025 18:30:31 -0500 Subject: [PATCH v1 2/8] Convert ArrayCoerceExpr to use a Param instead of CaseTestExpr. Just turning the crank ... --- src/backend/executor/execExpr.c | 22 ++++++-- src/backend/optimizer/util/clauses.c | 77 ++++++++++++++++++++++++---- src/backend/parser/parse_coerce.c | 10 ++-- src/backend/utils/adt/arrayfuncs.c | 6 +-- src/backend/utils/adt/selfuncs.c | 8 +-- src/include/nodes/primnodes.h | 7 ++- 6 files changed, 105 insertions(+), 25 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 2a66363502..13185652ec 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -1698,21 +1698,33 @@ ExecInitExprRec(Expr *node, ExprState *state, * Construct a sub-expression for the per-element expression; * but don't ready it until after we check it for triviality. * We assume it hasn't any Var references, but does have a - * CaseTestExpr representing the source array element values. + * Param representing the source array element values. */ elemstate = makeNode(ExprState); elemstate->expr = acoerce->elemexpr; elemstate->parent = state->parent; elemstate->ext_params = state->ext_params; - elemstate->innermost_caseval = (Datum *) palloc(sizeof(Datum)); - elemstate->innermost_casenull = (bool *) palloc(sizeof(bool)); + /* + * Push a step that copies the elemstate's resvalue/resnull + * into the appropriate Param slot; this is so that array_map + * doesn't need much knowledge about where to put the element + * values. The generated expression might not do anything + * more than fetch the value back; if so we optimize it below. + */ + scratch.opcode = EEOP_PARAM_SET_EXPR; + Assert(acoerce->acparam > 0); + scratch.d.paramset.paramid = acoerce->acparam; + scratch.d.paramset.setvalue = &elemstate->resvalue; + scratch.d.paramset.setnull = &elemstate->resnull; + ExprEvalPushStep(elemstate, &scratch); ExecInitExprRec(acoerce->elemexpr, elemstate, &elemstate->resvalue, &elemstate->resnull); - if (elemstate->steps_len == 1 && - elemstate->steps[0].opcode == EEOP_CASE_TESTVAL) + if (elemstate->steps_len == 2 && + elemstate->steps[0].opcode == EEOP_PARAM_SET_EXPR && + elemstate->steps[1].opcode == EEOP_PARAM_EXPR) { /* Trivial, so we need no per-element work at runtime */ elemstate = NULL; diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index b1ffe3c4f7..4edbbabf59 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -150,6 +150,7 @@ static Node *substitute_actual_parameters(Node *expr, int nargs, List *args, 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 Param *generate_new_expr_param(eval_const_expressions_context *context, Oid paramtype, int32 paramtypmod, Oid paramcollation); @@ -3075,7 +3076,7 @@ eval_const_expressions_mutator(Node *node, case T_ArrayCoerceExpr: { ArrayCoerceExpr *ac = makeNode(ArrayCoerceExpr); - Node *save_case_val; + CaseTestExpr *ctexpr = NULL; /* * Copy the node and const-simplify its arguments. We can't @@ -3088,17 +3089,55 @@ eval_const_expressions_mutator(Node *node, context); /* - * Set up for the CaseTestExpr node contained in the elemexpr. - * We must prevent it from absorbing any outer CASE value. + * Find the CaseTestExpr in elemexpr; this is probably much + * cheaper than recomputing its type/typmod/collation from the + * values we have. Also, if we don't find it, we know that we + * are re-simplifying an already-simplified ArrayCoerceExpr + * and must not change acparam. */ - save_case_val = context->case_val; - context->case_val = NULL; + if (find_casetestexpr((Node *) ac->elemexpr, &ctexpr)) + { + /* + * Set up the node to substitute for the CaseTestExpr node + * contained in elemexpr. Normally we replace it with a + * PARAM_EXPR Param, but if we're just estimating, leave + * it alone. (Even in estimation mode, assign a Param if + * we might try to evaluate the expression below. This + * will sometimes result in wasting a PARAM_EXPR slot, but + * that hardly matters.) + * + * We must save and restore context->case_val so as not to + * affect surrounding constructs. + */ + Node *save_case_val = context->case_val; - ac->elemexpr = (Expr *) - eval_const_expressions_mutator((Node *) ac->elemexpr, - context); + /* If we found a CaseTestExpr, we can't be re-simplifying */ + Assert(ac->acparam == 0); + if (!context->estimate || + (ac->arg && IsA(ac->arg, Const))) + { + Param *p; + + p = generate_new_expr_param(context, + ctexpr->typeId, + ctexpr->typeMod, + ctexpr->collation); + context->case_val = (Node *) p; + ac->acparam = p->paramid; + } + else + context->case_val = NULL; - context->case_val = save_case_val; + ac->elemexpr = (Expr *) + eval_const_expressions_mutator((Node *) ac->elemexpr, + context); + + context->case_val = save_case_val; + } + else + ac->elemexpr = (Expr *) + eval_const_expressions_mutator((Node *) ac->elemexpr, + context); /* * If constant argument and the per-element expression is @@ -5040,6 +5079,26 @@ sql_inline_error_callback(void *arg) errcontext("SQL function \"%s\" during inlining", callback_arg->proname); } +/* + * Locate the CaseTestExpr, if any, within the given node tree. + * + * This should only be called on node trees in which it's certain that + * there's not more than one CaseTestExpr, else the result is indeterminate. + * Note that *result is left unset on false return. + */ +static bool +find_casetestexpr(Node *node, CaseTestExpr **result) +{ + if (node == NULL) + return false; + if (IsA(node, CaseTestExpr)) + { + *result = (CaseTestExpr *) node; + return true; + } + return expression_tree_walker(node, find_casetestexpr, result); +} + /* * Generate a new PARAM_EXPR Param node that will not conflict with any other. * diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c index 0b5b81c7f2..39f42e6a60 100644 --- a/src/backend/parser/parse_coerce.c +++ b/src/backend/parser/parse_coerce.c @@ -936,9 +936,13 @@ build_coercion_expression(Node *node, /* * Set up a CaseTestExpr representing one element of the source array. - * This is an abuse of CaseTestExpr, but it's OK as long as there - * can't be any CaseExpr or ArrayCoerceExpr within the completed - * elemexpr. + * This is an abuse of CaseTestExpr, but it's safe because the + * finished elemexpr will simply be a coercion applied to the + * CaseTestExpr. The coercion expression cannot directly contain a + * CASE, nor any other construct that similarly abuses CaseTestExpr. + * (Subsequent inlining of a SQL-language cast function could create a + * hazard, but we leave it to the planner to deal with that scenario + * by replacing the CaseTestExpr with a Param beforehand.) */ ctest->typeId = get_element_type(sourceBaseTypeId); Assert(OidIsValid(ctest->typeId)); diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c index d777f38ed9..b5414a7844 100644 --- a/src/backend/utils/adt/arrayfuncs.c +++ b/src/backend/utils/adt/arrayfuncs.c @@ -3177,7 +3177,7 @@ array_set(ArrayType *array, int nSubscripts, int *indx, * Map an array through an arbitrary expression. Return a new array with * the same dimensions and each source element transformed by the given, * already-compiled expression. Each source element is placed in the - * innermost_caseval/innermost_casenull fields of the ExprState. + * resvalue/resnull fields of the ExprState before evaluating the expression. * * Parameters are: * * arrayd: Datum representing array argument. @@ -3223,8 +3223,8 @@ array_map(Datum arrayd, array_iter iter; ArrayMetaState *inp_extra; ArrayMetaState *ret_extra; - Datum *transform_source = exprstate->innermost_caseval; - bool *transform_source_isnull = exprstate->innermost_casenull; + Datum *transform_source = &exprstate->resvalue; + bool *transform_source_isnull = &exprstate->resnull; inpType = AARR_ELEMTYPE(v); ndim = AARR_NDIM(v); diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index d3d1e485bb..3673c5187d 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -1790,11 +1790,13 @@ strip_array_coercion(Node *node) ArrayCoerceExpr *acoerce = (ArrayCoerceExpr *) node; /* - * If the per-element expression is just a RelabelType on top of - * CaseTestExpr, then we know it's a binary-compatible relabeling. + * If the per-element expression is just a RelabelType on top of a + * Param, then we know it's a binary-compatible relabeling. (We + * could verify that it's the matching PARAM_EXPR Param, but that + * seems like overkill.) */ if (IsA(acoerce->elemexpr, RelabelType) && - IsA(((RelabelType *) acoerce->elemexpr)->arg, CaseTestExpr)) + IsA(((RelabelType *) acoerce->elemexpr)->arg, Param)) node = (Node *) acoerce->arg; else break; diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index af55661df9..2c87e6365d 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -1248,8 +1248,9 @@ typedef struct CoerceViaIO * ArrayCoerceExpr represents a type coercion from one array type to another, * which is implemented by applying the per-element coercion expression * "elemexpr" to each element of the source array. Within elemexpr, the - * source element is represented by a CaseTestExpr node. Note that even if - * elemexpr is a no-op (that is, just CaseTestExpr + RelabelType), the + * source element is initially represented by a CaseTestExpr node, which + * will be replaced by a PARAM_EXPR Param during planning. Note that even + * if elemexpr is a no-op (that is, just CaseTestExpr + RelabelType), the * coercion still requires some effort: we have to fix the element type OID * stored in the array header. * ---------------- @@ -1267,6 +1268,8 @@ typedef struct ArrayCoerceExpr Oid resultcollid pg_node_attr(query_jumble_ignore); /* how to display this node */ CoercionForm coerceformat pg_node_attr(query_jumble_ignore); + /* ID of PARAM_EXPR Param representing input, if assigned; else 0 */ + int acparam pg_node_attr(equal_ignore, query_jumble_ignore); ParseLoc location; /* token location, or -1 if unknown */ } ArrayCoerceExpr; -- 2.43.5