On 26.01.2024 05:37, vignesh C wrote:
On Tue, 24 Oct 2023 at 01:47, Alena Rybakina <lena.riback...@yandex.ru> wrote:
Hi!
I looked through your patch and noticed that it was not applied to the current
version of the master. I rebased it and attached a version. I didn't see any
problems and, honestly, no big changes were needed, all regression tests were
passed.
I think it's better to add a test, but to be honest, I haven't been able to
come up with something yet.
The patch does not apply anymore as in CFBot at [1]:
=== Applying patches on top of PostgreSQL commit ID
7014c9a4bba2d1b67d60687afb5b2091c1d07f73 ===
=== applying patch
./v2-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-the.patch
....
patching file src/include/executor/execExpr.h
Hunk #1 succeeded at 160 (offset 1 line).
Hunk #2 succeeded at 382 (offset 2 lines).
Hunk #3 FAILED at 778.
1 out of 3 hunks FAILED -- saving rejects to file
src/include/executor/execExpr.h.rej
patching file src/include/nodes/execnodes.h
Hunk #1 succeeded at 959 (offset 7 lines).
Please have a look and post an updated version.
[1] - http://cfbot.cputube.org/patch_46_4209.log
Regards,
Vignesh
Thank you!
I fixed it. The code remains the same.
--
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
From bf40b14c0cb63f47280299fd3f76a1711db6aada Mon Sep 17 00:00:00 2001
From: Alena Rybakina <a.rybak...@postgrespro.ru>
Date: Sun, 28 Jan 2024 11:58:44 +0300
Subject: [PATCH] WIP: Evaluate arguments of correlated SubPlans in the
referencing ExprState.
---
src/backend/executor/execExpr.c | 93 +++++++++++++++++----------
src/backend/executor/execExprInterp.c | 22 +++++++
src/backend/executor/execProcnode.c | 5 ++
src/backend/executor/nodeSubplan.c | 30 ++++-----
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/backend/jit/llvm/llvmjit_types.c | 1 +
src/include/executor/execExpr.h | 6 +-
src/include/nodes/execnodes.h | 1 -
8 files changed, 112 insertions(+), 52 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 3181b1136a2..d2e539e7b28 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -88,6 +88,9 @@ static void ExecBuildAggTransCall(ExprState *state, AggState *aggstate,
FunctionCallInfo fcinfo, AggStatePerTrans pertrans,
int transno, int setno, int setoff, bool ishash,
bool nullcheck);
+static void ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull);
/*
@@ -1386,7 +1389,6 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_SubPlan:
{
SubPlan *subplan = (SubPlan *) node;
- SubPlanState *sstate;
/*
* Real execution of a MULTIEXPR SubPlan has already been
@@ -1403,19 +1405,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
break;
}
- if (!state->parent)
- elog(ERROR, "SubPlan found with no parent plan");
-
- sstate = ExecInitSubPlan(subplan, state->parent);
-
- /* add SubPlanState nodes to state->parent->subPlan */
- state->parent->subPlan = lappend(state->parent->subPlan,
- sstate);
-
- scratch.opcode = EEOP_SUBPLAN;
- scratch.d.subplan.sstate = sstate;
-
- ExprEvalPushStep(state, &scratch);
+ ExecInitSubPlanExpr(subplan, state, resv, resnull);
break;
}
@@ -2752,29 +2742,12 @@ ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info)
foreach(lc, info->multiexpr_subplans)
{
SubPlan *subplan = (SubPlan *) lfirst(lc);
- SubPlanState *sstate;
Assert(subplan->subLinkType == MULTIEXPR_SUBLINK);
- /* This should match what ExecInitExprRec does for other SubPlans: */
-
- if (!state->parent)
- elog(ERROR, "SubPlan found with no parent plan");
-
- sstate = ExecInitSubPlan(subplan, state->parent);
-
- /* add SubPlanState nodes to state->parent->subPlan */
- state->parent->subPlan = lappend(state->parent->subPlan,
- sstate);
-
- scratch.opcode = EEOP_SUBPLAN;
- scratch.d.subplan.sstate = sstate;
-
/* The result can be ignored, but we better put it somewhere */
- scratch.resvalue = &state->resvalue;
- scratch.resnull = &state->resnull;
-
- ExprEvalPushStep(state, &scratch);
+ ExecInitSubPlanExpr(subplan, state,
+ &state->resvalue, &state->resnull);
}
}
@@ -4181,3 +4154,57 @@ ExecBuildParamSetEqual(TupleDesc desc,
return state;
}
+
+static void
+ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull)
+{
+ ExprEvalStep scratch = {0};
+ SubPlanState *sstate;
+ ListCell *pvar;
+ ListCell *l;
+
+ if (!state->parent)
+ elog(ERROR, "SubPlan found with no parent plan");
+
+ /*
+ * Generate steps to evaluate input arguments for the subplan.
+ *
+ * We evaluate the argument expressions into ExprState's resvalue/resnull,
+ * and then use PARAM_SET to update the parameter. We do that, instead of
+ * evaluating directly into the param, to avoid depending on the pointer
+ * value remaining stable / being included in the generated expression. No
+ * danger of conflicts with other uses of resvalue/resnull as storing and
+ * using the value always is in subsequent steps.
+ *
+ * Any calculation we have to do can be done in the parent econtext, since
+ * the Param values don't need to have per-query lifetime.
+ */
+ forboth(l, subplan->parParam, pvar, subplan->args)
+ {
+ int paramid = lfirst_int(l);
+
+ ExecInitExprRec(lfirst(pvar), state,
+ &state->resvalue, &state->resnull);
+
+ scratch.opcode = EEOP_PARAM_SET;
+ scratch.d.param.paramid = paramid;
+ /* type isn't needed, but an old value could be confusing */
+ scratch.d.param.paramtype = InvalidOid;
+ ExprEvalPushStep(state, &scratch);
+ }
+
+ sstate = ExecInitSubPlan(subplan, state->parent);
+
+ /* add SubPlanState nodes to state->parent->subPlan */
+ state->parent->subPlan = lappend(state->parent->subPlan,
+ sstate);
+
+ scratch.opcode = EEOP_SUBPLAN;
+ scratch.resvalue = resv;
+ scratch.resnull = resnull;
+ scratch.d.subplan.sstate = sstate;
+
+ ExprEvalPushStep(state, &scratch);
+}
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 3f20f1dd314..edab4ed357b 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -450,6 +450,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
&&CASE_EEOP_PARAM_EXEC,
&&CASE_EEOP_PARAM_EXTERN,
&&CASE_EEOP_PARAM_CALLBACK,
+ &&CASE_EEOP_PARAM_SET,
&&CASE_EEOP_CASE_TESTVAL,
&&CASE_EEOP_MAKE_READONLY,
&&CASE_EEOP_IOCOERCE,
@@ -1089,6 +1090,13 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
EEO_NEXT();
}
+ EEO_CASE(EEOP_PARAM_SET)
+ {
+ /* out of line, unlikely to matter performancewise */
+ ExecEvalParamSet(state, op, econtext);
+ EEO_NEXT();
+ }
+
EEO_CASE(EEOP_CASE_TESTVAL)
{
/*
@@ -2521,6 +2529,20 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
errmsg("no value found for parameter %d", paramId)));
}
+/*
+ * Set value of a param (currently always PARAM_EXEC) from
+ * state->res{value,null}.
+ */
+void
+ExecEvalParamSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
+{
+ ParamExecData *prm;
+
+ prm = &(econtext->ecxt_param_exec_vals[op->d.param.paramid]);
+ prm->value = state->resvalue;
+ prm->isnull = state->resnull;
+}
+
/*
* Evaluate a CoerceViaIO node in soft-error mode.
*
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index 6e48062f56a..34f28dfecea 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -393,6 +393,10 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
/*
* Initialize any initPlans present in this node. The planner put them in
* a separate list for us.
+ *
+ * The defining characteristic of initplans is that they don't have
+ * arguments, so we don't need to evaluate them (in contrast to
+ * ExecInitSubPlanExpr()).
*/
subps = NIL;
foreach(l, node->initPlan)
@@ -401,6 +405,7 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
SubPlanState *sstate;
Assert(IsA(subplan, SubPlan));
+ Assert(subplan->args == NIL);
sstate = ExecInitSubPlan(subplan, result);
subps = lappend(subps, sstate);
}
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 9697b1f396d..2335a828f4f 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -107,7 +107,7 @@ ExecHashSubPlan(SubPlanState *node,
TupleTableSlot *slot;
/* Shouldn't have any direct correlation Vars */
- if (subplan->parParam != NIL || node->args != NIL)
+ if (subplan->parParam != NIL || subplan->args != NIL)
elog(ERROR, "hashed subplan with direct correlation not supported");
/*
@@ -231,7 +231,6 @@ ExecScanSubPlan(SubPlanState *node,
TupleTableSlot *slot;
Datum result;
bool found = false; /* true if got at least one subplan tuple */
- ListCell *pvar;
ListCell *l;
ArrayBuildStateAny *astate = NULL;
@@ -248,26 +247,20 @@ ExecScanSubPlan(SubPlanState *node,
oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
/*
- * Set Params of this plan from parent plan correlation values. (Any
- * calculation we have to do is done in the parent econtext, since the
- * Param values don't need to have per-query lifetime.)
+ * We rely on the caller to evaluate plan correlation values, if
+ * necessary. However we still need to record the fact that the values
+ * (might have) changed, otherwise the ExecReScan() below won't know that
+ * nodes need to be rescanned.
*/
- Assert(list_length(subplan->parParam) == list_length(node->args));
-
- forboth(l, subplan->parParam, pvar, node->args)
+ Assert(list_length(subplan->parParam) == list_length(subplan->args));
+ foreach(l, subplan->parParam)
{
int paramid = lfirst_int(l);
- ParamExecData *prm = &(econtext->ecxt_param_exec_vals[paramid]);
- prm->value = ExecEvalExprSwitchContext((ExprState *) lfirst(pvar),
- econtext,
- &(prm->isnull));
planstate->chgParam = bms_add_member(planstate->chgParam, paramid);
}
- /*
- * Now that we've set up its parameters, we can reset the subplan.
- */
+ /* with that done, we can reset the subplan */
ExecReScan(planstate);
/*
@@ -817,6 +810,10 @@ slotNoNulls(TupleTableSlot *slot)
* as well as regular SubPlans. Note that we don't link the SubPlan into
* the parent's subPlan list, because that shouldn't happen for InitPlans.
* Instead, ExecInitExpr() does that one part.
+ *
+ * We also rely on ExecInitExpr(), more precisely ExecInitSubPlanExpr(), to
+ * evaluate input parameters, as that allows them to be evaluated as part of
+ * the expression referencing the SubPlan.
* ----------------------------------------------------------------
*/
SubPlanState *
@@ -844,7 +841,6 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
/* Initialize subexpressions */
sstate->testexpr = ExecInitExpr((Expr *) subplan->testexpr, parent);
- sstate->args = ExecInitExprList(subplan->args, parent);
/*
* initialize my state
@@ -1107,7 +1103,7 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
elog(ERROR, "ANY/ALL subselect unsupported as initplan");
if (subLinkType == CTE_SUBLINK)
elog(ERROR, "CTE subplans should not be executed via ExecSetParamPlan");
- if (subplan->parParam || node->args)
+ if (subplan->parParam || subplan->args)
elog(ERROR, "correlated subplans should not be executed via ExecSetParamPlan");
/*
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 0c448422e20..88eee19c50e 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1145,6 +1145,12 @@ llvm_compile_expr(ExprState *state)
break;
}
+ case EEOP_PARAM_SET:
+ build_EvalXFunc(b, mod, "ExecEvalParamSet",
+ v_state, op, v_econtext);
+ LLVMBuildBr(b, opblocks[opno + 1]);
+ break;
+
case EEOP_SBSREF_SUBSCRIPTS:
{
int jumpdone = op->d.sbsref_subscript.jumpdone;
diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index 47c9daf4023..9fc525cbe45 100644
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -159,6 +159,7 @@ void *referenced_functions[] =
ExecEvalNextValueExpr,
ExecEvalParamExec,
ExecEvalParamExtern,
+ ExecEvalParamSet,
ExecEvalRow,
ExecEvalRowNotNull,
ExecEvalRowNull,
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index a28ddcdd771..09c48a7ba92 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -160,6 +160,8 @@ typedef enum ExprEvalOp
EEOP_PARAM_EXEC,
EEOP_PARAM_EXTERN,
EEOP_PARAM_CALLBACK,
+ /* set PARAM_EXEC value */
+ EEOP_PARAM_SET,
/* return CaseTestExpr value */
EEOP_CASE_TESTVAL,
@@ -380,7 +382,7 @@ typedef struct ExprEvalStep
ExprEvalRowtypeCache rowcache;
} nulltest_row;
- /* for EEOP_PARAM_EXEC/EXTERN */
+ /* for EEOP_PARAM_EXEC/EXTERN and EEOP_PARAM_SET */
struct
{
int paramid; /* numeric ID for parameter */
@@ -777,6 +779,8 @@ 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 ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalCoerceViaIOSafe(ExprState *state, ExprEvalStep *op);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 444a5f0fd57..ce93ed451a6 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -959,7 +959,6 @@ typedef struct SubPlanState
struct PlanState *planstate; /* subselect plan's state tree */
struct PlanState *parent; /* parent plan node's state tree */
ExprState *testexpr; /* state of combining expression */
- List *args; /* states of argument expression(s) */
HeapTuple curTuple; /* copy of most recent tuple from subplan */
Datum curArray; /* most recent array from ARRAY() subplan */
/* these are used when hashing the subselect's output: */
--
2.34.1
