We've wanted to refactor or get rid of CaseTestExpr for quite awhile,
as a consequence of bugs caused by the loose connection between
CaseTestExpr and the node supplying its value. The increasing use of
it for random purposes like JsonExpr hasn't made things any safer.
While this has been in the back of my mind for awhile, looking at
the discussion at [1] nerd-sniped me into trying to actually do it.
My initial idea was to generate PARAM_EXEC Params instead of
CaseTestExprs, relying on unique assignment of their param IDs to
distinguish different values within an expression. While it'd not be
hard to get the parser to assign distinct IDs within one Query tree,
I soon found that maintaining their uniqueness through operations like
rule rewriting, subquery pullup, and qual pushdown into subqueries
would be a nightmare. Furthermore, it didn't seem like it'd be buying
much. None of those operations deform expressions in a way that would
risk inserting a different use of CaseTestExpr between a source and
target node. The thing that does cause such problems is inlining of
SQL function bodies into expressions.
Therefore, my proposal here is to leave the parser's usage of
CaseTestExpr alone, and replace CaseTestExprs with Params during
eval_const_expressions, just before any relevant function inlining
could happen. (The information transfer needed to replace them
happens during initial descent of the expression tree, while
function inlining would happen on the way back up. So by the
time a function inlining step happens, any CTEs in its arguments
will have become Params, and no ambiguity is possible.)
The executor will never see any CaseTestExprs any more, so its
support for them can still go away.
I also discovered that re-using PARAM_EXEC Params wasn't as good
an idea as it seemed. The setParam/parParam signaling mechanisms
assume that PARAM_EXEC Params represent cross-plan-node data
transfer, and the parallel-query logic does too. So this patch
series invents a new paramkind for intra-expression data transfer,
which I called PARAM_EXPR for lack of a better idea.
Given those decisions, the actual coding seemed pretty
straightforward, if tedious.
I'd originally thought of similarly replacing CoerceToDomainValue
with a Param, but desisted after finding that it wasn't as simple as
I'd hoped. There is not really any ambiguity in what's referenced by
such a node, since domain CHECK expressions can never get intermixed,
so the benefit would be quite minimal.
One idea that's left undone in the attached patch series is to
rename CaseTestExpr to something more generic that reflects its
current usage (and then rewrite the comments that claim the other
usages are "abuse"). That would be a purely cosmetic change and
I ran out of energy. Besides, I don't have a great idea for a
new name. I thought of ParamPlaceHolder to betoken that the
things will eventually get replaced by Params, but that seems
to risk confusion with the planner's PlaceHolderVar nodes.
Any ideas?
There's more detail in the per-patch commit messages below.
regards, tom lane
[1]
https://www.postgresql.org/message-id/flat/CACpMh%2BAiBYAWn%2BD1aU7Rsy-V1tox06Cbc0H3qA7rwL5zdJ%3DanQ%40mail.gmail.com
From 926b7bef8c2aa05380849e7da1802dd7de743772 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 29 Jan 2025 17:51:29 -0500
Subject: [PATCH v1 1/8] Replace CaseTestExpr with a Param during planning.
For some time we've wanted to get rid of CaseTestExpr and replace it
with a less-spongily-defined node type, because we have had bugs
caused by the lack of a clear connection between the CaseTestExpr and
the expression node providing its value. See for example commits
f0c7b789a and 14a158f9b. This patch series implements a partial fix:
replace CaseTestExpr with a Param early in planning.
I tried to generate Params in parse analysis and not use CaseTestExpr
ever, but it turns out to be far too hard to keep the Params' IDs
distinct during rule rewriting, subquery pullup, etc. It doesn't
seem really necessary either: in practice, the transformations that
risk confusion between different CaseTestExpr-using expressions don't
occur before eval_const_expressions. So as long as we lock down the
association between a CaseExpr and its controlled nodes when
eval_const_expressions first sees the controlled nodes, we'll be okay.
It turns out to be a good idea to invent a new paramkind, here
called PARAM_EXPR, for this purpose. I first intended to just use
PARAM_EXEC Params, but it's valuable to instead define PARAM_EXEC
as being for values passed between plan nodes while PARAM_EXPR is
for values passed around within a single expression. In particular
this allows easily treating PARAM_EXPR Params as parallel-safe,
which they should be since we'll never break a single expression
across worker nodes. The machinations around setParam signaling
should ignore these Params too.
This first patch installs the infrastructure for PARAM_EXPR Params
and causes CaseTestExpr nodes used for CASE to be replaced properly.
The various places that abuse CaseTestExpr for other purposes will
be dealt with in subsequent patches, with the end goal of removing
executor support for CaseTestExpr.
---
contrib/postgres_fdw/deparse.c | 191 +++++++++++++++-----------
src/backend/executor/execExpr.c | 53 ++++---
src/backend/executor/execExprInterp.c | 84 ++++++++++-
src/backend/executor/execMain.c | 23 +++-
src/backend/executor/execParallel.c | 1 +
src/backend/executor/execUtils.c | 6 +
src/backend/jit/llvm/llvmjit_expr.c | 16 ++-
src/backend/jit/llvm/llvmjit_types.c | 4 +-
src/backend/optimizer/plan/planner.c | 2 +
src/backend/optimizer/util/clauses.c | 144 +++++++++++++++++--
src/backend/parser/parse_expr.c | 5 +
src/backend/utils/adt/ruleutils.c | 36 +++--
src/include/executor/execExpr.h | 27 +++-
src/include/nodes/execnodes.h | 20 +--
src/include/nodes/params.h | 27 +++-
src/include/nodes/pathnodes.h | 3 +
src/include/nodes/plannodes.h | 2 +
src/include/nodes/primnodes.h | 26 +++-
src/tools/pgindent/typedefs.list | 2 +
19 files changed, 505 insertions(+), 167 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 6e7dc3d2df..7299fbc273 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -91,6 +91,18 @@ typedef struct foreign_loc_cxt
FDWCollateState state; /* state of current collation choice */
} foreign_loc_cxt;
+/*
+ * To locate collation info for the source of a PARAM_EXPR Param, we use a
+ * linked list of these structs. The sources and Params are strictly nested
+ * in expression trees, so it suffices to keep these on the stack.
+ */
+typedef struct foreign_param_cxt
+{
+ int paramid; /* paramid of the Param(s) this matches */
+ foreign_loc_cxt src_cxt; /* collation info for Param's source expr */
+ struct foreign_param_cxt *next; /* link to next innermost item, if any */
+} foreign_param_cxt;
+
/*
* Context for deparseExpr
*/
@@ -119,7 +131,7 @@ typedef struct deparse_expr_cxt
static bool foreign_expr_walker(Node *node,
foreign_glob_cxt *glob_cxt,
foreign_loc_cxt *outer_cxt,
- foreign_loc_cxt *case_arg_cxt);
+ foreign_param_cxt *param_cxt);
static char *deparse_type_name(Oid type_oid, int32 typemod);
/*
@@ -294,9 +306,8 @@ is_foreign_expr(PlannerInfo *root,
*
* In addition, *outer_cxt is updated with collation information.
*
- * case_arg_cxt is NULL if this subexpression is not inside a CASE-with-arg.
- * Otherwise, it points to the collation info derived from the arg expression,
- * which must be consulted by any CaseTestExpr.
+ * param_cxt points to a chain (initially empty) of foreign_param_cxt nodes
+ * that describe the sources of PARAM_EXPR Params, such as a CASE-with-arg.
*
* We must check that the expression contains only node types we can deparse,
* that all types/functions/operators are safe to send (they are "shippable"),
@@ -310,7 +321,7 @@ static bool
foreign_expr_walker(Node *node,
foreign_glob_cxt *glob_cxt,
foreign_loc_cxt *outer_cxt,
- foreign_loc_cxt *case_arg_cxt)
+ foreign_param_cxt *param_cxt)
{
bool check_type = true;
PgFdwRelationInfo *fpinfo;
@@ -476,6 +487,56 @@ foreign_expr_walker(Node *node,
{
Param *p = (Param *) node;
+ collation = p->paramcollid;
+
+ /*
+ * If it's a PARAM_EXPR Param, try to find the source. Punt
+ * if we can't. (That probably shouldn't happen, since we'd
+ * not have descended into the current subexpression if we
+ * didn't recognize the outer node that supplies the Param.)
+ */
+ if (p->paramkind == PARAM_EXPR)
+ {
+ foreign_param_cxt *pcxt;
+
+ for (pcxt = param_cxt; pcxt != NULL; pcxt = pcxt->next)
+ {
+ if (pcxt->paramid == p->paramid)
+ break;
+ }
+ if (pcxt == NULL)
+ return false;
+
+ /*
+ * Otherwise, any nondefault collation attached to the
+ * Param node must be derived from foreign Var(s) in the
+ * source expression.
+ */
+ if (collation == InvalidOid)
+ state = FDW_COLLATE_NONE;
+ else if (pcxt->src_cxt.state == FDW_COLLATE_SAFE &&
+ collation == pcxt->src_cxt.collation)
+ state = FDW_COLLATE_SAFE;
+ else if (collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ else if (p->paramkind == PARAM_EXTERN ||
+ p->paramkind == PARAM_EXEC)
+ {
+ /*
+ * Param supplied from outside the current plan node.
+ * Collation rule is same as for Consts and non-foreign
+ * Vars.
+ */
+ if (collation == InvalidOid ||
+ collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+
/*
* If it's a MULTIEXPR Param, punt. We can't tell from here
* whether the referenced sublink/subplan contains any remote
@@ -488,19 +549,11 @@ foreign_expr_walker(Node *node,
* references a sub-select elsewhere in the same targetlist,
* so we'd be on the hook to evaluate it somehow if we wanted
* to handle such cases as direct foreign updates.)
+ *
+ * We punt for any unrecognized paramkind, too.
*/
- if (p->paramkind == PARAM_MULTIEXPR)
- return false;
-
- /*
- * Collation rule is same as for Consts and non-foreign Vars.
- */
- collation = p->paramcollid;
- if (collation == InvalidOid ||
- collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
else
- state = FDW_COLLATE_UNSAFE;
+ return false;
}
break;
case T_SubscriptingRef:
@@ -517,17 +570,17 @@ foreign_expr_walker(Node *node,
* result, so do those first and reset inner_cxt afterwards.
*/
if (!foreign_expr_walker((Node *) sr->refupperindexpr,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
inner_cxt.collation = InvalidOid;
inner_cxt.state = FDW_COLLATE_NONE;
if (!foreign_expr_walker((Node *) sr->reflowerindexpr,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
inner_cxt.collation = InvalidOid;
inner_cxt.state = FDW_COLLATE_NONE;
if (!foreign_expr_walker((Node *) sr->refexpr,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/*
@@ -563,7 +616,7 @@ foreign_expr_walker(Node *node,
* Recurse to input subexpressions.
*/
if (!foreign_expr_walker((Node *) fe->args,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/*
@@ -611,7 +664,7 @@ foreign_expr_walker(Node *node,
* Recurse to input subexpressions.
*/
if (!foreign_expr_walker((Node *) oe->args,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/*
@@ -651,7 +704,7 @@ foreign_expr_walker(Node *node,
* Recurse to input subexpressions.
*/
if (!foreign_expr_walker((Node *) oe->args,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/*
@@ -677,7 +730,7 @@ foreign_expr_walker(Node *node,
* Recurse to input subexpression.
*/
if (!foreign_expr_walker((Node *) r->arg,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/*
@@ -704,7 +757,7 @@ foreign_expr_walker(Node *node,
* Recurse to input subexpressions.
*/
if (!foreign_expr_walker((Node *) b->args,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/* Output is always boolean and so noncollatable. */
@@ -720,7 +773,7 @@ foreign_expr_walker(Node *node,
* Recurse to input subexpressions.
*/
if (!foreign_expr_walker((Node *) nt->arg,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/* Output is always boolean and so noncollatable. */
@@ -731,23 +784,29 @@ foreign_expr_walker(Node *node,
case T_CaseExpr:
{
CaseExpr *ce = (CaseExpr *) node;
- foreign_loc_cxt arg_cxt;
+ foreign_param_cxt arg_cxt;
+ foreign_param_cxt *when_cxt;
foreign_loc_cxt tmp_cxt;
ListCell *lc;
/*
* Recurse to CASE's arg expression, if any. Its collation
- * has to be saved aside for use while examining CaseTestExprs
- * within the WHEN expressions.
+ * has to be saved aside for use while examining PARAM_EXPR
+ * Params within the WHEN expressions.
*/
- arg_cxt.collation = InvalidOid;
- arg_cxt.state = FDW_COLLATE_NONE;
if (ce->arg)
{
- if (!foreign_expr_walker((Node *) ce->arg,
- glob_cxt, &arg_cxt, case_arg_cxt))
+ arg_cxt.src_cxt.collation = InvalidOid;
+ arg_cxt.src_cxt.state = FDW_COLLATE_NONE;
+ if (!foreign_expr_walker((Node *) ce->arg, glob_cxt,
+ &arg_cxt.src_cxt, param_cxt))
return false;
+ arg_cxt.paramid = ce->caseparam;
+ arg_cxt.next = param_cxt;
+ when_cxt = &arg_cxt;
}
+ else
+ when_cxt = param_cxt;
/* Examine the CaseWhen subexpressions. */
foreach(lc, ce->args)
@@ -757,26 +816,29 @@ foreign_expr_walker(Node *node,
if (ce->arg)
{
/*
- * In a CASE-with-arg, the parser should have produced
- * WHEN clauses of the form "CaseTestExpr = RHS",
- * possibly with an implicit coercion inserted above
- * the CaseTestExpr. However in an expression that's
- * been through the optimizer, the WHEN clause could
- * be almost anything (since the equality operator
- * could have been expanded into an inline function).
- * In such cases forbid pushdown, because
+ * In a CASE-with-arg, the planner should have
+ * produced WHEN clauses of the form "PARAM_EXPR Param
+ * = RHS", possibly with an implicit coercion inserted
+ * above the Param. But after optimization, the WHEN
+ * clause could be almost anything (since the equality
+ * operator could have been expanded into an inline
+ * function). In such cases forbid pushdown, because
* deparseCaseExpr can't handle it.
*/
Node *whenExpr = (Node *) cw->expr;
List *opArgs;
+ Node *larg;
if (!IsA(whenExpr, OpExpr))
return false;
-
opArgs = ((OpExpr *) whenExpr)->args;
- if (list_length(opArgs) != 2 ||
- !IsA(strip_implicit_coercions(linitial(opArgs)),
- CaseTestExpr))
+ if (list_length(opArgs) != 2)
+ return false;
+ larg = strip_implicit_coercions(linitial(opArgs));
+ if (!IsA(larg, Param))
+ return false;
+ if (((Param *) larg)->paramkind != PARAM_EXPR ||
+ ((Param *) larg)->paramid != ce->caseparam)
return false;
}
@@ -788,18 +850,18 @@ foreign_expr_walker(Node *node,
tmp_cxt.collation = InvalidOid;
tmp_cxt.state = FDW_COLLATE_NONE;
if (!foreign_expr_walker((Node *) cw->expr,
- glob_cxt, &tmp_cxt, &arg_cxt))
+ glob_cxt, &tmp_cxt, when_cxt))
return false;
/* Recurse to THEN expression. */
if (!foreign_expr_walker((Node *) cw->result,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
}
/* Recurse to ELSE expression. */
if (!foreign_expr_walker((Node *) ce->defresult,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/*
@@ -822,31 +884,6 @@ foreign_expr_walker(Node *node,
state = FDW_COLLATE_UNSAFE;
}
break;
- case T_CaseTestExpr:
- {
- CaseTestExpr *c = (CaseTestExpr *) node;
-
- /* Punt if we seem not to be inside a CASE arg WHEN. */
- if (!case_arg_cxt)
- return false;
-
- /*
- * Otherwise, any nondefault collation attached to the
- * CaseTestExpr node must be derived from foreign Var(s) in
- * the CASE arg.
- */
- collation = c->collation;
- if (collation == InvalidOid)
- state = FDW_COLLATE_NONE;
- else if (case_arg_cxt->state == FDW_COLLATE_SAFE &&
- collation == case_arg_cxt->collation)
- state = FDW_COLLATE_SAFE;
- else if (collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
case T_ArrayExpr:
{
ArrayExpr *a = (ArrayExpr *) node;
@@ -855,7 +892,7 @@ foreign_expr_walker(Node *node,
* Recurse to input subexpressions.
*/
if (!foreign_expr_walker((Node *) a->elements,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/*
@@ -885,7 +922,7 @@ foreign_expr_walker(Node *node,
foreach(lc, l)
{
if (!foreign_expr_walker((Node *) lfirst(lc),
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
}
@@ -935,7 +972,7 @@ foreign_expr_walker(Node *node,
}
if (!foreign_expr_walker(n,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
}
@@ -968,7 +1005,7 @@ foreign_expr_walker(Node *node,
/* Check aggregate filter */
if (!foreign_expr_walker((Node *) agg->aggfilter,
- glob_cxt, &inner_cxt, case_arg_cxt))
+ glob_cxt, &inner_cxt, param_cxt))
return false;
/*
@@ -3601,7 +3638,7 @@ deparseCaseExpr(CaseExpr *node, deparse_expr_cxt *context)
deparseExpr(whenclause->expr, context);
else /* CASE arg WHEN */
{
- /* Ignore the CaseTestExpr and equality operator. */
+ /* Ignore the Param and equality operator. */
deparseExpr(lsecond(castNode(OpExpr, whenclause->expr)->args),
context);
}
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 8f28da4bf9..2a66363502 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1041,6 +1041,12 @@ ExecInitExprRec(Expr *node, ExprState *state,
scratch.d.param.paramtype = param->paramtype;
ExprEvalPushStep(state, &scratch);
break;
+ case PARAM_EXPR:
+ scratch.opcode = EEOP_PARAM_EXPR;
+ scratch.d.param.paramid = param->paramid;
+ scratch.d.param.paramtype = param->paramtype;
+ ExprEvalPushStep(state, &scratch);
+ break;
case PARAM_EXTERN:
/*
@@ -1771,23 +1777,17 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
CaseExpr *caseExpr = (CaseExpr *) node;
List *adjust_jumps = NIL;
- Datum *caseval = NULL;
- bool *casenull = NULL;
ListCell *lc;
/*
* If there's a test expression, we have to evaluate it and
- * save the value where the CaseTestExpr placeholders can find
- * it.
+ * save the value where the PARAM_EXPR Params can find it.
*/
if (caseExpr->arg != NULL)
{
- /* Evaluate testexpr into caseval/casenull workspace */
- caseval = palloc(sizeof(Datum));
- casenull = palloc(sizeof(bool));
-
+ /* Evaluate testexpr into CASE's result variables */
ExecInitExprRec(caseExpr->arg, state,
- caseval, casenull);
+ resv, resnull);
/*
* Since value might be read multiple times, force to R/O
@@ -1795,17 +1795,20 @@ ExecInitExprRec(Expr *node, ExprState *state,
*/
if (get_typlen(exprType((Node *) caseExpr->arg)) == -1)
{
- /* change caseval in-place */
+ /* change testexpr value in-place */
scratch.opcode = EEOP_MAKE_READONLY;
- scratch.resvalue = caseval;
- scratch.resnull = casenull;
- scratch.d.make_readonly.value = caseval;
- scratch.d.make_readonly.isnull = casenull;
+ scratch.d.make_readonly.value = resv;
+ scratch.d.make_readonly.isnull = resnull;
ExprEvalPushStep(state, &scratch);
- /* restore normal settings of scratch fields */
- scratch.resvalue = resv;
- scratch.resnull = resnull;
}
+
+ /* Now copy value into the appropriate Param slot */
+ Assert(caseExpr->caseparam > 0);
+ scratch.opcode = EEOP_PARAM_SET_EXPR;
+ scratch.d.paramset.paramid = caseExpr->caseparam;
+ scratch.d.paramset.setvalue = resv;
+ scratch.d.paramset.setnull = resnull;
+ ExprEvalPushStep(state, &scratch);
}
/*
@@ -1822,19 +1825,13 @@ ExecInitExprRec(Expr *node, ExprState *state,
int whenstep;
/*
- * Make testexpr result available to CaseTestExpr nodes
- * within the condition. We must save and restore prior
- * setting of innermost_caseval fields, in case this node
- * is itself within a larger CASE.
- *
- * If there's no test expression, we don't actually need
- * to save and restore these fields; but it's less code to
- * just do so unconditionally.
+ * XXX innermost_caseval/casenull will go away, but for
+ * now just make them NULL.
*/
save_innermost_caseval = state->innermost_caseval;
save_innermost_casenull = state->innermost_casenull;
- state->innermost_caseval = caseval;
- state->innermost_casenull = casenull;
+ state->innermost_caseval = NULL;
+ state->innermost_casenull = NULL;
/* evaluate condition into CASE's result variables */
ExecInitExprRec(when->expr, state, resv, resnull);
@@ -2836,7 +2833,7 @@ ExecInitSubPlanExpr(SubPlan *subplan,
ExecInitExprRec(arg, state,
&state->resvalue, &state->resnull);
- scratch.opcode = EEOP_PARAM_SET;
+ scratch.opcode = EEOP_PARAM_SET_EXEC;
scratch.d.param.paramid = paramid;
/* paramtype's not actually used, but we might as well fill it */
scratch.d.param.paramtype = exprType((Node *) arg);
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 1127e6f11e..fa3e76eccd 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -520,9 +520,11 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
&&CASE_EEOP_BOOLTEST_IS_FALSE,
&&CASE_EEOP_BOOLTEST_IS_NOT_FALSE,
&&CASE_EEOP_PARAM_EXEC,
+ &&CASE_EEOP_PARAM_EXPR,
&&CASE_EEOP_PARAM_EXTERN,
&&CASE_EEOP_PARAM_CALLBACK,
- &&CASE_EEOP_PARAM_SET,
+ &&CASE_EEOP_PARAM_SET_EXEC,
+ &&CASE_EEOP_PARAM_SET_EXPR,
&&CASE_EEOP_CASE_TESTVAL,
&&CASE_EEOP_MAKE_READONLY,
&&CASE_EEOP_IOCOERCE,
@@ -1250,6 +1252,14 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
EEO_NEXT();
}
+ EEO_CASE(EEOP_PARAM_EXPR)
+ {
+ /* out of line implementation (XXX should make it inline) */
+ ExecEvalParamExpr(state, op, econtext);
+
+ EEO_NEXT();
+ }
+
EEO_CASE(EEOP_PARAM_EXTERN)
{
/* out of line implementation: too large */
@@ -1264,10 +1274,17 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
EEO_NEXT();
}
- EEO_CASE(EEOP_PARAM_SET)
+ EEO_CASE(EEOP_PARAM_SET_EXEC)
{
/* out of line, unlikely to matter performance-wise */
- ExecEvalParamSet(state, op, econtext);
+ ExecEvalParamSetExec(state, op, econtext);
+ EEO_NEXT();
+ }
+
+ EEO_CASE(EEOP_PARAM_SET_EXPR)
+ {
+ /* out of line implementation: too large */
+ ExecEvalParamSetExpr(state, op, econtext);
EEO_NEXT();
}
@@ -2975,6 +2992,7 @@ ExecEvalParamExec(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
{
ParamExecData *prm;
+ /* paramid is zero-based */
prm = &(econtext->ecxt_param_exec_vals[op->d.param.paramid]);
if (unlikely(prm->execPlan != NULL))
{
@@ -2987,6 +3005,24 @@ ExecEvalParamExec(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
*op->resnull = prm->isnull;
}
+/*
+ * Evaluate a PARAM_EXPR parameter.
+ *
+ * PARAM_EXPR params (internal executor parameters) are stored in the
+ * ecxt_param_expr_vals array, and can be accessed by array index.
+ */
+void
+ExecEvalParamExpr(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
+{
+ ParamExprData *prm;
+
+ /* paramid is one-based */
+ Assert(op->d.param.paramid <= econtext->ecxt_num_param_expr_vals);
+ prm = &(econtext->ecxt_param_expr_vals[op->d.param.paramid - 1]);
+ *op->resvalue = prm->value;
+ *op->resnull = prm->isnull;
+}
+
/*
* Evaluate a PARAM_EXTERN parameter.
*
@@ -3032,11 +3068,10 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
}
/*
- * Set value of a param (currently always PARAM_EXEC) from
- * state->res{value,null}.
+ * Set value of a PARAM_EXEC Param from state->res{value,null}.
*/
void
-ExecEvalParamSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
+ExecEvalParamSetExec(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
{
ParamExecData *prm;
@@ -3049,6 +3084,43 @@ ExecEvalParamSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
prm->isnull = state->resnull;
}
+/*
+ * Set value of a PARAM_EXPR Param from *op->paramset.set{value,null}.
+ */
+void
+ExecEvalParamSetExpr(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
+{
+ ParamExprData *prm;
+
+ /*
+ * In normal query execution, the ecxt_param_expr_vals array should be
+ * large enough already. However, in a standalone expression we may have
+ * to resize it larger, since there is no good place to initialize the
+ * array for that case.
+ */
+ if (unlikely(op->d.paramset.paramid > econtext->ecxt_num_param_expr_vals))
+ {
+ MemoryContext oldcontext;
+
+ /* Be sure to put it in the same context as econtext itself */
+ oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
+ if (econtext->ecxt_param_expr_vals == NULL)
+ econtext->ecxt_param_expr_vals =
+ palloc0_array(ParamExprData, op->d.paramset.paramid);
+ else
+ econtext->ecxt_param_expr_vals =
+ repalloc0_array(econtext->ecxt_param_expr_vals,
+ ParamExprData,
+ econtext->ecxt_num_param_expr_vals,
+ op->d.paramset.paramid);
+ econtext->ecxt_num_param_expr_vals = op->d.paramset.paramid;
+ MemoryContextSwitchTo(oldcontext);
+ }
+ prm = &(econtext->ecxt_param_expr_vals[op->d.paramset.paramid - 1]);
+ prm->value = *op->d.paramset.setvalue;
+ prm->isnull = *op->d.paramset.setnull;
+}
+
/*
* Evaluate a CoerceViaIO node in soft-error mode.
*
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index fb8dba3ab2..552a852e7e 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -185,10 +185,14 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
int nParamExec;
nParamExec = list_length(queryDesc->plannedstmt->paramExecTypes);
- estate->es_param_exec_vals = (ParamExecData *)
- palloc0(nParamExec * sizeof(ParamExecData));
+ estate->es_param_exec_vals = palloc0_array(ParamExecData, nParamExec);
}
+ estate->es_num_param_expr_vals = queryDesc->plannedstmt->numExprParams;
+ if (estate->es_num_param_expr_vals > 0)
+ estate->es_param_expr_vals =
+ palloc0_array(ParamExprData, estate->es_num_param_expr_vals);
+
/* We now require all callers to provide sourceText */
Assert(queryDesc->sourceText != NULL);
estate->es_sourceText = queryDesc->sourceText;
@@ -2875,9 +2879,10 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
/*
* The external param list is simply shared from parent. The internal
- * param workspace has to be local state, but we copy the initial values
- * from the parent, so as to have access to any param values that were
- * already set from other parts of the parent's plan tree.
+ * param workspaces have to be local state, but we copy the initial values
+ * for PARAM_EXEC Params from the parent, so as to have access to any
+ * param values that were already set from other parts of the parent's
+ * plan tree.
*/
rcestate->es_param_list_info = parentestate->es_param_list_info;
if (parentestate->es_plannedstmt->paramExecTypes != NIL)
@@ -2907,8 +2912,7 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
/* now make the internal param workspace ... */
i = list_length(parentestate->es_plannedstmt->paramExecTypes);
- rcestate->es_param_exec_vals = (ParamExecData *)
- palloc0(i * sizeof(ParamExecData));
+ rcestate->es_param_exec_vals = palloc0_array(ParamExecData, i);
/* ... and copy down all values, whether really needed or not */
while (--i >= 0)
{
@@ -2920,6 +2924,11 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
}
}
+ rcestate->es_num_param_expr_vals = parentestate->es_num_param_expr_vals;
+ if (rcestate->es_num_param_expr_vals > 0)
+ rcestate->es_param_expr_vals =
+ palloc0_array(ParamExprData, rcestate->es_num_param_expr_vals);
+
/*
* Initialize private state information for each SubPlan. We must do this
* before running ExecInitNode on the main query tree, since
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index ff4d9dd1bb..7335cafc0a 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -208,6 +208,7 @@ ExecSerializePlan(Plan *plan, EState *estate)
pstmt->relationOids = NIL;
pstmt->invalItems = NIL; /* workers can't replan anyway... */
pstmt->paramExecTypes = estate->es_plannedstmt->paramExecTypes;
+ pstmt->numExprParams = estate->es_plannedstmt->numExprParams;
pstmt->utilityStmt = NULL;
pstmt->stmt_location = -1;
pstmt->stmt_len = -1;
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 7c539de5cf..9a236c3789 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -133,6 +133,8 @@ CreateExecutorState(void)
estate->es_param_list_info = NULL;
estate->es_param_exec_vals = NULL;
+ estate->es_param_expr_vals = NULL;
+ estate->es_num_param_expr_vals = 0;
estate->es_queryEnv = NULL;
@@ -262,6 +264,8 @@ CreateExprContextInternal(EState *estate, Size minContextSize,
maxBlockSize);
econtext->ecxt_param_exec_vals = estate->es_param_exec_vals;
+ econtext->ecxt_param_expr_vals = estate->es_param_expr_vals;
+ econtext->ecxt_num_param_expr_vals = estate->es_num_param_expr_vals;
econtext->ecxt_param_list_info = estate->es_param_list_info;
econtext->ecxt_aggvalues = NULL;
@@ -377,6 +381,8 @@ CreateStandaloneExprContext(void)
ALLOCSET_DEFAULT_SIZES);
econtext->ecxt_param_exec_vals = NULL;
+ econtext->ecxt_param_expr_vals = NULL;
+ econtext->ecxt_num_param_expr_vals = 0;
econtext->ecxt_param_list_info = NULL;
econtext->ecxt_aggvalues = NULL;
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index c1cf34f103..1b59c30b6f 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1182,6 +1182,12 @@ llvm_compile_expr(ExprState *state)
LLVMBuildBr(b, opblocks[opno + 1]);
break;
+ case EEOP_PARAM_EXPR:
+ build_EvalXFunc(b, mod, "ExecEvalParamExpr",
+ v_state, op, v_econtext);
+ LLVMBuildBr(b, opblocks[opno + 1]);
+ break;
+
case EEOP_PARAM_EXTERN:
build_EvalXFunc(b, mod, "ExecEvalParamExtern",
v_state, op, v_econtext);
@@ -1208,8 +1214,14 @@ llvm_compile_expr(ExprState *state)
break;
}
- case EEOP_PARAM_SET:
- build_EvalXFunc(b, mod, "ExecEvalParamSet",
+ case EEOP_PARAM_SET_EXEC:
+ build_EvalXFunc(b, mod, "ExecEvalParamSetExec",
+ v_state, op, v_econtext);
+ LLVMBuildBr(b, opblocks[opno + 1]);
+ break;
+
+ case EEOP_PARAM_SET_EXPR:
+ build_EvalXFunc(b, mod, "ExecEvalParamSetExpr",
v_state, op, v_econtext);
LLVMBuildBr(b, opblocks[opno + 1]);
break;
diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index dbe0282e98..7c2705efa4 100644
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -157,8 +157,10 @@ void *referenced_functions[] =
ExecEvalMinMax,
ExecEvalNextValueExpr,
ExecEvalParamExec,
+ ExecEvalParamExpr,
ExecEvalParamExtern,
- ExecEvalParamSet,
+ ExecEvalParamSetExec,
+ ExecEvalParamSetExpr,
ExecEvalRow,
ExecEvalRowNotNull,
ExecEvalRowNull,
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 6803edd085..85a98a065d 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -328,6 +328,7 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
glob->relationOids = NIL;
glob->invalItems = NIL;
glob->paramExecTypes = NIL;
+ glob->numExprParams = 0;
glob->lastPHId = 0;
glob->lastRowMarkId = 0;
glob->lastPlanNodeId = 0;
@@ -565,6 +566,7 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
result->relationOids = glob->relationOids;
result->invalItems = glob->invalItems;
result->paramExecTypes = glob->paramExecTypes;
+ result->numExprParams = glob->numExprParams;
/* utilityStmt should be null, but we might as well copy it */
result->utilityStmt = parse->utilityStmt;
result->stmt_location = parse->stmt_location;
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 43dfecfb47..b1ffe3c4f7 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -62,6 +62,7 @@ typedef struct
PlannerInfo *root;
List *active_fns;
Node *case_val;
+ int numExprParams;
bool estimate;
} eval_const_expressions_context;
@@ -149,6 +150,9 @@ 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 Param *generate_new_expr_param(eval_const_expressions_context *context,
+ Oid paramtype, int32 paramtypmod,
+ Oid paramcollation);
static Query *substitute_actual_srf_parameters(Query *expr,
int nargs, List *args);
static Node *substitute_actual_srf_parameters_mutator(Node *node,
@@ -921,17 +925,25 @@ max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context)
}
/*
- * We can't pass Params to workers at the moment either, so they are also
- * parallel-restricted, unless they are PARAM_EXTERN Params or are
- * PARAM_EXEC Params listed in safe_param_ids, meaning they could be
- * either generated within workers or can be computed by the leader and
- * then their value can be passed to workers.
+ * Params are parallel-restricted except in these cases:
+ *
+ * PARAM_EXTERN Params are made available to workers by copying the
+ * executor's ParamListInfo to workers, so they're parallel-safe.
+ *
+ * PARAM_EXPR Params are assumed to be supplied from somewhere higher in
+ * the same expression. Since we don't parallelize at the subexpression
+ * level, these can be considered parallel-safe.
+ *
+ * PARAM_EXEC Params are safe if listed in safe_param_ids, meaning they
+ * could be either generated within workers or can be computed by the
+ * leader and then their value can be passed to workers.
*/
else if (IsA(node, Param))
{
Param *param = (Param *) node;
- if (param->paramkind == PARAM_EXTERN)
+ if (param->paramkind == PARAM_EXTERN ||
+ param->paramkind == PARAM_EXPR)
return false;
if (param->paramkind != PARAM_EXEC ||
@@ -2236,6 +2248,15 @@ rowtype_field_matches(Oid rowtypeid, int fieldnum,
* root->glob->invalItems. This ensures the plan is known to depend on
* such functions, even though they aren't referenced anymore.
*
+ * This function also performs certain essential pre-processing on
+ * expression trees, including insertion of function default arguments,
+ * conversion of named-argument calls to positional notation, and
+ * replacement of CaseTestExpr nodes with suitable Params. Because of
+ * that, this *must* be invoked on all expressions, even when there
+ * might not be much value in the constant-folding aspect. It'd be
+ * cleaner to do those things separately, but the cost of an additional
+ * mutation pass seems unattractive.
+ *
* We assume that the tree has already been type-checked and contains
* only operators and functions that are reasonable to try to execute.
*
@@ -2244,10 +2265,6 @@ rowtype_field_matches(Oid rowtypeid, int fieldnum,
*
* NOTE: the planner assumes that this will always flatten nested AND and
* OR clauses into N-argument form. See comments in prepqual.c.
- *
- * NOTE: another critical effect is that any function calls that require
- * default arguments will be expanded, and named-argument calls will be
- * converted to positional notation. The executor won't handle either.
*--------------------
*/
Node *
@@ -2262,6 +2279,7 @@ eval_const_expressions(PlannerInfo *root, Node *node)
context.root = root; /* for inlined-function dependencies */
context.active_fns = NIL; /* nothing being recursively simplified */
context.case_val = NULL; /* no CASE being examined */
+ context.numExprParams = 0; /* used only if no root */
context.estimate = false; /* safe transformations only */
return eval_const_expressions_mutator(node, &context);
}
@@ -2389,6 +2407,10 @@ convert_saop_to_hashed_saop_walker(Node *node, void *context)
* value of the Param.
* 2. Fold stable, as well as immutable, functions to constants.
* 3. Reduce PlaceHolderVar nodes to their contained expressions.
+ *
+ * A step that is *not* taken in this mode is replacement of CaseTestExpr
+ * nodes with Params. Doing so would just result in useless consumption
+ * of PARAM_EXPR slots, since the result will never be executed.
*--------------------
*/
Node *
@@ -2401,6 +2423,7 @@ estimate_expression_value(PlannerInfo *root, Node *node)
context.root = NULL;
context.active_fns = NIL; /* nothing being recursively simplified */
context.case_val = NULL; /* no CASE being examined */
+ context.numExprParams = 0; /* not used in this path */
context.estimate = true; /* unsafe transformations OK */
return eval_const_expressions_mutator(node, &context);
}
@@ -3146,6 +3169,13 @@ eval_const_expressions_mutator(Node *node,
* expression when executing the CASE, since any contained
* CaseTestExprs that might have referred to it will have been
* replaced by the constant.
+ *
+ * If the test expression doesn't reduce to a constant, then
+ * contained CaseTestExpr placeholders need to be replaced
+ * with PARAM_EXPR Params instead. This ensures they will
+ * continue to be correctly associated with this CaseExpr
+ * even in the face of subsequent slicing-and-dicing of the
+ * expression tree.
*----------
*/
CaseExpr *caseexpr = (CaseExpr *) node;
@@ -3155,19 +3185,54 @@ eval_const_expressions_mutator(Node *node,
List *newargs;
bool const_true_cond;
Node *defresult = NULL;
+ int case_param = 0;
ListCell *arg;
/* Simplify the test expression, if any */
newarg = eval_const_expressions_mutator((Node *) caseexpr->arg,
context);
- /* Set up for contained CaseTestExpr nodes */
+ /*
+ * Set up the node to substitute for contained CaseTestExpr
+ * nodes. If we reduced the arg to a Const we can always
+ * replace CaseTestExprs with that. Otherwise, generate a
+ * PARAM_EXPR Param; or if we are just estimating, leave the
+ * CaseTestExprs alone.
+ *
+ * We must save and restore context->case_val so as not to
+ * affect surrounding constructs.
+ */
save_case_val = context->case_val;
- if (newarg && IsA(newarg, Const))
+
+ if (caseexpr->caseparam > 0)
+ {
+ /*
+ * We get here if asked to re-simplify an
+ * already-simplified CASE. We must not generate a new
+ * Param, or we'd be out of sync with the already-replaced
+ * CaseTestExprs.
+ */
+ case_param = caseexpr->caseparam;
+ context->case_val = NULL; /* no CaseTestExprs expected */
+ }
+ else if (newarg == NULL)
+ context->case_val = NULL; /* no CaseTestExprs expected */
+ else if (IsA(newarg, Const))
{
context->case_val = newarg;
newarg = NULL; /* not needed anymore, see above */
}
+ else if (!context->estimate)
+ {
+ Param *p;
+
+ p = generate_new_expr_param(context,
+ exprType(newarg),
+ exprTypmod(newarg),
+ exprCollation(newarg));
+ context->case_val = (Node *) p;
+ case_param = p->paramid;
+ }
else
context->case_val = NULL;
@@ -3245,15 +3310,25 @@ eval_const_expressions_mutator(Node *node,
newcase->arg = (Expr *) newarg;
newcase->args = newargs;
newcase->defresult = (Expr *) defresult;
+ newcase->caseparam = case_param;
newcase->location = caseexpr->location;
return (Node *) newcase;
}
case T_CaseTestExpr:
{
/*
- * If we know a constant test value for the current CASE
+ * If we have a replacement node for the current CASE
* construct, substitute it for the placeholder. Else just
* return the placeholder as-is.
+ *
+ * Note: this behavior is subtler than it looks. We can rely
+ * on context->case_val to be from the matching CASE construct
+ * because we are dealing with an expression as it was created
+ * by the parser. Subsequent processing, such as inlining of
+ * SQL-language functions, might result in insertion of CASE
+ * or other CaseTestExpr-abusing constructs between here and
+ * there --- but it will no longer matter once we've replaced
+ * the CaseTestExpr with a numbered Param.
*/
if (context->case_val)
return copyObject(context->case_val);
@@ -4928,6 +5003,7 @@ substitute_actual_parameters_mutator(Node *node,
{
Param *param = (Param *) node;
+ /* Only EXTERN Params should appear in the tree as yet */
if (param->paramkind != PARAM_EXTERN)
elog(ERROR, "unexpected paramkind: %d", (int) param->paramkind);
if (param->paramid <= 0 || param->paramid > context->nargs)
@@ -4964,6 +5040,48 @@ sql_inline_error_callback(void *arg)
errcontext("SQL function \"%s\" during inlining", callback_arg->proname);
}
+/*
+ * Generate a new PARAM_EXPR Param node that will not conflict with any other.
+ *
+ * Possibly this should be in paramassign.c, but then it couldn't rely on
+ * struct eval_const_expressions_context, which would make for a much more
+ * awkward API. Currently there seems no need to allocate PARAM_EXPR Params
+ * anywhere except within eval_const_expressions.
+ */
+static Param *
+generate_new_expr_param(eval_const_expressions_context *context,
+ Oid paramtype, int32 paramtypmod, Oid paramcollation)
+{
+ Param *retval;
+ int paramid;
+
+ /*
+ * If we're doing normal planning, use root->glob->numExprParams to count
+ * PARAM_EXPR Params, so that we allocate them uniquely across the entire
+ * plan tree. This is extremely conservative, as in most cases Params in
+ * different subexpressions won't have overlapping lifetimes and could
+ * share a ParamExprData slot. But the slots are small, so trying to
+ * re-use them seems more dangerous than it's worth.
+ *
+ * When processing a standalone expression, use context->numExprParams,
+ * meaning that the assignments are unique only within the expression.
+ */
+ if (context->root)
+ paramid = ++context->root->glob->numExprParams;
+ else
+ paramid = ++context->numExprParams;
+
+ retval = makeNode(Param);
+ retval->paramkind = PARAM_EXPR;
+ retval->paramid = paramid;
+ retval->paramtype = paramtype;
+ retval->paramtypmod = paramtypmod;
+ retval->paramcollid = paramcollation;
+ retval->location = -1;
+
+ return retval;
+}
+
/*
* evaluate_expr: pre-evaluate a constant expression
*
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index bad1df732e..a17b8a5797 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1667,6 +1667,11 @@ transformCaseExpr(ParseState *pstate, CaseExpr *c)
placeholder->typeId = exprType(arg);
placeholder->typeMod = exprTypmod(arg);
placeholder->collation = exprCollation(arg);
+
+ /*
+ * Note: the planner will replace the CaseTestExpr(s) with PARAM_EXPR
+ * Params, and will fill the CaseExpr's caseparam field at that time.
+ */
}
else
placeholder = NULL;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 54dad97555..59a4cde467 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8744,10 +8744,11 @@ get_parameter(Param *param, deparse_context *context)
/*
* Not PARAM_EXEC, or couldn't find referent: just print $N.
*
- * It's a bug if we get here for anything except PARAM_EXTERN Params, but
- * in production builds printing $N seems more useful than failing.
+ * It's a bug if we get here for anything except PARAM_EXTERN or
+ * PARAM_EXPR Params, but in production builds printing $N seems more
+ * useful than failing.
*/
- Assert(param->paramkind == PARAM_EXTERN);
+ Assert(param->paramkind == PARAM_EXTERN || param->paramkind == PARAM_EXPR);
appendStringInfo(context->buf, "$%d", param->paramid);
}
@@ -9748,21 +9749,30 @@ get_rule_expr(Node *node, deparse_context *context,
* form "CaseTestExpr = RHS", possibly with an
* implicit coercion inserted above the CaseTestExpr.
* For accurate decompilation of rules it's essential
- * that we show just the RHS. However in an
- * expression that's been through the optimizer, the
- * WHEN clause could be almost anything (since the
- * equality operator could have been expanded into an
- * inline function). If we don't recognize the form
- * of the WHEN clause, just punt and display it as-is.
+ * that we show just the RHS. In an expression that's
+ * been through the optimizer, the WHEN clause would
+ * typically look like "PARAM_EXPR Param = RHS", and
+ * we prefer to show just the RHS. However, an
+ * optimized WHEN clause could be almost anything
+ * (since the equality operator could have been
+ * expanded into an inline function). If we don't
+ * recognize the form of the WHEN clause, just punt
+ * and display it as-is.
*/
if (IsA(w, OpExpr))
{
List *args = ((OpExpr *) w)->args;
- if (list_length(args) == 2 &&
- IsA(strip_implicit_coercions(linitial(args)),
- CaseTestExpr))
- w = (Node *) lsecond(args);
+ if (list_length(args) == 2)
+ {
+ Node *larg = strip_implicit_coercions(linitial(args));
+
+ if (IsA(larg, CaseTestExpr) ||
+ (IsA(larg, Param) &&
+ ((Param *) larg)->paramkind == PARAM_EXPR &&
+ ((Param *) larg)->paramid == caseexpr->caseparam))
+ w = (Node *) lsecond(args);
+ }
}
}
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 5371e344ec..e03045de92 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -164,12 +164,15 @@ typedef enum ExprEvalOp
EEOP_BOOLTEST_IS_FALSE,
EEOP_BOOLTEST_IS_NOT_FALSE,
- /* evaluate PARAM_EXEC/EXTERN parameters */
+ /* evaluate PARAM_EXEC/EXPR/EXTERN parameters */
EEOP_PARAM_EXEC,
+ EEOP_PARAM_EXPR,
EEOP_PARAM_EXTERN,
EEOP_PARAM_CALLBACK,
- /* set PARAM_EXEC value */
- EEOP_PARAM_SET,
+
+ /* set PARAM_EXEC/EXPR values */
+ EEOP_PARAM_SET_EXEC,
+ EEOP_PARAM_SET_EXPR,
/* return CaseTestExpr value */
EEOP_CASE_TESTVAL,
@@ -411,13 +414,21 @@ typedef struct ExprEvalStep
ExprEvalRowtypeCache rowcache;
} nulltest_row;
- /* for EEOP_PARAM_EXEC/EXTERN and EEOP_PARAM_SET */
+ /* for EEOP_PARAM_EXEC/EXPR/EXTERN and EEOP_PARAM_SET_EXEC */
struct
{
int paramid; /* numeric ID for parameter */
Oid paramtype; /* OID of parameter's datatype */
} param;
+ /* for EEOP_PARAM_SET_EXPR */
+ struct
+ {
+ int paramid; /* numeric ID for parameter */
+ Datum *setvalue; /* source of value */
+ bool *setnull;
+ } paramset;
+
/* for EEOP_PARAM_CALLBACK */
struct
{
@@ -845,10 +856,14 @@ extern void ExecEvalFuncExprStrictFusage(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
-extern void ExecEvalParamSet(ExprState *state, ExprEvalStep *op,
- ExprContext *econtext);
+extern void ExecEvalParamExpr(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext);
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
+extern void ExecEvalParamSetExec(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext);
+extern void ExecEvalParamSetExpr(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext);
extern void ExecEvalCoerceViaIOSafe(ExprState *state, ExprEvalStep *op);
extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op);
extern void ExecEvalCurrentOfExpr(ExprState *state, ExprEvalStep *op);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d0f2dca592..366534c0d0 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -276,33 +276,35 @@ typedef struct ExprContext
/* Values to substitute for Param nodes in expression */
ParamExecData *ecxt_param_exec_vals; /* for PARAM_EXEC params */
+ ParamExprData *ecxt_param_expr_vals; /* for PARAM_EXPR params */
+ int ecxt_num_param_expr_vals;
ParamListInfo ecxt_param_list_info; /* for other param types */
/*
* Values to substitute for Aggref nodes in the expressions of an Agg
* node, or for WindowFunc nodes within a WindowAgg node.
*/
-#define FIELDNO_EXPRCONTEXT_AGGVALUES 8
+#define FIELDNO_EXPRCONTEXT_AGGVALUES 10
Datum *ecxt_aggvalues; /* precomputed values for aggs/windowfuncs */
-#define FIELDNO_EXPRCONTEXT_AGGNULLS 9
+#define FIELDNO_EXPRCONTEXT_AGGNULLS 11
bool *ecxt_aggnulls; /* null flags for aggs/windowfuncs */
/* Value to substitute for CaseTestExpr nodes in expression */
-#define FIELDNO_EXPRCONTEXT_CASEDATUM 10
+#define FIELDNO_EXPRCONTEXT_CASEDATUM 12
Datum caseValue_datum;
-#define FIELDNO_EXPRCONTEXT_CASENULL 11
+#define FIELDNO_EXPRCONTEXT_CASENULL 13
bool caseValue_isNull;
/* Value to substitute for CoerceToDomainValue nodes in expression */
-#define FIELDNO_EXPRCONTEXT_DOMAINDATUM 12
+#define FIELDNO_EXPRCONTEXT_DOMAINDATUM 14
Datum domainValue_datum;
-#define FIELDNO_EXPRCONTEXT_DOMAINNULL 13
+#define FIELDNO_EXPRCONTEXT_DOMAINNULL 15
bool domainValue_isNull;
/* Tuples that OLD/NEW Var nodes in RETURNING may refer to */
-#define FIELDNO_EXPRCONTEXT_OLDTUPLE 14
+#define FIELDNO_EXPRCONTEXT_OLDTUPLE 16
TupleTableSlot *ecxt_oldtuple;
-#define FIELDNO_EXPRCONTEXT_NEWTUPLE 15
+#define FIELDNO_EXPRCONTEXT_NEWTUPLE 17
TupleTableSlot *ecxt_newtuple;
/* Link to containing EState (NULL if a standalone ExprContext) */
@@ -684,6 +686,8 @@ typedef struct EState
/* Parameter info: */
ParamListInfo es_param_list_info; /* values of external params */
ParamExecData *es_param_exec_vals; /* values of internal params */
+ ParamExprData *es_param_expr_vals; /* values of internal params */
+ int es_num_param_expr_vals;
QueryEnvironment *es_queryEnv; /* query environment */
diff --git a/src/include/nodes/params.h b/src/include/nodes/params.h
index 4321ca6329..5c43a83f44 100644
--- a/src/include/nodes/params.h
+++ b/src/include/nodes/params.h
@@ -19,6 +19,7 @@ struct Bitmapset;
struct ExprState;
struct Param;
struct ParseState;
+struct SubPlanState;
/*
@@ -129,8 +130,8 @@ typedef struct ParamListInfoData
/* ----------------
* ParamExecData
*
- * ParamExecData entries are used for executor internal parameters
- * (that is, values being passed into or out of a sub-query). The
+ * ParamExecData entries are used for executor internal PARAM_EXEC
+ * parameters (values being passed between plan nodes). The
* paramid of a PARAM_EXEC Param is a (zero-based) index into an
* array of ParamExecData records, which is referenced through
* es_param_exec_vals or ecxt_param_exec_vals.
@@ -145,11 +146,31 @@ typedef struct ParamListInfoData
typedef struct ParamExecData
{
- void *execPlan; /* should be "SubPlanState *" */
+ struct SubPlanState *execPlan;
Datum value;
bool isnull;
} ParamExecData;
+
+/* ----------------
+ * ParamExprData
+ *
+ * ParamExprData entries are used for executor internal PARAM_EXPR
+ * parameters (values being passed within an expression). The
+ * paramid of a PARAM_EXPR Param is a (one-based) index into an
+ * array of ParamExprData records, which is referenced through
+ * es_param_expr_vals or ecxt_param_expr_vals. Values are assumed
+ * to be valid when needed.
+ * ----------------
+ */
+
+typedef struct ParamExprData
+{
+ Datum value;
+ bool isnull;
+} ParamExprData;
+
+
/* type of argument for ParamsErrorCallback */
typedef struct ParamsErrorCbData
{
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 54ee17697e..5ce7137efc 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -137,6 +137,9 @@ typedef struct PlannerGlobal
/* type OIDs for PARAM_EXEC Params */
List *paramExecTypes;
+ /* count of PARAM_EXPR Params (we don't need anything but a count) */
+ int numExprParams;
+
/* highest PlaceHolderVar ID assigned */
Index lastPHId;
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 6ef5d0b375..79e64b79ee 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -92,6 +92,8 @@ typedef struct PlannedStmt
List *paramExecTypes; /* type OIDs for PARAM_EXEC Params */
+ int numExprParams; /* count of PARAM_EXPR Params */
+
Node *utilityStmt; /* non-null if this is utility stmt */
/* statement location in source string (copied from Query) */
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 59e7bb26bb..af55661df9 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -361,11 +361,22 @@ typedef struct Const
* Such parameters are numbered from 1 to n.
*
* PARAM_EXEC: The parameter is an internal executor parameter, used
- * for passing values into and out of sub-queries or from
- * nestloop joins to their inner scans.
+ * for passing values between plan nodes (for example, values
+ * passed into and out of sub-queries or from nestloop joins to
+ * their inner scans).
* For historical reasons, such parameters are numbered from 0.
* These numbers are independent of PARAM_EXTERN numbers.
*
+ * PARAM_EXPR: The parameter is an internal executor parameter, used
+ * for passing values between parts of a single expression
+ * (for example, from CaseExpr into its test expressions).
+ * We distinguish these from PARAM_EXEC so that they do not
+ * affect decisions about inter-plan-node dependencies or
+ * parallel safety.
+ * Such parameters are numbered from 1 to n.
+ * These numbers are independent of PARAM_EXTERN and PARAM_EXEC
+ * numbers.
+ *
* PARAM_SUBLINK: The parameter represents an output column of a SubLink
* node's sub-select. The column number is contained in the
* `paramid' field. (This type of Param is converted to
@@ -382,6 +393,7 @@ typedef enum ParamKind
{
PARAM_EXTERN,
PARAM_EXEC,
+ PARAM_EXPR,
PARAM_SUBLINK,
PARAM_MULTIEXPR,
} ParamKind;
@@ -1310,11 +1322,17 @@ typedef struct CollateExpr
* of the WHEN clauses are just the comparison values. Parse analysis
* converts these to valid boolean expressions of the form
* CaseTestExpr '=' compexpr
- * where the CaseTestExpr node is a placeholder that emits the correct
+ * where the CaseTestExpr node is a placeholder that will emit the correct
* value at runtime. This structure is used so that the testexpr need be
* evaluated only once. Note that after parse analysis, the condition
* expressions always yield boolean.
*
+ * The planner will replace CaseTestExprs with PARAM_EXPR Params to ensure
+ * that different CASE expressions do not interfere with each other.
+ * Ideally we'd generate that representation in parse analysis and not use
+ * CaseTestExpr at all, but that would complicate rewriting, subquery pullup,
+ * and so forth unduly.
+ *
* Note: we can test whether a CaseExpr has been through parse analysis
* yet by checking whether casetype is InvalidOid or not.
*----------
@@ -1329,6 +1347,8 @@ typedef struct CaseExpr
Expr *arg; /* implicit equality comparison argument */
List *args; /* the arguments (list of WHEN clauses) */
Expr *defresult; /* the default result (ELSE clause) */
+ /* ID of PARAM_EXPR Param representing arg, if assigned; else 0 */
+ int caseparam pg_node_attr(equal_ignore, query_jumble_ignore);
ParseLoc location; /* token location, or -1 if unknown */
} CaseExpr;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index a2644a2e65..5edcd31c2c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2022,6 +2022,7 @@ ParallelWorkerInfo
Param
ParamCompileHook
ParamExecData
+ParamExprData
ParamExternData
ParamFetchHook
ParamKind
@@ -3516,6 +3517,7 @@ fmStringInfo
fmgr_hook_type
foreign_glob_cxt
foreign_loc_cxt
+foreign_param_cxt
freefunc
fsec_t
gbt_vsrt_arg
--
2.43.5
From 5a01fc722d63eeff63f764349c24c45aceb8590e Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
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
From 002ee98962b5f89227e1fa4751a146a188e557dd Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 29 Jan 2025 18:35:36 -0500
Subject: [PATCH v1 3/8] Remove contain_context_dependent_node
function-inlining restriction.
Now that CaseExpr and ArrayCoerceExpr convert their CaseTestExpr
sub-nodes to Params, the sort of confusion that this test meant to
prevent is impossible, and indeed the search function cannot return
true anymore (except perhaps in estimation mode, where it won't
matter). Let's remove the useless and optimization-defeating code.
Some of the regression tests added by commits f0c7b789a and 14a158f9b
are now dead code, in that the errors they intend to test for are
now impossible by design. I left them there for now because the
fact that they still pass is good validation for this patch series.
I'm wondering whether to remove them in the final version though,
because it's hard to justify expending test cycles forever on dead
issues.
---
src/backend/optimizer/util/clauses.c | 100 ---------------------------
1 file changed, 100 deletions(-)
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 4edbbabf59..8aa7ae8cac 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -103,8 +103,6 @@ static bool max_parallel_hazard_walker(Node *node,
max_parallel_hazard_context *context);
static bool contain_nonstrict_functions_walker(Node *node, void *context);
static bool contain_exec_param_walker(Node *node, List *param_ids);
-static bool contain_context_dependent_node(Node *clause);
-static bool contain_context_dependent_node_walker(Node *node, int *flags);
static bool contain_leaked_vars_walker(Node *node, void *context);
static Relids find_nonnullable_rels_walker(Node *node, bool top_level);
static List *find_nonnullable_vars_walker(Node *node, bool top_level);
@@ -1167,95 +1165,6 @@ contain_exec_param_walker(Node *node, List *param_ids)
return expression_tree_walker(node, contain_exec_param_walker, param_ids);
}
-/*****************************************************************************
- * Check clauses for context-dependent nodes
- *****************************************************************************/
-
-/*
- * contain_context_dependent_node
- * Recursively search for context-dependent nodes within a clause.
- *
- * CaseTestExpr nodes must appear directly within the corresponding CaseExpr,
- * not nested within another one, or they'll see the wrong test value. If one
- * appears "bare" in the arguments of a SQL function, then we can't inline the
- * SQL function for fear of creating such a situation. The same applies for
- * CaseTestExpr used within the elemexpr of an ArrayCoerceExpr.
- *
- * CoerceToDomainValue would have the same issue if domain CHECK expressions
- * could get inlined into larger expressions, but presently that's impossible.
- * Still, it might be allowed in future, or other node types with similar
- * issues might get invented. So give this function a generic name, and set
- * up the recursion state to allow multiple flag bits.
- */
-static bool
-contain_context_dependent_node(Node *clause)
-{
- int flags = 0;
-
- return contain_context_dependent_node_walker(clause, &flags);
-}
-
-#define CCDN_CASETESTEXPR_OK 0x0001 /* CaseTestExpr okay here? */
-
-static bool
-contain_context_dependent_node_walker(Node *node, int *flags)
-{
- if (node == NULL)
- return false;
- if (IsA(node, CaseTestExpr))
- return !(*flags & CCDN_CASETESTEXPR_OK);
- else if (IsA(node, CaseExpr))
- {
- CaseExpr *caseexpr = (CaseExpr *) node;
-
- /*
- * If this CASE doesn't have a test expression, then it doesn't create
- * a context in which CaseTestExprs should appear, so just fall
- * through and treat it as a generic expression node.
- */
- if (caseexpr->arg)
- {
- int save_flags = *flags;
- bool res;
-
- /*
- * Note: in principle, we could distinguish the various sub-parts
- * of a CASE construct and set the flag bit only for some of them,
- * since we are only expecting CaseTestExprs to appear in the
- * "expr" subtree of the CaseWhen nodes. But it doesn't really
- * seem worth any extra code. If there are any bare CaseTestExprs
- * elsewhere in the CASE, something's wrong already.
- */
- *flags |= CCDN_CASETESTEXPR_OK;
- res = expression_tree_walker(node,
- contain_context_dependent_node_walker,
- flags);
- *flags = save_flags;
- return res;
- }
- }
- else if (IsA(node, ArrayCoerceExpr))
- {
- ArrayCoerceExpr *ac = (ArrayCoerceExpr *) node;
- int save_flags;
- bool res;
-
- /* Check the array expression */
- if (contain_context_dependent_node_walker((Node *) ac->arg, flags))
- return true;
-
- /* Check the elemexpr, which is allowed to contain CaseTestExpr */
- save_flags = *flags;
- *flags |= CCDN_CASETESTEXPR_OK;
- res = contain_context_dependent_node_walker((Node *) ac->elemexpr,
- flags);
- *flags = save_flags;
- return res;
- }
- return expression_tree_walker(node, contain_context_dependent_node_walker,
- flags);
-}
-
/*****************************************************************************
* Check clauses for Vars passed to non-leakproof functions
*****************************************************************************/
@@ -4658,8 +4567,6 @@ evaluate_function(Oid funcid, Oid result_type, int32 result_typmod,
* doesn't work in the general case because it discards information such
* as OUT-parameter declarations.
*
- * Also, context-dependent expression nodes in the argument list are trouble.
- *
* Returns a simplified expression if successful, or NULL if cannot
* simplify the function.
*/
@@ -4899,13 +4806,6 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
contain_nonstrict_functions(newexpr))
goto fail;
- /*
- * If any parameter expression contains a context-dependent node, we can't
- * inline, for fear of putting such a node into the wrong context.
- */
- if (contain_context_dependent_node((Node *) args))
- goto fail;
-
/*
* We may be able to do it; there are still checks on parameter usage to
* make, but those are most easily done in combination with the actual
--
2.43.5
From f8ab9db37ee139cfe5b875370d90398a459fc769 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 29 Jan 2025 20:12:05 -0500
Subject: [PATCH v1 4/8] Convert JsonConstructorExpr & JsonExpr to use a Param.
More crank-turning.
---
src/backend/executor/execExpr.c | 34 ++++--
src/backend/optimizer/util/clauses.c | 143 ++++++++++++++++++++++++++
src/backend/parser/parse_expr.c | 7 +-
src/backend/parser/parse_jsontable.c | 11 ++
src/backend/utils/adt/jsonpath_exec.c | 12 +--
src/include/nodes/primnodes.h | 10 ++
6 files changed, 199 insertions(+), 18 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 13185652ec..80709f14de 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2465,16 +2465,20 @@ ExecInitExprRec(Expr *node, ExprState *state,
if (ctor->coercion)
{
- Datum *innermost_caseval = state->innermost_caseval;
- bool *innermost_isnull = state->innermost_casenull;
-
- state->innermost_caseval = resv;
- state->innermost_casenull = resnull;
+ /*
+ * Apply the coercion expression to the result-so-far. We
+ * assume it has a PARAM_EXPR Param representing the input
+ * value. Push a step that copies the current result into
+ * the appropriate Param slot.
+ */
+ scratch.opcode = EEOP_PARAM_SET_EXPR;
+ Assert(ctor->coparam > 0);
+ scratch.d.paramset.paramid = ctor->coparam;
+ scratch.d.paramset.setvalue = resv;
+ scratch.d.paramset.setnull = resnull;
+ ExprEvalPushStep(state, &scratch);
ExecInitExprRec(ctor->coercion, state, resv, resnull);
-
- state->innermost_caseval = innermost_caseval;
- state->innermost_casenull = innermost_isnull;
}
}
break;
@@ -2496,6 +2500,20 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
JsonExpr *jsexpr = castNode(JsonExpr, node);
+ /*
+ * If the expression involves a Param, the caller will have
+ * loaded the source value into resv/resnull. Push a step
+ * that copies that into the appropriate Param slot.
+ */
+ if (jsexpr->feparam > 0)
+ {
+ scratch.opcode = EEOP_PARAM_SET_EXPR;
+ scratch.d.paramset.paramid = jsexpr->feparam;
+ scratch.d.paramset.setvalue = resv;
+ scratch.d.paramset.setnull = resnull;
+ ExprEvalPushStep(state, &scratch);
+ }
+
/*
* No need to initialize a full JsonExprState For
* JSON_TABLE(), because the upstream caller tfuncFetchRows()
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 8aa7ae8cac..d638a290ec 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -3734,6 +3734,149 @@ eval_const_expressions_mutator(Node *node,
return ece_evaluate_expr((Node *) newcre);
return (Node *) newcre;
}
+ case T_JsonConstructorExpr:
+ {
+ JsonConstructorExpr *jce = makeNode(JsonConstructorExpr);
+ CaseTestExpr *ctexpr = NULL;
+
+ /*
+ * Copy the node and const-simplify its arguments. We can't
+ * use ece_generic_processing() here because we need to mess
+ * with case_val only while processing the coercion expr.
+ */
+ memcpy(jce, node, sizeof(JsonConstructorExpr));
+ jce->args = (List *)
+ eval_const_expressions_mutator((Node *) jce->args,
+ context);
+ jce->func = (Expr *)
+ eval_const_expressions_mutator((Node *) jce->func,
+ context);
+
+ /*
+ * Find the CaseTestExpr in coercion; this is simpler 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 JsonConstructorExpr
+ * (or that coercion is empty) and must not change coparam.
+ */
+ if (find_casetestexpr((Node *) jce->coercion, &ctexpr))
+ {
+ /*
+ * Set up the node to substitute for the CaseTestExpr node
+ * contained in coercion. 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(jce->coparam == 0);
+ if (!context->estimate)
+ {
+ Param *p;
+
+ p = generate_new_expr_param(context,
+ ctexpr->typeId,
+ ctexpr->typeMod,
+ ctexpr->collation);
+ context->case_val = (Node *) p;
+ jce->coparam = p->paramid;
+ }
+ else
+ context->case_val = NULL;
+
+ jce->coercion = (Expr *)
+ eval_const_expressions_mutator((Node *) jce->coercion,
+ context);
+
+ context->case_val = save_case_val;
+ }
+ else
+ jce->coercion = (Expr *)
+ eval_const_expressions_mutator((Node *) jce->coercion,
+ context);
+
+ jce->returning = (JsonReturning *)
+ eval_const_expressions_mutator((Node *) jce->returning,
+ context);
+ return (Node *) jce;
+ }
+ case T_JsonExpr:
+ {
+ JsonExpr *je = makeNode(JsonExpr);
+ CaseTestExpr *ctexpr = NULL;
+
+ /*
+ * Copy the node and const-simplify its arguments. We can't
+ * use ece_generic_processing() here because we need to mess
+ * with case_val only while processing the formatted_expr.
+ */
+ memcpy(je, node, sizeof(JsonExpr));
+
+ /*
+ * The formatted_expr might or might not contain a
+ * CaseTestExpr; if it doesn't, we don't want to uselessly
+ * consume a PARAM_EXPR slot. Besides that, copying its
+ * type/typmod/collation is much easier than trying to
+ * reverse-engineer what it would have been.
+ */
+ if (find_casetestexpr(je->formatted_expr, &ctexpr))
+ {
+ /*
+ * Set up the node to substitute for the CaseTestExpr node
+ * contained in formatted_expr. 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(je->feparam == 0);
+ if (!context->estimate)
+ {
+ Param *p;
+
+ p = generate_new_expr_param(context,
+ ctexpr->typeId,
+ ctexpr->typeMod,
+ ctexpr->collation);
+ context->case_val = (Node *) p;
+ je->feparam = p->paramid;
+ }
+ else
+ context->case_val = NULL;
+
+ je->formatted_expr =
+ eval_const_expressions_mutator(je->formatted_expr,
+ context);
+
+ context->case_val = save_case_val;
+ }
+ else
+ je->formatted_expr =
+ eval_const_expressions_mutator(je->formatted_expr,
+ context);
+
+ je->path_spec =
+ eval_const_expressions_mutator(je->path_spec,
+ context);
+ je->passing_values = (List *)
+ eval_const_expressions_mutator((Node *) je->passing_values,
+ context);
+ je->on_empty = (JsonBehavior *)
+ eval_const_expressions_mutator((Node *) je->on_empty,
+ context);
+ je->on_error = (JsonBehavior *)
+ eval_const_expressions_mutator((Node *) je->on_error,
+ context);
+ return (Node *) je;
+ }
default:
break;
}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index a17b8a5797..ddf0d49495 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -3676,7 +3676,12 @@ makeJsonConstructorExpr(ParseState *pstate, JsonConstructorType type,
* Coerce to the RETURNING type and format, if needed. We abuse
* CaseTestExpr here as placeholder to pass the result of either
* evaluating 'fexpr' or whatever is produced by ExecEvalJsonConstructor()
- * that is of type JSON or JSONB to the coercion function.
+ * that is of type JSON or JSONB to the coercion function. This is safe
+ * because the finished 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.)
*/
if (fexpr)
{
diff --git a/src/backend/parser/parse_jsontable.c b/src/backend/parser/parse_jsontable.c
index 13d533b83f..96a3b26d96 100644
--- a/src/backend/parser/parse_jsontable.c
+++ b/src/backend/parser/parse_jsontable.c
@@ -319,6 +319,17 @@ transformJsonTableColumns(JsonTableParseContext *cxt, List *columns,
JsonFuncExpr *jfe;
CaseTestExpr *param = makeNode(CaseTestExpr);
+ /*
+ * We abuse CaseTestExpr here as a placeholder to pass the
+ * row pattern value to any coercion function that may be
+ * needed. This is safe because the finished column
+ * 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.)
+ */
param->collation = InvalidOid;
param->typeId = contextItemTypid;
param->typeMod = -1;
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index dbab24737e..2fef0adf89 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -4470,17 +4470,11 @@ JsonTableGetValue(TableFuncScanState *state, int colnum,
/* Evaluate JsonExpr. */
else if (estate)
{
- Datum saved_caseValue = econtext->caseValue_datum;
- bool saved_caseIsNull = econtext->caseValue_isNull;
-
- /* Pass the row pattern value via CaseTestExpr. */
- econtext->caseValue_datum = current->value;
- econtext->caseValue_isNull = false;
+ /* Pass the row pattern value where the expression expects it. */
+ estate->resvalue = current->value;
+ estate->resnull = false;
result = ExecEvalExpr(estate, econtext, isnull);
-
- econtext->caseValue_datum = saved_caseValue;
- econtext->caseValue_isNull = saved_caseIsNull;
}
/* ORDINAL column */
else
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 2c87e6365d..aad1fc2897 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1753,6 +1753,11 @@ typedef enum JsonConstructorType
/*
* JsonConstructorExpr -
* wrapper over FuncExpr/Aggref/WindowFunc for SQL/JSON constructors
+ *
+ * If a coercion to the RETURNING type is required, the coercion field holds a
+ * suitable coercion expression tree (else it's NULL). The base node of that
+ * tree is initially a CaseTestExpr, but the planner replaces that with a
+ * PARAM_EXPR Param, and sets coparam to that Param's paramid.
*/
typedef struct JsonConstructorExpr
{
@@ -1761,6 +1766,8 @@ typedef struct JsonConstructorExpr
List *args;
Expr *func; /* underlying json[b]_xxx() function call */
Expr *coercion; /* coercion to RETURNING type */
+ /* ID of PARAM_EXPR Param representing input, if assigned; else 0 */
+ int coparam pg_node_attr(equal_ignore, query_jumble_ignore);
JsonReturning *returning; /* RETURNING clause */
bool absent_on_null; /* ABSENT ON NULL? */
bool unique; /* WITH UNIQUE KEYS? (JSON_OBJECT[AGG] only) */
@@ -1876,6 +1883,9 @@ typedef struct JsonExpr
/* jsonb-valued expression to query */
Node *formatted_expr;
+ /* ID of PARAM_EXPR Param within formatted_expr, if any; else 0 */
+ int feparam pg_node_attr(equal_ignore, query_jumble_ignore);
+
/* Format of the above expression needed by ruleutils.c */
JsonFormat *format;
--
2.43.5
From e10b5f94a1b33e713a9676589d5a1d15b99403d5 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
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
From f2eac8057d61ecd201d4e3aaebf35751ef7f63a9 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 29 Jan 2025 20:56:18 -0500
Subject: [PATCH v1 6/8] Remove plpgsql's abuse of CaseTestExpr.
We can use a PARAM_EXTERN Param almost as easily as a CaseTestExpr,
since the cast expression this is used in has no need to access
any other Params.
At this point, the executor's support for CaseTestExpr is dead
code, because such nodes will never get past the planner.
I'll leave removing that for a separate patch, though.
---
src/pl/plpgsql/src/pl_exec.c | 47 +++++++++++++++++++++++++++---------
1 file changed, 35 insertions(+), 12 deletions(-)
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 35cda55cf9..e1f7067b1a 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -170,6 +170,7 @@ typedef struct /* cast_hash table entry */
plpgsql_CastExprHashEntry *cast_centry; /* link to matching expr entry */
/* ExprState is valid only when cast_lxid matches current LXID */
ExprState *cast_exprstate; /* expression's eval tree */
+ ParamListInfo cast_params; /* input Param value for cast expr */
bool cast_in_use; /* true while we're executing eval tree */
LocalTransactionId cast_lxid;
} plpgsql_CastHashEntry;
@@ -7754,18 +7755,29 @@ do_cast_value(PLpgSQL_execstate *estate,
if (cast_entry)
{
ExprContext *econtext = estate->eval_econtext;
+ ParamListInfo paramLI;
MemoryContext oldcontext;
oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
- econtext->caseValue_datum = value;
- econtext->caseValue_isNull = *isnull;
-
cast_entry->cast_in_use = true;
+ /* Set up the value for the cast expression's Param */
+ paramLI = cast_entry->cast_params;
+ paramLI->params[0].value = value;
+ paramLI->params[0].isnull = *isnull;
+
+ /*
+ * Someday we might need to save-n-restore ecxt_param_list_info here,
+ * but that day is not yet: it's always NULL except during expr eval.
+ */
+ econtext->ecxt_param_list_info = paramLI;
+
value = ExecEvalExpr(cast_entry->cast_exprstate, econtext,
isnull);
+ econtext->ecxt_param_list_info = NULL;
+
cast_entry->cast_in_use = false;
MemoryContextSwitchTo(oldcontext);
@@ -7780,7 +7792,7 @@ do_cast_value(PLpgSQL_execstate *estate,
* Returns a plpgsql_CastHashEntry if an expression has to be evaluated,
* or NULL if the cast is a mere no-op relabeling. If there's work to be
* done, the cast_exprstate field contains an expression evaluation tree
- * based on a CaseTestExpr input, and the cast_in_use field should be set
+ * based on a PARAM_EXTERN parameter, and the cast_in_use field should be set
* true while executing it.
* ----------
*/
@@ -7816,6 +7828,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
cast_entry->cast_centry = expr_entry;
cast_entry->cast_exprstate = NULL;
+ cast_entry->cast_params = NULL;
cast_entry->cast_in_use = false;
cast_entry->cast_lxid = InvalidLocalTransactionId;
}
@@ -7834,7 +7847,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
*/
Node *cast_expr;
CachedExpression *cast_cexpr;
- CaseTestExpr *placeholder;
+ Param *placeholder;
/*
* Drop old cached expression if there is one.
@@ -7853,13 +7866,16 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
/*
- * We use a CaseTestExpr as the base of the coercion tree, since it's
- * very cheap to insert the source value for that.
+ * We use a PARAM_EXTERN Param as the base of the coercion tree. The
+ * cast expression will not contain any other such Params.
*/
- placeholder = makeNode(CaseTestExpr);
- placeholder->typeId = srctype;
- placeholder->typeMod = srctypmod;
- placeholder->collation = get_typcollation(srctype);
+ placeholder = makeNode(Param);
+ placeholder->paramkind = PARAM_EXTERN;
+ placeholder->paramid = 1;
+ placeholder->paramtype = srctype;
+ placeholder->paramtypmod = srctypmod;
+ placeholder->paramcollid = get_typcollation(srctype);
+ placeholder->location = -1;
/*
* Apply coercion. We use the special coercion context
@@ -7929,6 +7945,7 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
/* Be sure to reset the exprstate hashtable entry, too. */
cast_entry->cast_exprstate = NULL;
+ cast_entry->cast_params = NULL;
cast_entry->cast_in_use = false;
cast_entry->cast_lxid = InvalidLocalTransactionId;
@@ -7954,8 +7971,14 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
curlxid = MyProc->vxid.lxid;
if (cast_entry->cast_lxid != curlxid || cast_entry->cast_in_use)
{
+ ParamListInfo paramLI;
+
oldcontext = MemoryContextSwitchTo(estate->simple_eval_estate->es_query_cxt);
- cast_entry->cast_exprstate = ExecInitExpr(expr_entry->cast_expr, NULL);
+ cast_entry->cast_params = paramLI = makeParamList(1);
+ paramLI->params[0].pflags = 0;
+ paramLI->params[0].ptype = srctype;
+ cast_entry->cast_exprstate = ExecInitExprWithParams(expr_entry->cast_expr,
+ paramLI);
cast_entry->cast_in_use = false;
cast_entry->cast_lxid = curlxid;
MemoryContextSwitchTo(oldcontext);
--
2.43.5
From 0d597c7bb5e6df237a046528922709605bac0cea Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 29 Jan 2025 21:12:34 -0500
Subject: [PATCH v1 7/8] Remove dead code for executor support of CaseTestExpr.
If a CaseTestExpr manages to get through the planner, we'll now
get "unrecognized node type", which is good.
I also took the opportunity to rename ExprEvalStep's "casetest"
variant to "domaintest", since it's now only used for that.
---
src/backend/executor/execExpr.c | 38 +----------
src/backend/executor/execExprInterp.c | 84 +++--------------------
src/backend/executor/execUtils.c | 6 --
src/backend/jit/llvm/llvmjit_expr.c | 98 ++++++---------------------
src/include/executor/execExpr.h | 7 +-
src/include/nodes/execnodes.h | 17 ++---
6 files changed, 39 insertions(+), 211 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 6d2b9179b2..8c8e4db419 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1823,25 +1823,11 @@ ExecInitExprRec(Expr *node, ExprState *state,
foreach(lc, caseExpr->args)
{
CaseWhen *when = (CaseWhen *) lfirst(lc);
- Datum *save_innermost_caseval;
- bool *save_innermost_casenull;
int whenstep;
- /*
- * XXX innermost_caseval/casenull will go away, but for
- * now just make them NULL.
- */
- save_innermost_caseval = state->innermost_caseval;
- save_innermost_casenull = state->innermost_casenull;
- state->innermost_caseval = NULL;
- state->innermost_casenull = NULL;
-
/* evaluate condition into CASE's result variables */
ExecInitExprRec(when->expr, state, resv, resnull);
- state->innermost_caseval = save_innermost_caseval;
- state->innermost_casenull = save_innermost_casenull;
-
/* If WHEN result isn't true, jump to next CASE arm */
scratch.opcode = EEOP_JUMP_IF_NOT_TRUE;
scratch.d.jump.jumpdone = -1; /* computed later */
@@ -1893,25 +1879,6 @@ ExecInitExprRec(Expr *node, ExprState *state,
break;
}
- case T_CaseTestExpr:
- {
- /*
- * Read from location identified by innermost_caseval. Note
- * that innermost_caseval could be NULL, if this node isn't
- * actually within a CaseExpr, ArrayCoerceExpr, etc structure.
- * That can happen because some parts of the system abuse
- * CaseTestExpr to cause a read of a value externally supplied
- * in econtext->caseValue_datum. We'll take care of that
- * scenario at runtime.
- */
- scratch.opcode = EEOP_CASE_TESTVAL;
- scratch.d.casetest.value = state->innermost_caseval;
- scratch.d.casetest.isnull = state->innermost_casenull;
-
- ExprEvalPushStep(state, &scratch);
- break;
- }
-
case T_ArrayExpr:
{
ArrayExpr *arrayexpr = (ArrayExpr *) node;
@@ -2616,9 +2583,8 @@ ExecInitExprRec(Expr *node, ExprState *state,
* scenario at runtime.
*/
scratch.opcode = EEOP_DOMAIN_TESTVAL;
- /* we share instruction union variant with case testval */
- scratch.d.casetest.value = state->innermost_domainval;
- scratch.d.casetest.isnull = state->innermost_domainnull;
+ scratch.d.domaintest.value = state->innermost_domainval;
+ scratch.d.domaintest.isnull = state->innermost_domainnull;
ExprEvalPushStep(state, &scratch);
break;
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index fa3e76eccd..57060686b1 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -160,7 +160,6 @@ static Datum ExecJustScanVar(ExprState *state, ExprContext *econtext, bool *isnu
static Datum ExecJustAssignInnerVar(ExprState *state, ExprContext *econtext, bool *isnull);
static Datum ExecJustAssignOuterVar(ExprState *state, ExprContext *econtext, bool *isnull);
static Datum ExecJustAssignScanVar(ExprState *state, ExprContext *econtext, bool *isnull);
-static Datum ExecJustApplyFuncToCase(ExprState *state, ExprContext *econtext, bool *isnull);
static Datum ExecJustConst(ExprState *state, ExprContext *econtext, bool *isnull);
static Datum ExecJustInnerVarVirt(ExprState *state, ExprContext *econtext, bool *isnull);
static Datum ExecJustOuterVarVirt(ExprState *state, ExprContext *econtext, bool *isnull);
@@ -364,13 +363,6 @@ ExecReadyInterpretedExpr(ExprState *state)
state->evalfunc_private = ExecJustAssignScanVar;
return;
}
- else if (step0 == EEOP_CASE_TESTVAL &&
- step1 == EEOP_FUNCEXPR_STRICT &&
- state->steps[0].d.casetest.value)
- {
- state->evalfunc_private = ExecJustApplyFuncToCase;
- return;
- }
else if (step0 == EEOP_INNER_VAR &&
step1 == EEOP_HASHDATUM_FIRST)
{
@@ -525,7 +517,6 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
&&CASE_EEOP_PARAM_CALLBACK,
&&CASE_EEOP_PARAM_SET_EXEC,
&&CASE_EEOP_PARAM_SET_EXPR,
- &&CASE_EEOP_CASE_TESTVAL,
&&CASE_EEOP_MAKE_READONLY,
&&CASE_EEOP_IOCOERCE,
&&CASE_EEOP_IOCOERCE_SAFE,
@@ -1288,40 +1279,21 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
EEO_NEXT();
}
- EEO_CASE(EEOP_CASE_TESTVAL)
+ EEO_CASE(EEOP_DOMAIN_TESTVAL)
{
/*
* Normally upper parts of the expression tree have setup the
* values to be returned here, but some parts of the system
- * currently misuse {caseValue,domainValue}_{datum,isNull} to set
- * run-time data. So if no values have been set-up, use
- * ExprContext's. This isn't pretty, but also not *that* ugly,
- * and this is unlikely to be performance sensitive enough to
- * worry about an extra branch.
+ * currently misuse domainValue_{datum,isNull} to set run-time
+ * data. So if no values have been set-up, use ExprContext's.
+ * This isn't pretty, but also not *that* ugly, and this is
+ * unlikely to be performance sensitive enough to worry about an
+ * extra branch.
*/
- if (op->d.casetest.value)
+ if (op->d.domaintest.value)
{
- *op->resvalue = *op->d.casetest.value;
- *op->resnull = *op->d.casetest.isnull;
- }
- else
- {
- *op->resvalue = econtext->caseValue_datum;
- *op->resnull = econtext->caseValue_isNull;
- }
-
- EEO_NEXT();
- }
-
- EEO_CASE(EEOP_DOMAIN_TESTVAL)
- {
- /*
- * See EEOP_CASE_TESTVAL comment.
- */
- if (op->d.casetest.value)
- {
- *op->resvalue = *op->d.casetest.value;
- *op->resnull = *op->d.casetest.isnull;
+ *op->resvalue = *op->d.domaintest.value;
+ *op->resnull = *op->d.domaintest.isnull;
}
else
{
@@ -2569,44 +2541,6 @@ ExecJustAssignScanVar(ExprState *state, ExprContext *econtext, bool *isnull)
return ExecJustAssignVarImpl(state, econtext->ecxt_scantuple, isnull);
}
-/* Evaluate CASE_TESTVAL and apply a strict function to it */
-static Datum
-ExecJustApplyFuncToCase(ExprState *state, ExprContext *econtext, bool *isnull)
-{
- ExprEvalStep *op = &state->steps[0];
- FunctionCallInfo fcinfo;
- NullableDatum *args;
- int nargs;
- Datum d;
-
- /*
- * XXX with some redesign of the CaseTestExpr mechanism, maybe we could
- * get rid of this data shuffling?
- */
- *op->resvalue = *op->d.casetest.value;
- *op->resnull = *op->d.casetest.isnull;
-
- op++;
-
- nargs = op->d.func.nargs;
- fcinfo = op->d.func.fcinfo_data;
- args = fcinfo->args;
-
- /* strict function, so check for NULL args */
- for (int argno = 0; argno < nargs; argno++)
- {
- if (args[argno].isnull)
- {
- *isnull = true;
- return (Datum) 0;
- }
- }
- fcinfo->isnull = false;
- d = op->d.func.fn_addr(fcinfo);
- *isnull = fcinfo->isnull;
- return d;
-}
-
/* Simple Const expression */
static Datum
ExecJustConst(ExprState *state, ExprContext *econtext, bool *isnull)
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 9a236c3789..fd541c23ec 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -271,9 +271,6 @@ CreateExprContextInternal(EState *estate, Size minContextSize,
econtext->ecxt_aggvalues = NULL;
econtext->ecxt_aggnulls = NULL;
- econtext->caseValue_datum = (Datum) 0;
- econtext->caseValue_isNull = true;
-
econtext->domainValue_datum = (Datum) 0;
econtext->domainValue_isNull = true;
@@ -388,9 +385,6 @@ CreateStandaloneExprContext(void)
econtext->ecxt_aggvalues = NULL;
econtext->ecxt_aggnulls = NULL;
- econtext->caseValue_datum = (Datum) 0;
- econtext->caseValue_isNull = true;
-
econtext->domainValue_datum = (Datum) 0;
econtext->domainValue_isNull = true;
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 1b59c30b6f..7c73cca28a 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1275,60 +1275,6 @@ llvm_compile_expr(ExprState *state)
break;
}
- case EEOP_CASE_TESTVAL:
- {
- LLVMBasicBlockRef b_avail,
- b_notavail;
- LLVMValueRef v_casevaluep,
- v_casevalue;
- LLVMValueRef v_casenullp,
- v_casenull;
- LLVMValueRef v_casevaluenull;
-
- b_avail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.avail", opno);
- b_notavail = l_bb_before_v(opblocks[opno + 1],
- "op.%d.notavail", opno);
-
- v_casevaluep = l_ptr_const(op->d.casetest.value,
- l_ptr(TypeSizeT));
- v_casenullp = l_ptr_const(op->d.casetest.isnull,
- l_ptr(TypeStorageBool));
-
- v_casevaluenull =
- LLVMBuildICmp(b, LLVMIntEQ,
- LLVMBuildPtrToInt(b, v_casevaluep,
- TypeSizeT, ""),
- l_sizet_const(0), "");
- LLVMBuildCondBr(b, v_casevaluenull, b_notavail, b_avail);
-
- /* if casetest != NULL */
- LLVMPositionBuilderAtEnd(b, b_avail);
- v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
- v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
- LLVMBuildStore(b, v_casevalue, v_resvaluep);
- LLVMBuildStore(b, v_casenull, v_resnullp);
- LLVMBuildBr(b, opblocks[opno + 1]);
-
- /* if casetest == NULL */
- LLVMPositionBuilderAtEnd(b, b_notavail);
- v_casevalue =
- l_load_struct_gep(b,
- StructExprContext,
- v_econtext,
- FIELDNO_EXPRCONTEXT_CASEDATUM, "");
- v_casenull =
- l_load_struct_gep(b,
- StructExprContext,
- v_econtext,
- FIELDNO_EXPRCONTEXT_CASENULL, "");
- LLVMBuildStore(b, v_casevalue, v_resvaluep);
- LLVMBuildStore(b, v_casenull, v_resnullp);
-
- LLVMBuildBr(b, opblocks[opno + 1]);
- break;
- }
-
case EEOP_MAKE_READONLY:
{
LLVMBasicBlockRef b_notnull;
@@ -1972,55 +1918,55 @@ llvm_compile_expr(ExprState *state)
{
LLVMBasicBlockRef b_avail,
b_notavail;
- LLVMValueRef v_casevaluep,
- v_casevalue;
- LLVMValueRef v_casenullp,
- v_casenull;
- LLVMValueRef v_casevaluenull;
+ LLVMValueRef v_domainvaluep,
+ v_domainvalue;
+ LLVMValueRef v_domainnullp,
+ v_domainnull;
+ LLVMValueRef v_domainvaluenull;
b_avail = l_bb_before_v(opblocks[opno + 1],
"op.%d.avail", opno);
b_notavail = l_bb_before_v(opblocks[opno + 1],
"op.%d.notavail", opno);
- v_casevaluep = l_ptr_const(op->d.casetest.value,
- l_ptr(TypeSizeT));
- v_casenullp = l_ptr_const(op->d.casetest.isnull,
- l_ptr(TypeStorageBool));
+ v_domainvaluep = l_ptr_const(op->d.domaintest.value,
+ l_ptr(TypeSizeT));
+ v_domainnullp = l_ptr_const(op->d.domaintest.isnull,
+ l_ptr(TypeStorageBool));
- v_casevaluenull =
+ v_domainvaluenull =
LLVMBuildICmp(b, LLVMIntEQ,
- LLVMBuildPtrToInt(b, v_casevaluep,
+ LLVMBuildPtrToInt(b, v_domainvaluep,
TypeSizeT, ""),
l_sizet_const(0), "");
LLVMBuildCondBr(b,
- v_casevaluenull,
+ v_domainvaluenull,
b_notavail, b_avail);
- /* if casetest != NULL */
+ /* if domaintest != NULL */
LLVMPositionBuilderAtEnd(b, b_avail);
- v_casevalue = l_load(b, TypeSizeT, v_casevaluep, "");
- v_casenull = l_load(b, TypeStorageBool, v_casenullp, "");
- LLVMBuildStore(b, v_casevalue, v_resvaluep);
- LLVMBuildStore(b, v_casenull, v_resnullp);
+ v_domainvalue = l_load(b, TypeSizeT, v_domainvaluep, "");
+ v_domainnull = l_load(b, TypeStorageBool, v_domainnullp, "");
+ LLVMBuildStore(b, v_domainvalue, v_resvaluep);
+ LLVMBuildStore(b, v_domainnull, v_resnullp);
LLVMBuildBr(b, opblocks[opno + 1]);
- /* if casetest == NULL */
+ /* if domaintest == NULL */
LLVMPositionBuilderAtEnd(b, b_notavail);
- v_casevalue =
+ v_domainvalue =
l_load_struct_gep(b,
StructExprContext,
v_econtext,
FIELDNO_EXPRCONTEXT_DOMAINDATUM,
"");
- v_casenull =
+ v_domainnull =
l_load_struct_gep(b,
StructExprContext,
v_econtext,
FIELDNO_EXPRCONTEXT_DOMAINNULL,
"");
- LLVMBuildStore(b, v_casevalue, v_resvaluep);
- LLVMBuildStore(b, v_casenull, v_resnullp);
+ LLVMBuildStore(b, v_domainvalue, v_resvaluep);
+ LLVMBuildStore(b, v_domainnull, v_resnullp);
LLVMBuildBr(b, opblocks[opno + 1]);
break;
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index e03045de92..5e5623b11b 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -174,9 +174,6 @@ typedef enum ExprEvalOp
EEOP_PARAM_SET_EXEC,
EEOP_PARAM_SET_EXPR,
- /* return CaseTestExpr value */
- EEOP_CASE_TESTVAL,
-
/* apply MakeExpandedObjectReadOnly() to target value */
EEOP_MAKE_READONLY,
@@ -438,12 +435,12 @@ typedef struct ExprEvalStep
Oid paramtype; /* OID of parameter's datatype */
} cparam;
- /* for EEOP_CASE_TESTVAL/DOMAIN_TESTVAL */
+ /* for EEOP_DOMAIN_TESTVAL */
struct
{
Datum *value; /* value to return */
bool *isnull;
- } casetest;
+ } domaintest;
/* for EEOP_MAKE_READONLY */
struct
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 366534c0d0..866b236ae4 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -134,9 +134,6 @@ typedef struct ExprState
struct PlanState *parent; /* parent PlanState node, if any */
ParamListInfo ext_params; /* for compiling PARAM_EXTERN nodes */
- Datum *innermost_caseval;
- bool *innermost_casenull;
-
Datum *innermost_domainval;
bool *innermost_domainnull;
@@ -289,22 +286,16 @@ typedef struct ExprContext
#define FIELDNO_EXPRCONTEXT_AGGNULLS 11
bool *ecxt_aggnulls; /* null flags for aggs/windowfuncs */
- /* Value to substitute for CaseTestExpr nodes in expression */
-#define FIELDNO_EXPRCONTEXT_CASEDATUM 12
- Datum caseValue_datum;
-#define FIELDNO_EXPRCONTEXT_CASENULL 13
- bool caseValue_isNull;
-
/* Value to substitute for CoerceToDomainValue nodes in expression */
-#define FIELDNO_EXPRCONTEXT_DOMAINDATUM 14
+#define FIELDNO_EXPRCONTEXT_DOMAINDATUM 12
Datum domainValue_datum;
-#define FIELDNO_EXPRCONTEXT_DOMAINNULL 15
+#define FIELDNO_EXPRCONTEXT_DOMAINNULL 13
bool domainValue_isNull;
/* Tuples that OLD/NEW Var nodes in RETURNING may refer to */
-#define FIELDNO_EXPRCONTEXT_OLDTUPLE 16
+#define FIELDNO_EXPRCONTEXT_OLDTUPLE 14
TupleTableSlot *ecxt_oldtuple;
-#define FIELDNO_EXPRCONTEXT_NEWTUPLE 17
+#define FIELDNO_EXPRCONTEXT_NEWTUPLE 15
TupleTableSlot *ecxt_newtuple;
/* Link to containing EState (NULL if a standalone ExprContext) */
--
2.43.5
From ad7bc5cc5f007217f260de069fe5a2066b753329 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 29 Jan 2025 21:19:11 -0500
Subject: [PATCH v1 8/8] Optimize away useless Param set/load pairs.
The code generation patterns that the previous patches added to
execExpr.c will often give rise to this situation, so clean it up.
There might be more optimization work to be done here.
In particular, I'm not clear on whether it's worth replacing the
ExecJustApplyFuncToCase function removed in the previous patch.
I don't know what use-case that was intended to cover, so it's
hard to be sure what the new corresponding pattern is.
---
src/backend/executor/execExpr.c | 51 ++++++++++++++++++++++++++++++---
1 file changed, 47 insertions(+), 4 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 8c8e4db419..54cf396819 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -892,14 +892,57 @@ ExecCheck(ExprState *state, ExprContext *econtext)
* Prepare a compiled expression for execution. This has to be called for
* every ExprState before it can be executed.
*
- * NB: While this currently only calls ExecReadyInterpretedExpr(),
- * this will likely get extended to further expression evaluation methods.
- * Therefore this should be used instead of directly calling
- * ExecReadyInterpretedExpr().
+ * Here we perform some simple optimizations on the completed steps list,
+ * and then dispatch to the appropriate evaluation method.
*/
static void
ExecReadyExpr(ExprState *state)
{
+ /*
+ * Simple peephole-like optimizations can be done on the steps list. While
+ * JIT compilation could perhaps perform the equivalent optimizations by
+ * itself, it's surely cheaper to do it here.
+ */
+
+ /* DIRECT_THREADED should not already be set, else we can't test opcodes */
+ Assert((state->flags & EEO_FLAG_DIRECT_THREADED) == 0);
+
+ /*
+ * A common pattern is for the steps list to include EEOP_PARAM_SET_EXPR
+ * followed by EEOP_PARAM_EXPR to load the same Param's value to the same
+ * place. The load step is clearly useless. Moreover, if there are no
+ * further reads of that Param, we can dispense with the
+ * EEOP_PARAM_SET_EXPR step too.
+ */
+ for (int i = 0; i < state->steps_len - 2; i++)
+ {
+ if (state->steps[i].opcode == EEOP_PARAM_SET_EXPR &&
+ state->steps[i + 1].opcode == EEOP_PARAM_EXPR &&
+ state->steps[i].d.param.paramid == state->steps[i + 1].d.param.paramid &&
+ state->steps[i].resvalue == state->steps[i + 1].resvalue &&
+ state->steps[i].resnull == state->steps[i + 1].resnull)
+ {
+ int paramid = state->steps[i].d.param.paramid;
+ int numdel = 2; /* assume both can be removed */
+
+ for (int j = i + 2; j < state->steps_len; j++)
+ {
+ if (state->steps[j].opcode == EEOP_PARAM_EXPR &&
+ state->steps[j].d.param.paramid == paramid)
+ {
+ /* found another read, so must keep the SET */
+ numdel = 1;
+ break;
+ }
+ }
+ memmove(&state->steps[i + 2 - numdel], &state->steps[i + 2],
+ (state->steps_len - i - 2) * sizeof(ExprEvalStep));
+ state->steps_len -= numdel;
+ i--; /* reconsider the current position */
+ }
+ }
+
+ /* Now dispatch... */
if (jit_compile_expr(state))
return;
--
2.43.5
