Skip to content

Commit 2977a31

Browse files
tglsfdctglsfdc
committed
Avoid sharing PARAM_EXEC slots between different levels of NestLoop.
Up to now, createplan.c attempted to share PARAM_EXEC slots for NestLoopParams across different plan levels, if the same underlying Var was being fed down to different righthand-side subplan trees by different NestLoops. This was, I think, more of an artifact of using subselect.c's PlannerParamItem infrastructure than an explicit design goal, but anyway that was the end result. This works well enough as long as the plan tree is executing synchronously, but the feature whereby Gather can execute the parallelized subplan locally breaks it. An upper NestLoop node might execute for a row retrieved from a parallel worker, and assign a value for a PARAM_EXEC slot from that row, while the leader's copy of the parallelized subplan is suspended with a different active value of the row the Var comes from. When control eventually returns to the leader's subplan, it gets the wrong answers if the same PARAM_EXEC slot is being used within the subplan, as reported in bug #15577 from Bartosz Polnik. This is pretty reminiscent of the problem fixed in commit 46c508f, and the proper fix seems to be the same: don't try to share PARAM_EXEC slots across different levels of controlling NestLoop nodes. This requires decoupling NestLoopParam handling from PlannerParamItem handling, although the logic remains somewhat similar. To avoid bizarre division of labor between subselect.c and createplan.c, I decided to move all the param-slot-assignment logic for both cases out of those files and put it into a new file paramassign.c. Hopefully it's a bit better documented now, too. A regression test case for this might be nice, but we don't know a test case that triggers the problem with a suitably small amount of data. Back-patch to 9.6 where we added Gather nodes. It's conceivable that related problems exist in older branches; but without some evidence for that, I'll leave the older branches alone. Discussion: https://postgr.es/m/15577-ca61ab18904af852@postgresql.org
1 parent cbb3e08 commit 2977a31

File tree

7 files changed

+666
-520
lines changed

7 files changed

+666
-520
lines changed

‎src/backend/optimizer/plan/createplan.c

Lines changed: 10 additions & 172 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
#include <limits.h>
2020
#include <math.h>
2121

22-
#include "access/stratnum.h"
2322
#include "access/sysattr.h"
2423
#include "catalog/pg_class.h"
2524
#include "foreign/fdwapi.h"
@@ -29,11 +28,11 @@
2928
#include "nodes/nodeFuncs.h"
3029
#include "optimizer/clauses.h"
3130
#include "optimizer/cost.h"
31+
#include "optimizer/paramassign.h"
3232
#include "optimizer/paths.h"
3333
#include "optimizer/placeholder.h"
3434
#include "optimizer/plancat.h"
3535
#include "optimizer/planmain.h"
36-
#include "optimizer/planner.h"
3736
#include "optimizer/predtest.h"
3837
#include "optimizer/restrictinfo.h"
3938
#include "optimizer/subselect.h"
@@ -153,8 +152,6 @@ static MergeJoin *create_mergejoin_plan(PlannerInfo *root, MergePath *best_path)
153152
static HashJoin *create_hashjoin_plan(PlannerInfo *root, HashPath *best_path);
154153
static Node *replace_nestloop_params(PlannerInfo *root, Node *expr);
155154
static Node *replace_nestloop_params_mutator(Node *node, PlannerInfo *root);
156-
static void process_subquery_nestloop_params(PlannerInfo *root,
157-
List *subplan_params);
158155
static List *fix_indexqual_references(PlannerInfo *root, IndexPath *index_path);
159156
static List *fix_indexorderby_references(PlannerInfo *root, IndexPath *index_path);
160157
static Node *fix_indexqual_operand(Node *node, IndexOptInfo *index, int indexcol);
@@ -307,7 +304,7 @@ create_plan(PlannerInfo *root, Path *best_path)
307304
/* plan_params should not be in use in current query level */
308305
Assert(root->plan_params == NIL);
309306

310-
/* Initialize this module's private workspace in PlannerInfo */
307+
/* Initialize this module's workspace in PlannerInfo */
311308
root->curOuterRels = NULL;
312309
root->curOuterParams = NIL;
313310

@@ -1464,7 +1461,7 @@ create_gather_plan(PlannerInfo *root, GatherPath *best_path)
14641461
gather_plan = make_gather(tlist,
14651462
NIL,
14661463
best_path->num_workers,
1467-
SS_assign_special_param(root),
1464+
assign_special_exec_param(root),
14681465
best_path->single_copy,
14691466
subplan);
14701467

@@ -1500,7 +1497,7 @@ create_gather_merge_plan(PlannerInfo *root, GatherMergePath *best_path)
15001497
copy_generic_path_info(&gm_plan->plan, &best_path->path);
15011498

15021499
/* Assign the rescan Param. */
1503-
gm_plan->rescan_param = SS_assign_special_param(root);
1500+
gm_plan->rescan_param = assign_special_exec_param(root);
15041501

15051502
/* Gather Merge is pointless with no pathkeys; use Gather instead. */
15061503
Assert(pathkeys != NIL);
@@ -3684,9 +3681,6 @@ create_nestloop_plan(PlannerInfo *root,
36843681
Relids outerrelids;
36853682
List *nestParams;
36863683
Relids saveOuterRels = root->curOuterRels;
3687-
ListCell *cell;
3688-
ListCell *prev;
3689-
ListCell *next;
36903684

36913685
/* NestLoop can project, so no need to be picky about child tlists */
36923686
outer_plan = create_plan_recurse(root, best_path->outerjoinpath, 0);
@@ -3730,38 +3724,10 @@ create_nestloop_plan(PlannerInfo *root,
37303724

37313725
/*
37323726
* Identify any nestloop parameters that should be supplied by this join
3733-
* node, and move them from root->curOuterParams to the nestParams list.
3727+
* node, and remove them from root->curOuterParams.
37343728
*/
37353729
outerrelids = best_path->outerjoinpath->parent->relids;
3736-
nestParams = NIL;
3737-
prev = NULL;
3738-
for (cell = list_head(root->curOuterParams); cell; cell = next)
3739-
{
3740-
NestLoopParam *nlp = (NestLoopParam *) lfirst(cell);
3741-
3742-
next = lnext(cell);
3743-
if (IsA(nlp->paramval, Var) &&
3744-
bms_is_member(nlp->paramval->varno, outerrelids))
3745-
{
3746-
root->curOuterParams = list_delete_cell(root->curOuterParams,
3747-
cell, prev);
3748-
nestParams = lappend(nestParams, nlp);
3749-
}
3750-
else if (IsA(nlp->paramval, PlaceHolderVar) &&
3751-
bms_overlap(((PlaceHolderVar *) nlp->paramval)->phrels,
3752-
outerrelids) &&
3753-
bms_is_subset(find_placeholder_info(root,
3754-
(PlaceHolderVar *) nlp->paramval,
3755-
false)->ph_eval_at,
3756-
outerrelids))
3757-
{
3758-
root->curOuterParams = list_delete_cell(root->curOuterParams,
3759-
cell, prev);
3760-
nestParams = lappend(nestParams, nlp);
3761-
}
3762-
else
3763-
prev = cell;
3764-
}
3730+
nestParams = identify_current_nestloop_params(root, outerrelids);
37653731

37663732
join_plan = make_nestloop(tlist,
37673733
joinclauses,
@@ -4244,42 +4210,18 @@ replace_nestloop_params_mutator(Node *node, PlannerInfo *root)
42444210
if (IsA(node, Var))
42454211
{
42464212
Var *var = (Var *) node;
4247-
Param *param;
4248-
NestLoopParam *nlp;
4249-
ListCell *lc;
42504213

42514214
/* Upper-level Vars should be long gone at this point */
42524215
Assert(var->varlevelsup == 0);
42534216
/* If not to be replaced, we can just return the Var unmodified */
42544217
if (!bms_is_member(var->varno, root->curOuterRels))
42554218
return node;
4256-
/* Create a Param representing the Var */
4257-
param = assign_nestloop_param_var(root, var);
4258-
/* Is this param already listed in root->curOuterParams? */
4259-
foreach(lc, root->curOuterParams)
4260-
{
4261-
nlp = (NestLoopParam *) lfirst(lc);
4262-
if (nlp->paramno == param->paramid)
4263-
{
4264-
Assert(equal(var, nlp->paramval));
4265-
/* Present, so we can just return the Param */
4266-
return (Node *) param;
4267-
}
4268-
}
4269-
/* No, so add it */
4270-
nlp = makeNode(NestLoopParam);
4271-
nlp->paramno = param->paramid;
4272-
nlp->paramval = var;
4273-
root->curOuterParams = lappend(root->curOuterParams, nlp);
4274-
/* And return the replacement Param */
4275-
return (Node *) param;
4219+
/* Replace the Var with a nestloop Param */
4220+
return (Node *) replace_nestloop_param_var(root, var);
42764221
}
42774222
if (IsA(node, PlaceHolderVar))
42784223
{
42794224
PlaceHolderVar *phv = (PlaceHolderVar *) node;
4280-
Param *param;
4281-
NestLoopParam *nlp;
4282-
ListCell *lc;
42834225

42844226
/* Upper-level PlaceHolderVars should be long gone at this point */
42854227
Assert(phv->phlevelsup == 0);
@@ -4316,118 +4258,14 @@ replace_nestloop_params_mutator(Node *node, PlannerInfo *root)
43164258
root);
43174259
return (Node *) newphv;
43184260
}
4319-
/* Create a Param representing the PlaceHolderVar */
4320-
param = assign_nestloop_param_placeholdervar(root, phv);
4321-
/* Is this param already listed in root->curOuterParams? */
4322-
foreach(lc, root->curOuterParams)
4323-
{
4324-
nlp = (NestLoopParam *) lfirst(lc);
4325-
if (nlp->paramno == param->paramid)
4326-
{
4327-
Assert(equal(phv, nlp->paramval));
4328-
/* Present, so we can just return the Param */
4329-
return (Node *) param;
4330-
}
4331-
}
4332-
/* No, so add it */
4333-
nlp = makeNode(NestLoopParam);
4334-
nlp->paramno = param->paramid;
4335-
nlp->paramval = (Var *) phv;
4336-
root->curOuterParams = lappend(root->curOuterParams, nlp);
4337-
/* And return the replacement Param */
4338-
return (Node *) param;
4261+
/* Replace the PlaceHolderVar with a nestloop Param */
4262+
return (Node *) replace_nestloop_param_placeholdervar(root, phv);
43394263
}
43404264
return expression_tree_mutator(node,
43414265
replace_nestloop_params_mutator,
43424266
(void *) root);
43434267
}
43444268

4345-
/*
4346-
* process_subquery_nestloop_params
4347-
* Handle params of a parameterized subquery that need to be fed
4348-
* from an outer nestloop.
4349-
*
4350-
* Currently, that would be *all* params that a subquery in FROM has demanded
4351-
* from the current query level, since they must be LATERAL references.
4352-
*
4353-
* The subplan's references to the outer variables are already represented
4354-
* as PARAM_EXEC Params, so we need not modify the subplan here. What we
4355-
* do need to do is add entries to root->curOuterParams to signal the parent
4356-
* nestloop plan node that it must provide these values.
4357-
*/
4358-
static void
4359-
process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
4360-
{
4361-
ListCell *ppl;
4362-
4363-
foreach(ppl, subplan_params)
4364-
{
4365-
PlannerParamItem *pitem = (PlannerParamItem *) lfirst(ppl);
4366-
4367-
if (IsA(pitem->item, Var))
4368-
{
4369-
Var *var = (Var *) pitem->item;
4370-
NestLoopParam *nlp;
4371-
ListCell *lc;
4372-
4373-
/* If not from a nestloop outer rel, complain */
4374-
if (!bms_is_member(var->varno, root->curOuterRels))
4375-
elog(ERROR, "non-LATERAL parameter required by subquery");
4376-
/* Is this param already listed in root->curOuterParams? */
4377-
foreach(lc, root->curOuterParams)
4378-
{
4379-
nlp = (NestLoopParam *) lfirst(lc);
4380-
if (nlp->paramno == pitem->paramId)
4381-
{
4382-
Assert(equal(var, nlp->paramval));
4383-
/* Present, so nothing to do */
4384-
break;
4385-
}
4386-
}
4387-
if (lc == NULL)
4388-
{
4389-
/* No, so add it */
4390-
nlp = makeNode(NestLoopParam);
4391-
nlp->paramno = pitem->paramId;
4392-
nlp->paramval = copyObject(var);
4393-
root->curOuterParams = lappend(root->curOuterParams, nlp);
4394-
}
4395-
}
4396-
else if (IsA(pitem->item, PlaceHolderVar))
4397-
{
4398-
PlaceHolderVar *phv = (PlaceHolderVar *) pitem->item;
4399-
NestLoopParam *nlp;
4400-
ListCell *lc;
4401-
4402-
/* If not from a nestloop outer rel, complain */
4403-
if (!bms_is_subset(find_placeholder_info(root, phv, false)->ph_eval_at,
4404-
root->curOuterRels))
4405-
elog(ERROR, "non-LATERAL parameter required by subquery");
4406-
/* Is this param already listed in root->curOuterParams? */
4407-
foreach(lc, root->curOuterParams)
4408-
{
4409-
nlp = (NestLoopParam *) lfirst(lc);
4410-
if (nlp->paramno == pitem->paramId)
4411-
{
4412-
Assert(equal(phv, nlp->paramval));
4413-
/* Present, so nothing to do */
4414-
break;
4415-
}
4416-
}
4417-
if (lc == NULL)
4418-
{
4419-
/* No, so add it */
4420-
nlp = makeNode(NestLoopParam);
4421-
nlp->paramno = pitem->paramId;
4422-
nlp->paramval = (Var *) copyObject(phv);
4423-
root->curOuterParams = lappend(root->curOuterParams, nlp);
4424-
}
4425-
}
4426-
else
4427-
elog(ERROR, "unexpected type of subquery parameter");
4428-
}
4429-
}
4430-
44314269
/*
44324270
* fix_indexqual_references
44334271
* Adjust indexqual clauses to the form the executor's indexqual

‎src/backend/optimizer/plan/planner.c

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
#endif
3939
#include "optimizer/clauses.h"
4040
#include "optimizer/cost.h"
41+
#include "optimizer/paramassign.h"
4142
#include "optimizer/pathnode.h"
4243
#include "optimizer/paths.h"
4344
#include "optimizer/plancat.h"
@@ -522,7 +523,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
522523
root->hasInheritedTarget = false;
523524
root->hasRecursion = hasRecursion;
524525
if (hasRecursion)
525-
root->wt_param_id = SS_assign_special_param(root);
526+
root->wt_param_id = assign_special_exec_param(root);
526527
else
527528
root->wt_param_id = -1;
528529
root->non_recursive_path = NULL;
@@ -1449,7 +1450,7 @@ inheritance_planner(PlannerInfo *root)
14491450
returningLists,
14501451
rowMarks,
14511452
NULL,
1452-
SS_assign_special_param(root)));
1453+
assign_special_exec_param(root)));
14531454
}
14541455

14551456
/*--------------------
@@ -1999,7 +2000,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
19992000
{
20002001
path = (Path *) create_lockrows_path(root, final_rel, path,
20012002
root->rowMarks,
2002-
SS_assign_special_param(root));
2003+
assign_special_exec_param(root));
20032004
}
20042005

20052006
/*
@@ -2060,7 +2061,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
20602061
returningLists,
20612062
rowMarks,
20622063
parse->onConflict,
2063-
SS_assign_special_param(root));
2064+
assign_special_exec_param(root));
20642065
}
20652066

20662067
/* And shove it into final_rel */

0 commit comments

Comments
 (0)