Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

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: */