From bdc5dedfcaa57ddeef115252283019d79083d8a2 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Tue, 6 Jan 2026 19:51:19 -0500 Subject: [PATCH] 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 Diagnosed-by: Tom Lane Diagnosed-by: Andres Freund Discussion: https://postgr.es/m/19370-7fb7a5854b7618f1@postgresql.org Backpatch-through: 18 --- src/backend/executor/execExpr.c | 18 ++++++++++-------- src/backend/executor/execExprInterp.c | 6 +++--- src/test/regress/expected/subselect.out | 17 +++++++++++++++++ src/test/regress/sql/subselect.sql | 11 +++++++++++ 4 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index f1569879b52..038a93c5cc7 100644 --- 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 1a37737d4a2..b0d63f59eab 100644 --- 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 ff667bec8ba..aa8c5dcc7f4 100644 --- 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 d77588ea91f..99bbcc64d5d 100644 --- 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