From 772a6d45ef84d9868594c5ea41d68a9797db1de1 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 10 Jul 2008 01:17:29 +0000
Subject: [PATCH] Fix mis-calculation of extParam/allParam sets for plan nodes,
as seen in bug #4290. The fundamental bug is that masking extParam by
outer_params, as finalize_plan had been doing, caused us to lose the
information that an initPlan depended on the output of a sibling initPlan.
On reflection the best thing to do seemed to be not to try to adjust
outer_params for this case but get rid of it entirely. The only thing it was
really doing for us was to filter out param IDs associated with SubPlan
nodes, and that can be done (with greater accuracy) while processing
individual SubPlan nodes in finalize_primnode. This approach was vindicated
by the discovery that the masking method was hiding a second bug:
SS_finalize_plan failed to remove extParam bits for initPlan output params
that were referenced in the main plan tree (it only got rid of those
referenced by other initPlans). It's not clear that this caused any real
problems, given the limited use of extParam by the executor, but it's
certainly not what was intended.
I originally thought that there was also a problem with needing to include
indirect dependencies on external params in initPlans' param sets, but it
turns out that the executor handles this correctly so long as the depended-on
initPlan is earlier in the initPlans list than the one using its output.
That seems a bit of a fragile assumption, but it is true at the moment,
so I just documented it in some code comments rather than making what would
be rather invasive changes to remove the assumption.
Back-patch to 8.1. Previous versions don't have the case of initPlans
referring to other initPlans' outputs, so while the existing logic is still
questionable for them, there are not any known bugs to be fixed. So I'll
refrain from changing them for now.
---
src/backend/executor/execAmi.c | 16 +++-
src/backend/optimizer/plan/planagg.c | 8 +-
src/backend/optimizer/plan/subselect.c | 116 +++++++++++++++---------
src/test/regress/expected/subselect.out | 27 ++++++
src/test/regress/sql/subselect.sql | 26 ++++++
5 files changed, 145 insertions(+), 48 deletions(-)
diff --git a/src/backend/executor/execAmi.c b/src/backend/executor/execAmi.c
index 3565a2fb12a..b637b5a08f2 100644
--- a/src/backend/executor/execAmi.c
+++ b/src/backend/executor/execAmi.c
@@ -6,7 +6,7 @@
* Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * $PostgreSQL: pgsql/src/backend/executor/execAmi.c,v 1.94 2008/01/01 19:45:49 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/executor/execAmi.c,v 1.95 2008/07/10 01:17:29 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -63,7 +63,19 @@ ExecReScan(PlanState *node, ExprContext *exprCtxt)
if (node->instrument)
InstrEndLoop(node->instrument);
- /* If we have changed parameters, propagate that info */
+ /*
+ * If we have changed parameters, propagate that info.
+ *
+ * Note: ExecReScanSetParamPlan() can add bits to node->chgParam,
+ * corresponding to the output param(s) that the InitPlan will update.
+ * Since we make only one pass over the list, that means that an InitPlan
+ * can depend on the output param(s) of a sibling InitPlan only if that
+ * sibling appears earlier in the list. This is workable for now given
+ * the limited ways in which one InitPlan could depend on another, but
+ * eventually we might need to work harder (or else make the planner
+ * enlarge the extParam/allParam sets to include the params of depended-on
+ * InitPlans).
+ */
if (node->chgParam != NULL)
{
ListCell *l;
diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c
index 88bea645531..240c529fa40 100644
--- a/src/backend/optimizer/plan/planagg.c
+++ b/src/backend/optimizer/plan/planagg.c
@@ -8,7 +8,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/optimizer/plan/planagg.c,v 1.39 2008/06/19 00:46:04 alvherre Exp $
+ * $PostgreSQL: pgsql/src/backend/optimizer/plan/planagg.c,v 1.40 2008/07/10 01:17:29 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -563,7 +563,11 @@ make_agg_subplan(PlannerInfo *root, MinMaxAggInfo *info)
exprType((Node *) tle->expr),
-1);
- /* Make sure the InitPlan gets into the outer list */
+ /*
+ * Make sure the InitPlan gets into the outer list. It has to appear
+ * after any other InitPlans it might depend on, too (see comments in
+ * ExecReScan).
+ */
root->init_plans = list_concat(root->init_plans, subroot.init_plans);
}
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 9fa838618aa..460b5ad1c76 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -7,7 +7,7 @@
* Portions Copyright (c) 1994, Regents of the University of California
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/optimizer/plan/subselect.c,v 1.130 2008/04/21 20:54:15 tgl Exp $
+ * $PostgreSQL: pgsql/src/backend/optimizer/plan/subselect.c,v 1.131 2008/07/10 01:17:29 tgl Exp $
*
*-------------------------------------------------------------------------
*/
@@ -47,8 +47,7 @@ typedef struct process_sublinks_context
typedef struct finalize_primnode_context
{
PlannerInfo *root;
- Bitmapset *paramids; /* Set of PARAM_EXEC paramids found */
- Bitmapset *outer_params; /* Set of accessible outer paramids */
+ Bitmapset *paramids; /* Non-local PARAM_EXEC paramids found */
} finalize_primnode_context;
@@ -68,7 +67,6 @@ static Node *process_sublinks_mutator(Node *node,
process_sublinks_context *context);
static Bitmapset *finalize_plan(PlannerInfo *root,
Plan *plan,
- Bitmapset *outer_params,
Bitmapset *valid_params);
static bool finalize_primnode(Node *node, finalize_primnode_context *context);
@@ -394,17 +392,21 @@ make_subplan(PlannerInfo *root, SubLink *slink, Node *testexpr, bool isTopQual)
}
else
{
- List *params;
List *args;
ListCell *l;
- /* Adjust the Params */
- params = generate_subquery_params(root,
- plan->targetlist,
- &splan->paramIds);
- splan->testexpr = convert_testexpr(root,
- testexpr,
- params);
+ if (testexpr)
+ {
+ List *params;
+
+ /* Adjust the Params in the testexpr */
+ params = generate_subquery_params(root,
+ plan->targetlist,
+ &splan->paramIds);
+ splan->testexpr = convert_testexpr(root,
+ testexpr,
+ params);
+ }
/*
* We can't convert subplans of ALL_SUBLINK or ANY_SUBLINK types to
@@ -1031,8 +1033,7 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
void
SS_finalize_plan(PlannerInfo *root, Plan *plan)
{
- Bitmapset *outer_params,
- *valid_params,
+ Bitmapset *valid_params,
*initExtParam,
*initSetParam;
Cost initplan_cost;
@@ -1042,9 +1043,12 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
/*
* First, scan the param list to discover the sets of params that are
* available from outer query levels and my own query level. We do this
- * once to save time in the per-plan recursion steps.
+ * once to save time in the per-plan recursion steps. (This calculation
+ * is overly generous: it can include a lot of params that actually
+ * shouldn't be referenced here. However, valid_params is just used as
+ * a debugging crosscheck, so it's not worth trying to be exact.)
*/
- outer_params = valid_params = NULL;
+ valid_params = NULL;
paramid = 0;
foreach(l, root->glob->paramlist)
{
@@ -1053,7 +1057,6 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
if (pitem->abslevel < root->query_level)
{
/* valid outer-level parameter */
- outer_params = bms_add_member(outer_params, paramid);
valid_params = bms_add_member(valid_params, paramid);
}
else if (pitem->abslevel == root->query_level &&
@@ -1069,9 +1072,8 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
/*
* Now recurse through plan tree.
*/
- (void) finalize_plan(root, plan, outer_params, valid_params);
+ (void) finalize_plan(root, plan, valid_params);
- bms_free(outer_params);
bms_free(valid_params);
/*
@@ -1107,11 +1109,13 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
/* allParam must include all these params */
plan->allParam = bms_add_members(plan->allParam, initExtParam);
plan->allParam = bms_add_members(plan->allParam, initSetParam);
+ /* extParam must include any child extParam */
+ plan->extParam = bms_add_members(plan->extParam, initExtParam);
/* but extParam shouldn't include any setParams */
- initExtParam = bms_del_members(initExtParam, initSetParam);
- /* empty test ensures extParam is exactly NULL if it's empty */
- if (!bms_is_empty(initExtParam))
- plan->extParam = bms_join(plan->extParam, initExtParam);
+ plan->extParam = bms_del_members(plan->extParam, initSetParam);
+ /* ensure extParam is exactly NULL if it's empty */
+ if (bms_is_empty(plan->extParam))
+ plan->extParam = NULL;
plan->startup_cost += initplan_cost;
plan->total_cost += initplan_cost;
@@ -1124,8 +1128,7 @@ SS_finalize_plan(PlannerInfo *root, Plan *plan)
* This is just an internal notational convenience.
*/
static Bitmapset *
-finalize_plan(PlannerInfo *root, Plan *plan,
- Bitmapset *outer_params, Bitmapset *valid_params)
+finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params)
{
finalize_primnode_context context;
@@ -1134,7 +1137,6 @@ finalize_plan(PlannerInfo *root, Plan *plan,
context.root = root;
context.paramids = NULL; /* initialize set to empty */
- context.outer_params = outer_params;
/*
* When we call finalize_primnode, context.paramids sets are automatically
@@ -1218,7 +1220,6 @@ finalize_plan(PlannerInfo *root, Plan *plan,
bms_add_members(context.paramids,
finalize_plan(root,
(Plan *) lfirst(l),
- outer_params,
valid_params));
}
}
@@ -1234,7 +1235,6 @@ finalize_plan(PlannerInfo *root, Plan *plan,
bms_add_members(context.paramids,
finalize_plan(root,
(Plan *) lfirst(l),
- outer_params,
valid_params));
}
}
@@ -1250,7 +1250,6 @@ finalize_plan(PlannerInfo *root, Plan *plan,
bms_add_members(context.paramids,
finalize_plan(root,
(Plan *) lfirst(l),
- outer_params,
valid_params));
}
}
@@ -1301,13 +1300,11 @@ finalize_plan(PlannerInfo *root, Plan *plan,
context.paramids = bms_add_members(context.paramids,
finalize_plan(root,
plan->lefttree,
- outer_params,
valid_params));
context.paramids = bms_add_members(context.paramids,
finalize_plan(root,
plan->righttree,
- outer_params,
valid_params));
/* Now we have all the paramids */
@@ -1315,22 +1312,24 @@ finalize_plan(PlannerInfo *root, Plan *plan,
if (!bms_is_subset(context.paramids, valid_params))
elog(ERROR, "plan should not reference subplan's variable");
- plan->extParam = bms_intersect(context.paramids, outer_params);
- plan->allParam = context.paramids;
-
/*
+ * Note: by definition, extParam and allParam should have the same value
+ * in any plan node that doesn't have child initPlans. We set them
+ * equal here, and later SS_finalize_plan will update them properly
+ * in node(s) that it attaches initPlans to.
+ *
* For speed at execution time, make sure extParam/allParam are actually
* NULL if they are empty sets.
*/
- if (bms_is_empty(plan->extParam))
+ if (bms_is_empty(context.paramids))
{
- bms_free(plan->extParam);
plan->extParam = NULL;
+ plan->allParam = NULL;
}
- if (bms_is_empty(plan->allParam))
+ else
{
- bms_free(plan->allParam);
- plan->allParam = NULL;
+ plan->extParam = context.paramids;
+ plan->allParam = bms_copy(context.paramids);
}
return plan->allParam;
@@ -1359,12 +1358,41 @@ finalize_primnode(Node *node, finalize_primnode_context *context)
{
SubPlan *subplan = (SubPlan *) node;
Plan *plan = planner_subplan_get_plan(context->root, subplan);
+ ListCell *lc;
+ Bitmapset *subparamids;
+
+ /* Recurse into the testexpr, but not into the Plan */
+ finalize_primnode(subplan->testexpr, context);
+
+ /*
+ * Remove any param IDs of output parameters of the subplan that were
+ * referenced in the testexpr. These are not interesting for
+ * parameter change signaling since we always re-evaluate the subplan.
+ * Note that this wouldn't work too well if there might be uses of the
+ * same param IDs elsewhere in the plan, but that can't happen because
+ * generate_new_param never tries to merge params.
+ */
+ foreach(lc, subplan->paramIds)
+ {
+ context->paramids = bms_del_member(context->paramids,
+ lfirst_int(lc));
+ }
- /* Add outer-level params needed by the subplan to paramids */
- context->paramids = bms_join(context->paramids,
- bms_intersect(plan->extParam,
- context->outer_params));
- /* fall through to recurse into subplan args */
+ /* Also examine args list */
+ finalize_primnode((Node *) subplan->args, context);
+
+ /*
+ * Add params needed by the subplan to paramids, but excluding those
+ * we will pass down to it.
+ */
+ subparamids = bms_copy(plan->extParam);
+ foreach(lc, subplan->parParam)
+ {
+ subparamids = bms_del_member(subparamids, lfirst_int(lc));
+ }
+ context->paramids = bms_join(context->paramids, subparamids);
+
+ return false; /* no more to do here */
}
return expression_tree_walker(node, finalize_primnode,
(void *) context);
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index 4bd0b4afbb5..f6dbc0212cd 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -438,3 +438,30 @@ select * from numeric_table
3
(4 rows)
+--
+-- Test case for bug #4290: bogus calculation of subplan param sets
+--
+create temp table ta (id int primary key, val int);
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "ta_pkey" for table "ta"
+insert into ta values(1,1);
+insert into ta values(2,2);
+create temp table tb (id int primary key, aval int);
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "tb_pkey" for table "tb"
+insert into tb values(1,1);
+insert into tb values(2,1);
+insert into tb values(3,2);
+insert into tb values(4,2);
+create temp table tc (id int primary key, aid int);
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "tc_pkey" for table "tc"
+insert into tc values(1,1);
+insert into tc values(2,2);
+select
+ ( select min(tb.id) from tb
+ where tb.aval = (select ta.val from ta where ta.id = tc.aid) ) as min_tb_id
+from tc;
+ min_tb_id
+-----------
+ 1
+ 3
+(2 rows)
+
diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index d99bf321bdd..3a3f11793dc 100644
--- a/src/test/regress/sql/subselect.sql
+++ b/src/test/regress/sql/subselect.sql
@@ -272,3 +272,29 @@ select * from float_table
select * from numeric_table
where num_col in (select float_col from float_table);
+
+--
+-- Test case for bug #4290: bogus calculation of subplan param sets
+--
+
+create temp table ta (id int primary key, val int);
+
+insert into ta values(1,1);
+insert into ta values(2,2);
+
+create temp table tb (id int primary key, aval int);
+
+insert into tb values(1,1);
+insert into tb values(2,1);
+insert into tb values(3,2);
+insert into tb values(4,2);
+
+create temp table tc (id int primary key, aid int);
+
+insert into tc values(1,1);
+insert into tc values(2,2);
+
+select
+ ( select min(tb.id) from tb
+ where tb.aval = (select ta.val from ta where ta.id = tc.aid) ) as min_tb_id
+from tc;
--
GitLab