Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 2e822a1

Browse files
committed
Apply band-aid fix for an oversight in reparameterize_path_by_child.
The path we wish to reparameterize is not a standalone object: in particular, it implicitly references baserestrictinfo clauses in the associated RelOptInfo, and if it's a SampleScan path then there is also the TableSampleClause in the RTE to worry about. Both of those could contain lateral references to the join partner relation, which would need to be modified to refer to its child. Since we aren't doing that, affected queries can give wrong answers, or odd failures such as "variable not found in subplan target list", or executor crashes. But we can't just summarily modify those expressions, because they are shared with other paths for the rel. We'd break things if we modify them and then end up using some non-partitioned-join path. In HEAD, we plan to fix this by postponing reparameterization until create_plan(), when we know that those other paths are no longer of interest, and then adjusting those expressions along with the ones in the path itself. That seems like too big a change for stable branches however. In the back branches, let's just detect whether any troublesome lateral references actually exist in those expressions, and fail reparameterization if so. This will result in not performing a partitioned join in such cases. Given the lack of field complaints, nobody's likely to miss the optimization. Report and patch by Richard Guo. Apply to 12-16 only, since the intended fix for HEAD looks quite different. We're not quite ready to push the HEAD fix, but with back-branch releases coming up soon, it seems wise to get this stopgap fix in place there. Discussion: https://postgr.es/m/CAMbWs496+N=UAjOc=rcD3P7B6oJe4rZw08e_TZRUsWbPxZW3Tw@mail.gmail.com
1 parent 43eb5c6 commit 2e822a1

File tree

3 files changed

+390
-0
lines changed

3 files changed

+390
-0
lines changed

src/backend/optimizer/util/pathnode.c

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "optimizer/optimizer.h"
2727
#include "optimizer/pathnode.h"
2828
#include "optimizer/paths.h"
29+
#include "optimizer/placeholder.h"
2930
#include "optimizer/planmain.h"
3031
#include "optimizer/prep.h"
3132
#include "optimizer/restrictinfo.h"
@@ -57,6 +58,10 @@ static int append_startup_cost_compare(const void *a, const void *b);
5758
static List *reparameterize_pathlist_by_child(PlannerInfo *root,
5859
List *pathlist,
5960
RelOptInfo *child_rel);
61+
static bool contain_references_to(PlannerInfo *root, Node *clause,
62+
Relids relids);
63+
static bool ris_contain_references_to(PlannerInfo *root, List *rinfos,
64+
Relids relids);
6065

6166

6267
/*****************************************************************************
@@ -3915,13 +3920,59 @@ do { \
39153920
switch (nodeTag(path))
39163921
{
39173922
case T_Path:
3923+
3924+
/*
3925+
* If the path's restriction clauses contain lateral references to
3926+
* the other relation, we can't reparameterize, because we must
3927+
* not change the RelOptInfo's contents here. (Doing so would
3928+
* break things if we end up using a non-partitionwise join.)
3929+
*/
3930+
if (ris_contain_references_to(root,
3931+
path->parent->baserestrictinfo,
3932+
child_rel->top_parent_relids))
3933+
return NULL;
3934+
3935+
/*
3936+
* If it's a SampleScan with tablesample parameters referencing
3937+
* the other relation, we can't reparameterize, because we must
3938+
* not change the RTE's contents here. (Doing so would break
3939+
* things if we end up using a non-partitionwise join.)
3940+
*/
3941+
if (path->pathtype == T_SampleScan)
3942+
{
3943+
Index scan_relid = path->parent->relid;
3944+
RangeTblEntry *rte;
3945+
3946+
/* it should be a base rel with a tablesample clause... */
3947+
Assert(scan_relid > 0);
3948+
rte = planner_rt_fetch(scan_relid, root);
3949+
Assert(rte->rtekind == RTE_RELATION);
3950+
Assert(rte->tablesample != NULL);
3951+
3952+
if (contain_references_to(root, (Node *) rte->tablesample,
3953+
child_rel->top_parent_relids))
3954+
return NULL;
3955+
}
3956+
39183957
FLAT_COPY_PATH(new_path, path, Path);
39193958
break;
39203959

39213960
case T_IndexPath:
39223961
{
39233962
IndexPath *ipath;
39243963

3964+
/*
3965+
* If the path's restriction clauses contain lateral
3966+
* references to the other relation, we can't reparameterize,
3967+
* because we must not change the IndexOptInfo's contents
3968+
* here. (Doing so would break things if we end up using a
3969+
* non-partitionwise join.)
3970+
*/
3971+
if (ris_contain_references_to(root,
3972+
path->parent->baserestrictinfo,
3973+
child_rel->top_parent_relids))
3974+
return NULL;
3975+
39253976
FLAT_COPY_PATH(ipath, path, IndexPath);
39263977
ADJUST_CHILD_ATTRS(ipath->indexclauses);
39273978
new_path = (Path *) ipath;
@@ -3932,6 +3983,18 @@ do { \
39323983
{
39333984
BitmapHeapPath *bhpath;
39343985

3986+
/*
3987+
* If the path's restriction clauses contain lateral
3988+
* references to the other relation, we can't reparameterize,
3989+
* because we must not change the RelOptInfo's contents here.
3990+
* (Doing so would break things if we end up using a
3991+
* non-partitionwise join.)
3992+
*/
3993+
if (ris_contain_references_to(root,
3994+
path->parent->baserestrictinfo,
3995+
child_rel->top_parent_relids))
3996+
return NULL;
3997+
39353998
FLAT_COPY_PATH(bhpath, path, BitmapHeapPath);
39363999
REPARAMETERIZE_CHILD_PATH(bhpath->bitmapqual);
39374000
new_path = (Path *) bhpath;
@@ -3963,6 +4026,18 @@ do { \
39634026
ForeignPath *fpath;
39644027
ReparameterizeForeignPathByChild_function rfpc_func;
39654028

4029+
/*
4030+
* If the path's restriction clauses contain lateral
4031+
* references to the other relation, we can't reparameterize,
4032+
* because we must not change the RelOptInfo's contents here.
4033+
* (Doing so would break things if we end up using a
4034+
* non-partitionwise join.)
4035+
*/
4036+
if (ris_contain_references_to(root,
4037+
path->parent->baserestrictinfo,
4038+
child_rel->top_parent_relids))
4039+
return NULL;
4040+
39664041
FLAT_COPY_PATH(fpath, path, ForeignPath);
39674042
if (fpath->fdw_outerpath)
39684043
REPARAMETERIZE_CHILD_PATH(fpath->fdw_outerpath);
@@ -3981,6 +4056,18 @@ do { \
39814056
{
39824057
CustomPath *cpath;
39834058

4059+
/*
4060+
* If the path's restriction clauses contain lateral
4061+
* references to the other relation, we can't reparameterize,
4062+
* because we must not change the RelOptInfo's contents here.
4063+
* (Doing so would break things if we end up using a
4064+
* non-partitionwise join.)
4065+
*/
4066+
if (ris_contain_references_to(root,
4067+
path->parent->baserestrictinfo,
4068+
child_rel->top_parent_relids))
4069+
return NULL;
4070+
39844071
FLAT_COPY_PATH(cpath, path, CustomPath);
39854072
REPARAMETERIZE_CHILD_PATH_LIST(cpath->custom_paths);
39864073
if (cpath->methods &&
@@ -4146,3 +4233,91 @@ reparameterize_pathlist_by_child(PlannerInfo *root,
41464233

41474234
return result;
41484235
}
4236+
4237+
/*
4238+
* contain_references_to
4239+
* Detect whether any Vars or PlaceHolderVars in the given clause contain
4240+
* lateral references to the given 'relids'.
4241+
*/
4242+
static bool
4243+
contain_references_to(PlannerInfo *root, Node *clause, Relids relids)
4244+
{
4245+
bool ret = false;
4246+
List *vars;
4247+
ListCell *lc;
4248+
4249+
/*
4250+
* Examine all Vars and PlaceHolderVars used in the clause.
4251+
*
4252+
* By omitting the relevant flags, this also gives us a cheap sanity check
4253+
* that no aggregates or window functions appear in the clause. We don't
4254+
* expect any of those in scan-level restrictions or tablesamples.
4255+
*/
4256+
vars = pull_var_clause(clause, PVC_INCLUDE_PLACEHOLDERS);
4257+
foreach(lc, vars)
4258+
{
4259+
Node *node = (Node *) lfirst(lc);
4260+
4261+
if (IsA(node, Var))
4262+
{
4263+
Var *var = (Var *) node;
4264+
4265+
if (bms_is_member(var->varno, relids))
4266+
{
4267+
ret = true;
4268+
break;
4269+
}
4270+
}
4271+
else if (IsA(node, PlaceHolderVar))
4272+
{
4273+
PlaceHolderVar *phv = (PlaceHolderVar *) node;
4274+
PlaceHolderInfo *phinfo = find_placeholder_info(root, phv, false);
4275+
4276+
/*
4277+
* We should check both ph_eval_at (in case the PHV is to be
4278+
* computed at the other relation and then laterally referenced
4279+
* here) and ph_lateral (in case the PHV is to be evaluated here
4280+
* but contains lateral references to the other relation). The
4281+
* former case should not occur in baserestrictinfo clauses, but
4282+
* it can occur in tablesample clauses.
4283+
*/
4284+
if (bms_overlap(phinfo->ph_eval_at, relids) ||
4285+
bms_overlap(phinfo->ph_lateral, relids))
4286+
{
4287+
ret = true;
4288+
break;
4289+
}
4290+
}
4291+
else
4292+
Assert(false);
4293+
}
4294+
4295+
list_free(vars);
4296+
4297+
return ret;
4298+
}
4299+
4300+
/*
4301+
* ris_contain_references_to
4302+
* Apply contain_references_to() to a list of RestrictInfos.
4303+
*
4304+
* We need extra code for this because pull_var_clause() can't descend
4305+
* through RestrictInfos.
4306+
*/
4307+
static bool
4308+
ris_contain_references_to(PlannerInfo *root, List *rinfos, Relids relids)
4309+
{
4310+
ListCell *lc;
4311+
4312+
foreach(lc, rinfos)
4313+
{
4314+
RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
4315+
4316+
/* Pseudoconstant clauses can't contain any Vars or PHVs */
4317+
if (rinfo->pseudoconstant)
4318+
continue;
4319+
if (contain_references_to(root, (Node *) rinfo->clause, relids))
4320+
return true;
4321+
}
4322+
return false;
4323+
}

0 commit comments

Comments
 (0)