Re: Consistent segfault in complex query

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> If it helps, here is a patch that adds isolation tests to
> eval-plan-qual.spec for two test cases (one with CTE, one without).
> I've verified that these reproduce the crash, and that they run
> successfully with your patch. I can't currently see any more specific
> code paths to probe in these tests.

Thanks!  I incorporated these into the attached proposed patches.

The main difference from what I had yesterday is that I rewrote
ExecEvalParamExecParams to my satisfaction.  The crucial thing I didn't
like about it was that it hard-wired use of the GetPerTupleExprContext
econtext for initplan evaluation.  That seemed like it risked memory
leaks in case of repeated initplan evaluation for a single top-level
output tuple.  I've since convinced myself that it's basically impossible
to leak memory in ExecSetParamPlan right now (cf comments below), but
that doesn't seem like an assumption to bake into an API when it isn't
even buying us anything to do so.

The attached is split into two parts because 0001 will need to go all
the way back, whereas 0002 only applies to HEAD and v11.  I don't
plan to make them separate commits though.

A quick test says that back-patching 0001 might be slightly painful;
a lot of the hunks don't apply.  I've not looked at why not.

            regards, tom lane

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c583e02..0074014 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 46,51 ****
--- 46,52 ----
  #include "commands/matview.h"
  #include "commands/trigger.h"
  #include "executor/execdebug.h"
+ #include "executor/nodeSubplan.h"
  #include "foreign/fdwapi.h"
  #include "mb/pg_wchar.h"
  #include "miscadmin.h"
*************** EvalPlanQualBegin(EPQState *epqstate, ES
*** 3078,3083 ****
--- 3079,3092 ----
          {
              int            i;

+             /*
+              * Force evaluation of any InitPlan outputs that could be needed
+              * by the subplan, just in case they got reset since
+              * EvalPlanQualStart (see comments therein).
+              */
+             ExecSetParamPlanMulti(planstate->plan->extParam,
+                                   GetPerTupleExprContext(parentestate));
+
              i = list_length(parentestate->es_plannedstmt->paramExecTypes);

              while (--i >= 0)
*************** EvalPlanQualStart(EPQState *epqstate, ES
*** 3170,3178 ****
--- 3179,3210 ----
      {
          int            i;

+         /*
+          * Force evaluation of any InitPlan outputs that could be needed by
+          * the subplan.  (With more complexity, maybe we could postpone this
+          * till the subplan actually demands them, but it doesn't seem worth
+          * the trouble; this is a corner case already, since usually the
+          * InitPlans would have been evaluated before reaching EvalPlanQual.)
+          *
+          * This will not touch output params of InitPlans that occur somewhere
+          * within the subplan tree, only those that are attached to the
+          * ModifyTable node or above it and are referenced within the subplan.
+          * That's OK though, because the planner would only attach such
+          * InitPlans to a lower-level SubqueryScan node, and EPQ execution
+          * will not descend into a SubqueryScan.
+          *
+          * The EState's per-output-tuple econtext is sufficiently short-lived
+          * for this, since it should get reset before there is any chance of
+          * doing EvalPlanQual again.
+          */
+         ExecSetParamPlanMulti(planTree->extParam,
+                               GetPerTupleExprContext(parentestate));
+
+         /* now make the internal param workspace ... */
          i = list_length(parentestate->es_plannedstmt->paramExecTypes);
          estate->es_param_exec_vals = (ParamExecData *)
              palloc0(i * sizeof(ParamExecData));
+         /* ... and copy down all values, whether really needed or not */
          while (--i >= 0)
          {
              /* copy value if any, but not execPlan link */
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 6b37075..63de981 100644
*** a/src/backend/executor/nodeSubplan.c
--- b/src/backend/executor/nodeSubplan.c
*************** ExecInitSubPlan(SubPlan *subplan, PlanSt
*** 1009,1014 ****
--- 1009,1025 ----
   * of initplans: we don't run the subplan until/unless we need its output.
   * Note that this routine MUST clear the execPlan fields of the plan's
   * output parameters after evaluating them!
+  *
+  * The results of this function are stored in the EState associated with the
+  * ExprContext (particularly, its ecxt_param_exec_vals); any pass-by-ref
+  * result Datums are allocated in the EState's per-query memory.  The passed
+  * econtext can be any ExprContext belonging to that EState; which one is
+  * important only to the extent that the ExprContext's per-tuple memory
+  * context is used to evaluate any parameters passed down to the subplan.
+  * (Thus in principle, the shorter-lived the ExprContext the better, since
+  * that data isn't needed after we return.  In practice, because initplan
+  * parameters are never more complex than Vars, Aggrefs, etc, evaluating them
+  * currently never leaks any memory anyway.)
   * ----------------------------------------------------------------
   */
  void
*************** ExecSetParamPlan(SubPlanState *node, Exp
*** 1196,1201 ****
--- 1207,1243 ----
  }

  /*
+  * ExecSetParamPlanMulti
+  *
+  * Apply ExecSetParamPlan to evaluate any not-yet-evaluated initplan output
+  * parameters whose ParamIDs are listed in "params".  Any listed params that
+  * are not initplan outputs are ignored.
+  *
+  * As with ExecSetParamPlan, any ExprContext belonging to the current EState
+  * can be used, but in principle a shorter-lived ExprContext is better than a
+  * longer-lived one.
+  */
+ void
+ ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext)
+ {
+     int            paramid;
+
+     paramid = -1;
+     while ((paramid = bms_next_member(params, paramid)) >= 0)
+     {
+         ParamExecData *prm = &(econtext->ecxt_param_exec_vals[paramid]);
+
+         if (prm->execPlan != NULL)
+         {
+             /* Parameter not evaluated yet, so go do it */
+             ExecSetParamPlan(prm->execPlan, econtext);
+             /* ExecSetParamPlan should have processed this param... */
+             Assert(prm->execPlan == NULL);
+         }
+     }
+ }
+
+ /*
   * Mark an initplan as needing recalculation
   */
  void
diff --git a/src/include/executor/nodeSubplan.h b/src/include/executor/nodeSubplan.h
index d9784a2..fd21b5d 100644
*** a/src/include/executor/nodeSubplan.h
--- b/src/include/executor/nodeSubplan.h
*************** extern void ExecReScanSetParamPlan(SubPl
*** 28,31 ****
--- 28,33 ----

  extern void ExecSetParamPlan(SubPlanState *node, ExprContext *econtext);

+ extern void ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext);
+
  #endif                            /* NODESUBPLAN_H */
diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out
index 9bbfdc1..49b3fb3 100644
*** a/src/test/isolation/expected/eval-plan-qual.out
--- b/src/test/isolation/expected/eval-plan-qual.out
*************** ta_id          ta_value       tb_row
*** 184,189 ****
--- 184,220 ----
  1              newTableAValue (1,tableBValue)
  step c2: COMMIT;

+ starting permutation: updateforcip updateforcip2 c1 c2 read_a
+ step updateforcip:
+     UPDATE table_a SET value = NULL WHERE id = 1;
+
+ step updateforcip2:
+     UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
+  <waiting ...>
+ step c1: COMMIT;
+ step updateforcip2: <... completed>
+ step c2: COMMIT;
+ step read_a: SELECT * FROM table_a ORDER BY id;
+ id             value
+
+ 1              newValue
+
+ starting permutation: updateforcip updateforcip3 c1 c2 read_a
+ step updateforcip:
+     UPDATE table_a SET value = NULL WHERE id = 1;
+
+ step updateforcip3:
+     WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
+     UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
+  <waiting ...>
+ step c1: COMMIT;
+ step updateforcip3: <... completed>
+ step c2: COMMIT;
+ step read_a: SELECT * FROM table_a ORDER BY id;
+ id             value
+
+ 1              newValue
+
  starting permutation: wrtwcte readwcte c1 c2
  step wrtwcte: UPDATE table_a SET value = 'tableAValue2' WHERE id = 1;
  step readwcte:
diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec
index 0b70ad5..367922d 100644
*** a/src/test/isolation/specs/eval-plan-qual.spec
--- b/src/test/isolation/specs/eval-plan-qual.spec
*************** step "updateforss"    {
*** 92,97 ****
--- 92,104 ----
      UPDATE table_b SET value = 'newTableBValue' WHERE id = 1;
  }

+ # these tests exercise EvalPlanQual with conditional InitPlans which
+ # have not been executed prior to the EPQ
+
+ step "updateforcip"    {
+     UPDATE table_a SET value = NULL WHERE id = 1;
+ }
+
  # these tests exercise mark/restore during EPQ recheck, cf bug #15032

  step "selectjoinforupdate"    {
*************** step "readforss"    {
*** 129,134 ****
--- 136,148 ----
      FROM table_a ta
      WHERE ta.id = 1 FOR UPDATE OF ta;
  }
+ step "updateforcip2"    {
+     UPDATE table_a SET value = COALESCE(value, (SELECT text 'newValue')) WHERE id = 1;
+ }
+ step "updateforcip3"    {
+     WITH d(val) AS (SELECT text 'newValue' FROM generate_series(1,1))
+     UPDATE table_a SET value = COALESCE(value, (SELECT val FROM d)) WHERE id = 1;
+ }
  step "wrtwcte"    { UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; }
  step "wrjt"    { UPDATE jointest SET data = 42 WHERE id = 7; }
  step "c2"    { COMMIT; }
*************** session "s3"
*** 137,142 ****
--- 151,157 ----
  setup        { BEGIN ISOLATION LEVEL READ COMMITTED; }
  step "read"    { SELECT * FROM accounts ORDER BY accountid; }
  step "read_ext"    { SELECT * FROM accounts_ext ORDER BY accountid; }
+ step "read_a"    { SELECT * FROM table_a ORDER BY id; }

  # this test exercises EvalPlanQual with a CTE, cf bug #14328
  step "readwcte"    {
*************** permutation "wx2" "partiallock" "c2" "c1
*** 171,176 ****
--- 186,193 ----
  permutation "wx2" "lockwithvalues" "c2" "c1" "read"
  permutation "wx2_ext" "partiallock_ext" "c2" "c1" "read_ext"
  permutation "updateforss" "readforss" "c1" "c2"
+ permutation "updateforcip" "updateforcip2" "c1" "c2" "read_a"
+ permutation "updateforcip" "updateforcip3" "c1" "c2" "read_a"
  permutation "wrtwcte" "readwcte" "c1" "c2"
  permutation "wrjt" "selectjoinforupdate" "c2" "c1"
  permutation "wrtwcte" "multireadwcte" "c1" "c2"
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 9d6e25a..f597363 100644
*** a/src/backend/executor/execExprInterp.c
--- b/src/backend/executor/execExprInterp.c
*************** ExecEvalParamExec(ExprState *state, Expr
*** 2253,2285 ****
  }

  /*
-  * ExecEvalParamExecParams
-  *
-  * Execute the subplan stored in PARAM_EXEC initplans params, if not executed
-  * till now.
-  */
- void
- ExecEvalParamExecParams(Bitmapset *params, EState *estate)
- {
-     ParamExecData *prm;
-     int            paramid;
-
-     paramid = -1;
-     while ((paramid = bms_next_member(params, paramid)) >= 0)
-     {
-         prm = &(estate->es_param_exec_vals[paramid]);
-
-         if (prm->execPlan != NULL)
-         {
-             /* Parameter not evaluated yet, so go do it */
-             ExecSetParamPlan(prm->execPlan, GetPerTupleExprContext(estate));
-             /* ExecSetParamPlan should have processed this param... */
-             Assert(prm->execPlan == NULL);
-         }
-     }
- }
-
- /*
   * Evaluate a PARAM_EXTERN parameter.
   *
   * PARAM_EXTERN parameters must be sought in ecxt_param_list_info.
--- 2253,2258 ----
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index ee0f07a..c93084e 100644
*** a/src/backend/executor/execParallel.c
--- b/src/backend/executor/execParallel.c
***************
*** 23,29 ****

  #include "postgres.h"

- #include "executor/execExpr.h"
  #include "executor/execParallel.h"
  #include "executor/executor.h"
  #include "executor/nodeAppend.h"
--- 23,28 ----
***************
*** 36,41 ****
--- 35,41 ----
  #include "executor/nodeIndexonlyscan.h"
  #include "executor/nodeSeqscan.h"
  #include "executor/nodeSort.h"
+ #include "executor/nodeSubplan.h"
  #include "executor/tqueue.h"
  #include "nodes/nodeFuncs.h"
  #include "optimizer/planmain.h"
*************** ExecInitParallelPlan(PlanState *planstat
*** 581,588 ****
      char       *query_string;
      int            query_len;

!     /* Force parameters we're going to pass to workers to be evaluated. */
!     ExecEvalParamExecParams(sendParams, estate);

      /* Allocate object for return value. */
      pei = palloc0(sizeof(ParallelExecutorInfo));
--- 581,598 ----
      char       *query_string;
      int            query_len;

!     /*
!      * Force any initplan outputs that we're going to pass to workers to be
!      * evaluated, if they weren't already.
!      *
!      * For simplicity, we use the EState's per-output-tuple ExprContext here.
!      * That risks intra-query memory leakage, since we might pass through here
!      * many times before that ExprContext gets reset; but ExecSetParamPlan
!      * doesn't normally leak any memory in the context (see its comments), so
!      * it doesn't seem worth complicating this function's API to pass it a
!      * shorter-lived ExprContext.  This might need to change someday.
!      */
!     ExecSetParamPlanMulti(sendParams, GetPerTupleExprContext(estate));

      /* Allocate object for return value. */
      pei = palloc0(sizeof(ParallelExecutorInfo));
*************** ExecParallelReinitialize(PlanState *plan
*** 831,838 ****
      /* Old workers must already be shut down */
      Assert(pei->finished);

!     /* Force parameters we're going to pass to workers to be evaluated. */
!     ExecEvalParamExecParams(sendParams, estate);

      ReinitializeParallelDSM(pei->pcxt);
      pei->tqueue = ExecParallelSetupTupleQueues(pei->pcxt, true);
--- 841,852 ----
      /* Old workers must already be shut down */
      Assert(pei->finished);

!     /*
!      * Force any initplan outputs that we're going to pass to workers to be
!      * evaluated, if they weren't already (see comments in
!      * ExecInitParallelPlan).
!      */
!     ExecSetParamPlanMulti(sendParams, GetPerTupleExprContext(estate));

      ReinitializeParallelDSM(pei->pcxt);
      pei->tqueue = ExecParallelSetupTupleQueues(pei->pcxt, true);
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index f7b1f77..02af68f 100644
*** a/src/include/executor/execExpr.h
--- b/src/include/executor/execExpr.h
*************** extern void ExecEvalFuncExprStrictFusage
*** 697,703 ****
                               ExprContext *econtext);
  extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
                    ExprContext *econtext);
- extern void ExecEvalParamExecParams(Bitmapset *params, EState *estate);
  extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
                      ExprContext *econtext);
  extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op);
--- 697,702 ----