Fix buggy interaction between array subscripts and subplan params

In a7f107df2 I changed subplan param evaluation to happen within the
containing expression. As part of that, ExecInitSubPlanExpr() was changed to
evaluate parameters via a new EEOP_PARAM_SET expression step. These parameters
were temporarily stored into ExprState->resvalue/resnull, with some reasoning
why that would be fine. Unfortunately, that analysis was wrong -
ExecInitSubscriptionRef() evaluates the input array into "resv"/"resnull",
which will often point to ExprState->resvalue/resnull. This means that the
EEOP_PARAM_SET, if inside an array subscript, would overwrite the input array
to array subscript.

The fix is fairly simple - instead of evaluating into
ExprState->resvalue/resnull, store the temporary result of the subplan in the
subplan's return value.

Bug: #19370
Reported-by: Zepeng Zhang <redraiment@gmail.com>
Diagnosed-by: Tom Lane <tgl@sss.pgh.pa.us>
Diagnosed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/19370-7fb7a5854b7618f1@postgresql.org
Backpatch-through: 18

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index f1569879b529d5390dc5c43f4ff3c045a64de11b..038a93c5cc78bb9672161c4a29653b40f02e7fa2 100644 (file)
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2833,12 +2833,13 @@ ExecInitSubPlanExpr(SubPlan *subplan,
        /*
         * 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.
+        * We evaluate the argument expressions into resv/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. It's ok
+        * to use resv/resnull for multiple params, as each parameter evaluation
+        * is immediately followed by an EEOP_PARAM_SET (and thus are saved before
+        * they could be overwritten again).
         *
         * 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.
@@ -2849,10 +2850,11 @@ ExecInitSubPlanExpr(SubPlan *subplan,
                int                     paramid = lfirst_int(l);
                Expr       *arg = (Expr *) lfirst(pvar);
 
-               ExecInitExprRec(arg, state,
-                                               &state->resvalue, &state->resnull);
+               ExecInitExprRec(arg, state, resv, resnull);
 
                scratch.opcode = EEOP_PARAM_SET;
+               scratch.resvalue = resv;
+               scratch.resnull = resnull;
                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 1a37737d4a23599a0170a63d9aaf8e0300833fd0..b0d63f59eabdf7e873147db044744df27d1a017c 100644 (file)
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -3107,7 +3107,7 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 
 /*
  * Set value of a param (currently always PARAM_EXEC) from
- * state->res{value,null}.
+ * op->res{value,null}.
  */
 void
 ExecEvalParamSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
@@ -3119,8 +3119,8 @@ ExecEvalParamSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
        /* Shouldn't have a pending evaluation anymore */
        Assert(prm->execPlan == NULL);
 
-       prm->value = state->resvalue;
-       prm->isnull = state->resnull;
+       prm->value = *op->resvalue;
+       prm->isnull = *op->resnull;
 }
 
 /*

diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index ff667bec8ba2f0c3b21f7cd815cfc4e6f1b485d7..aa8c5dcc7f4c37b8bbfbf22aa528ae375776bf27 100644 (file)
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -676,6 +676,23 @@ select * from (
    0
 (1 row)
 
+--
+-- Test cases for interactions between PARAM_EXEC, subplans and array
+-- subscripts
+--
+-- check that array subscription doesn't conflict with PARAM_EXEC (see #19370)
+SELECT (array[1,2])[(SELECT g.i)] FROM generate_series(1, 1) g(i);
+ array 
+-------
+     1
+(1 row)
+
+SELECT (array[1,2])[(SELECT g.i):(SELECT g.i + 1)] FROM generate_series(1, 1) g(i);
+ array 
+-------
+ {1,2}
+(1 row)
+
 --
 -- Test that an IN implemented using a UniquePath does unique-ification
 -- with the right semantics, as per bug #4113.  (Unfortunately we have

diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index d77588ea91f8d3832a2b5f5f40cee2f3830d6785..99bbcc64d5d1741375b792190275ca86f5a5dfe6 100644 (file)
--- a/src/test/regress/sql/subselect.sql
+++ b/src/test/regress/sql/subselect.sql
@@ -340,6 +340,17 @@ select * from (
   where not exists (select 1 from tenk1 as b where b.unique2 = 10000)
 ) ss;
 
+
+--
+-- Test cases for interactions between PARAM_EXEC, subplans and array
+-- subscripts
+--
+
+-- check that array subscription doesn't conflict with PARAM_EXEC (see #19370)
+SELECT (array[1,2])[(SELECT g.i)] FROM generate_series(1, 1) g(i);
+SELECT (array[1,2])[(SELECT g.i):(SELECT g.i + 1)] FROM generate_series(1, 1) g(i);
+
+
 --
 -- Test that an IN implemented using a UniquePath does unique-ification
 -- with the right semantics, as per bug #4113.  (Unfortunately we have