Re: Evaluate arguments of correlated SubPlans in the referencing ExprState - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: Evaluate arguments of correlated SubPlans in the referencing ExprState |
Date | |
Msg-id | 3988437.1721332879@sss.pgh.pa.us Whole thread Raw |
In response to | Re: Evaluate arguments of correlated SubPlans in the referencing ExprState (Alena Rybakina <lena.ribackina@yandex.ru>) |
Responses |
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState |
List | pgsql-hackers |
Alena Rybakina <lena.ribackina@yandex.ru> writes: > I fixed it. The code remains the same. I see the cfbot is again complaining that this patch doesn't apply. In hopes of pushing this over the finish line, I fixed up the (minor) patch conflict and also addressed the cosmetic complaints I had upthread [1]. I think the attached v4 is committable. If Andres is too busy, I can push it, but really it's his patch ... (BTW, I see no need for additional test cases. Coverage checks show that all this code is reached during the core regression tests.) regards, tom lane [1] https://www.postgresql.org/message-id/2618533.1677874158%40sss.pgh.pa.us diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index ccd4863778..9a13825161 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -69,6 +69,9 @@ static void ExecInitExprRec(Expr *node, ExprState *state, static void ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args, Oid funcid, Oid inputcollid, ExprState *state); +static void ExecInitSubPlanExpr(SubPlan *subplan, + ExprState *state, + Datum *resv, bool *resnull); static void ExecCreateExprSetupSteps(ExprState *state, Node *node); static void ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info); static bool expr_setup_walker(Node *node, ExprSetupInfo *info); @@ -1405,7 +1408,6 @@ ExecInitExprRec(Expr *node, ExprState *state, case T_SubPlan: { SubPlan *subplan = (SubPlan *) node; - SubPlanState *sstate; /* * Real execution of a MULTIEXPR SubPlan has already been @@ -1422,19 +1424,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; } @@ -2714,6 +2704,70 @@ ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args, Oid funcid, } } +/* + * Append the steps necessary for the evaluation of a SubPlan node to + * ExprState->steps. + * + * subplan - SubPlan expression to evaluate + * state - ExprState to whose ->steps to append the necessary operations + * resv / resnull - where to store the result of the node into + */ +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. + */ + Assert(list_length(subplan->parParam) == list_length(subplan->args)); + forboth(l, subplan->parParam, pvar, subplan->args) + { + int paramid = lfirst_int(l); + Expr *arg = (Expr *) lfirst(pvar); + + ExecInitExprRec(arg, state, + &state->resvalue, &state->resnull); + + scratch.opcode = EEOP_PARAM_SET; + scratch.d.param.paramid = paramid; + /* paramtype's not actually used, but we might as well fill it */ + scratch.d.param.paramtype = exprType((Node *) arg); + 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); +} + /* * Add expression steps performing setup that's needed before any of the * main execution of the expression. @@ -2788,29 +2842,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); } } diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index d8735286c4..acdd918cbb 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, @@ -1093,6 +1094,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) { /* @@ -2555,6 +2563,24 @@ 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]); + + /* Shouldn't have a pending evaluation anymore */ + Assert(prm->execPlan == NULL); + + 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 6e48062f56..34f28dfece 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 9697b1f396..a96cdd01e1 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,19 @@ 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) + 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 +809,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 +840,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 +1102,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 cbd9ed7cc4..27f94f9007 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 f93c383fd5..4a9e077014 100644 --- a/src/backend/jit/llvm/llvmjit_types.c +++ b/src/backend/jit/llvm/llvmjit_types.c @@ -160,6 +160,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 55337d4916..9f520a867a 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, @@ -384,7 +386,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 */ @@ -796,6 +798,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 cac684d9b3..c3670f7158 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -961,7 +961,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: */
pgsql-hackers by date: