Skip to content

Commit 9e7e29c

Browse files
tglsfdctglsfdc
committed
Fix planner problems with LATERAL references in PlaceHolderVars.
The planner largely failed to consider the possibility that a PlaceHolderVar's expression might contain a lateral reference to a Var coming from somewhere outside the PHV's syntactic scope. We had a previous report of a problem in this area, which I tried to fix in a quick-hack way in commit 4da6439, but Antonin Houska pointed out that there were still some problems, and investigation turned up other issues. This patch largely reverts that commit in favor of a more thoroughly thought-through solution. The new theory is that a PHV's ph_eval_at level cannot be higher than its original syntactic level. If it contains lateral references, those don't change the ph_eval_at level, but rather they create a lateral-reference requirement for the ph_eval_at join relation. The code in joinpath.c needs to handle that. Another issue is that createplan.c wasn't handling nested PlaceHolderVars properly. In passing, push knowledge of lateral-reference checks for join clauses into join_clause_is_movable_to. This is mainly so that FDWs don't need to deal with it. This patch doesn't fix the original join-qual-placement problem reported by Jeremy Evans (and indeed, one of the new regression test cases shows the wrong answer because of that). But the PlaceHolderVar problems need to be fixed before that issue can be addressed, so committing this separately seems reasonable.
1 parent 175ec8d commit 9e7e29c

25 files changed

+838
-309
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

+3-28
Original file line numberDiff line numberDiff line change
@@ -540,7 +540,6 @@ postgresGetForeignPaths(PlannerInfo *root,
540540
{
541541
PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) baserel->fdw_private;
542542
ForeignPath *path;
543-
Relids lateral_referencers;
544543
List *join_quals;
545544
Relids required_outer;
546545
double rows;
@@ -579,34 +578,13 @@ postgresGetForeignPaths(PlannerInfo *root,
579578
* consider combinations of clauses, probably.
580579
*/
581580

582-
/*
583-
* If there are any rels that have LATERAL references to this one, we
584-
* cannot use join quals referencing them as remote quals for this one,
585-
* since such rels would have to be on the inside not the outside of a
586-
* nestloop join relative to this one. Create a Relids set listing all
587-
* such rels, for use in checks of potential join clauses.
588-
*/
589-
lateral_referencers = NULL;
590-
foreach(lc, root->lateral_info_list)
591-
{
592-
LateralJoinInfo *ljinfo = (LateralJoinInfo *) lfirst(lc);
593-
594-
if (bms_is_member(baserel->relid, ljinfo->lateral_lhs))
595-
lateral_referencers = bms_add_member(lateral_referencers,
596-
ljinfo->lateral_rhs);
597-
}
598-
599581
/* Scan the rel's join clauses */
600582
foreach(lc, baserel->joininfo)
601583
{
602584
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
603585

604586
/* Check if clause can be moved to this rel */
605-
if (!join_clause_is_movable_to(rinfo, baserel->relid))
606-
continue;
607-
608-
/* Not useful if it conflicts with any LATERAL references */
609-
if (bms_overlap(rinfo->clause_relids, lateral_referencers))
587+
if (!join_clause_is_movable_to(rinfo, baserel))
610588
continue;
611589

612590
/* See if it is safe to send to remote */
@@ -667,7 +645,7 @@ postgresGetForeignPaths(PlannerInfo *root,
667645
baserel,
668646
ec_member_matches_foreign,
669647
(void *) &arg,
670-
lateral_referencers);
648+
baserel->lateral_referencers);
671649

672650
/* Done if there are no more expressions in the foreign rel */
673651
if (arg.current == NULL)
@@ -682,12 +660,9 @@ postgresGetForeignPaths(PlannerInfo *root,
682660
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
683661

684662
/* Check if clause can be moved to this rel */
685-
if (!join_clause_is_movable_to(rinfo, baserel->relid))
663+
if (!join_clause_is_movable_to(rinfo, baserel))
686664
continue;
687665

688-
/* Shouldn't conflict with any LATERAL references */
689-
Assert(!bms_overlap(rinfo->clause_relids, lateral_referencers));
690-
691666
/* See if it is safe to send to remote */
692667
if (!is_foreign_expr(root, baserel, rinfo->clause))
693668
continue;

‎src/backend/nodes/copyfuncs.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -1921,8 +1921,8 @@ _copyLateralJoinInfo(const LateralJoinInfo *from)
19211921
{
19221922
LateralJoinInfo *newnode = makeNode(LateralJoinInfo);
19231923

1924-
COPY_SCALAR_FIELD(lateral_rhs);
19251924
COPY_BITMAPSET_FIELD(lateral_lhs);
1925+
COPY_BITMAPSET_FIELD(lateral_rhs);
19261926

19271927
return newnode;
19281928
}
@@ -1956,6 +1956,7 @@ _copyPlaceHolderInfo(const PlaceHolderInfo *from)
19561956
COPY_SCALAR_FIELD(phid);
19571957
COPY_NODE_FIELD(ph_var);
19581958
COPY_BITMAPSET_FIELD(ph_eval_at);
1959+
COPY_BITMAPSET_FIELD(ph_lateral);
19591960
COPY_BITMAPSET_FIELD(ph_needed);
19601961
COPY_SCALAR_FIELD(ph_width);
19611962

‎src/backend/nodes/equalfuncs.c

+13-8
Original file line numberDiff line numberDiff line change
@@ -763,15 +763,19 @@ _equalPlaceHolderVar(const PlaceHolderVar *a, const PlaceHolderVar *b)
763763
/*
764764
* We intentionally do not compare phexpr. Two PlaceHolderVars with the
765765
* same ID and levelsup should be considered equal even if the contained
766-
* expressions have managed to mutate to different states. One way in
767-
* which that can happen is that initplan sublinks would get replaced by
768-
* differently-numbered Params when sublink folding is done. (The end
769-
* result of such a situation would be some unreferenced initplans, which
770-
* is annoying but not really a problem.)
766+
* expressions have managed to mutate to different states. This will
767+
* happen during final plan construction when there are nested PHVs, since
768+
* the inner PHV will get replaced by a Param in some copies of the outer
769+
* PHV. Another way in which it can happen is that initplan sublinks
770+
* could get replaced by differently-numbered Params when sublink folding
771+
* is done. (The end result of such a situation would be some
772+
* unreferenced initplans, which is annoying but not really a problem.) On
773+
* the same reasoning, there is no need to examine phrels.
771774
*
772775
* COMPARE_NODE_FIELD(phexpr);
776+
*
777+
* COMPARE_BITMAPSET_FIELD(phrels);
773778
*/
774-
COMPARE_BITMAPSET_FIELD(phrels);
775779
COMPARE_SCALAR_FIELD(phid);
776780
COMPARE_SCALAR_FIELD(phlevelsup);
777781

@@ -796,8 +800,8 @@ _equalSpecialJoinInfo(const SpecialJoinInfo *a, const SpecialJoinInfo *b)
796800
static bool
797801
_equalLateralJoinInfo(const LateralJoinInfo *a, const LateralJoinInfo *b)
798802
{
799-
COMPARE_SCALAR_FIELD(lateral_rhs);
800803
COMPARE_BITMAPSET_FIELD(lateral_lhs);
804+
COMPARE_BITMAPSET_FIELD(lateral_rhs);
801805

802806
return true;
803807
}
@@ -819,8 +823,9 @@ static bool
819823
_equalPlaceHolderInfo(const PlaceHolderInfo *a, const PlaceHolderInfo *b)
820824
{
821825
COMPARE_SCALAR_FIELD(phid);
822-
COMPARE_NODE_FIELD(ph_var);
826+
COMPARE_NODE_FIELD(ph_var); /* should be redundant */
823827
COMPARE_BITMAPSET_FIELD(ph_eval_at);
828+
COMPARE_BITMAPSET_FIELD(ph_lateral);
824829
COMPARE_BITMAPSET_FIELD(ph_needed);
825830
COMPARE_SCALAR_FIELD(ph_width);
826831

‎src/backend/nodes/outfuncs.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -1756,6 +1756,7 @@ _outRelOptInfo(StringInfo str, const RelOptInfo *node)
17561756
WRITE_INT_FIELD(max_attr);
17571757
WRITE_NODE_FIELD(lateral_vars);
17581758
WRITE_BITMAPSET_FIELD(lateral_relids);
1759+
WRITE_BITMAPSET_FIELD(lateral_referencers);
17591760
WRITE_NODE_FIELD(indexlist);
17601761
WRITE_UINT_FIELD(pages);
17611762
WRITE_FLOAT_FIELD(tuples, "%.0f");
@@ -1913,8 +1914,8 @@ _outLateralJoinInfo(StringInfo str, const LateralJoinInfo *node)
19131914
{
19141915
WRITE_NODE_TYPE("LATERALJOININFO");
19151916

1916-
WRITE_UINT_FIELD(lateral_rhs);
19171917
WRITE_BITMAPSET_FIELD(lateral_lhs);
1918+
WRITE_BITMAPSET_FIELD(lateral_rhs);
19181919
}
19191920

19201921
static void
@@ -1938,6 +1939,7 @@ _outPlaceHolderInfo(StringInfo str, const PlaceHolderInfo *node)
19381939
WRITE_UINT_FIELD(phid);
19391940
WRITE_NODE_FIELD(ph_var);
19401941
WRITE_BITMAPSET_FIELD(ph_eval_at);
1942+
WRITE_BITMAPSET_FIELD(ph_lateral);
19411943
WRITE_BITMAPSET_FIELD(ph_needed);
19421944
WRITE_INT_FIELD(ph_width);
19431945
}

‎src/backend/optimizer/README

+14
Original file line numberDiff line numberDiff line change
@@ -802,5 +802,19 @@ parameterized paths still apply, though; in particular, each such path is
802802
still expected to enforce any join clauses that can be pushed down to it,
803803
so that all paths of the same parameterization have the same rowcount.
804804

805+
We also allow LATERAL subqueries to be flattened (pulled up into the parent
806+
query) by the optimizer, but only when they don't contain any lateral
807+
references to relations outside the lowest outer join that can null the
808+
LATERAL subquery. This restriction prevents lateral references from being
809+
introduced into outer-join qualifications, which would create semantic
810+
confusion. Note that even with this restriction, pullup of a LATERAL
811+
subquery can result in creating PlaceHolderVars that contain lateral
812+
references to relations outside their syntactic scope. We still evaluate
813+
such PHVs at their syntactic location or lower, but the presence of such a
814+
PHV in the quals or targetlist of a plan node requires that node to appear
815+
on the inside of a nestloop join relative to the rel(s) supplying the
816+
lateral reference. (Perhaps now that that stuff works, we could relax the
817+
pullup restriction?)
818+
805819

806820
-- bjm & tgl

‎src/backend/optimizer/path/allpaths.c

+6-10
Original file line numberDiff line numberDiff line change
@@ -386,8 +386,7 @@ set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
386386
/*
387387
* We don't support pushing join clauses into the quals of a seqscan, but
388388
* it could still have required parameterization due to LATERAL refs in
389-
* its tlist. (That can only happen if the seqscan is on a relation
390-
* pulled up out of a UNION ALL appendrel.)
389+
* its tlist.
391390
*/
392391
required_outer = rel->lateral_relids;
393392

@@ -550,8 +549,8 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
550549
* Note: the resulting childrel->reltargetlist may contain arbitrary
551550
* expressions, which otherwise would not occur in a reltargetlist.
552551
* Code that might be looking at an appendrel child must cope with
553-
* such. Note in particular that "arbitrary expression" can include
554-
* "Var belonging to another relation", due to LATERAL references.
552+
* such. (Normally, a reltargetlist would only include Vars and
553+
* PlaceHolderVars.)
555554
*/
556555
childrel->joininfo = (List *)
557556
adjust_appendrel_attrs(root,
@@ -1355,8 +1354,7 @@ set_cte_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
13551354
/*
13561355
* We don't support pushing join clauses into the quals of a CTE scan, but
13571356
* it could still have required parameterization due to LATERAL refs in
1358-
* its tlist. (That can only happen if the CTE scan is on a relation
1359-
* pulled up out of a UNION ALL appendrel.)
1357+
* its tlist.
13601358
*/
13611359
required_outer = rel->lateral_relids;
13621360

@@ -1408,10 +1406,8 @@ set_worktable_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
14081406
/*
14091407
* We don't support pushing join clauses into the quals of a worktable
14101408
* scan, but it could still have required parameterization due to LATERAL
1411-
* refs in its tlist. (That can only happen if the worktable scan is on a
1412-
* relation pulled up out of a UNION ALL appendrel. I'm not sure this is
1413-
* actually possible given the restrictions on recursive references, but
1414-
* it's easy enough to support.)
1409+
* refs in its tlist. (I'm not sure this is actually possible given the
1410+
* restrictions on recursive references, but it's easy enough to support.)
14151411
*/
14161412
required_outer = rel->lateral_relids;
14171413

‎src/backend/optimizer/path/costsize.c

+3-4
Original file line numberDiff line numberDiff line change
@@ -3943,10 +3943,9 @@ set_rel_width(PlannerInfo *root, RelOptInfo *rel)
39433943

39443944
/*
39453945
* Ordinarily, a Var in a rel's reltargetlist must belong to that rel;
3946-
* but there are corner cases involving LATERAL references in
3947-
* appendrel members where that isn't so (see set_append_rel_size()).
3948-
* If the Var has the wrong varno, fall through to the generic case
3949-
* (it doesn't seem worth the trouble to be any smarter).
3946+
* but there are corner cases involving LATERAL references where that
3947+
* isn't so. If the Var has the wrong varno, fall through to the
3948+
* generic case (it doesn't seem worth the trouble to be any smarter).
39503949
*/
39513950
if (IsA(node, Var) &&
39523951
((Var *) node)->varno == rel->relid)

‎src/backend/optimizer/path/indxpath.c

+13-39
Original file line numberDiff line numberDiff line change
@@ -141,12 +141,10 @@ static void match_restriction_clauses_to_index(RelOptInfo *rel,
141141
IndexClauseSet *clauseset);
142142
static void match_join_clauses_to_index(PlannerInfo *root,
143143
RelOptInfo *rel, IndexOptInfo *index,
144-
Relids lateral_referencers,
145144
IndexClauseSet *clauseset,
146145
List **joinorclauses);
147146
static void match_eclass_clauses_to_index(PlannerInfo *root,
148147
IndexOptInfo *index,
149-
Relids lateral_referencers,
150148
IndexClauseSet *clauseset);
151149
static void match_clauses_to_index(IndexOptInfo *index,
152150
List *clauses,
@@ -220,14 +218,14 @@ static Const *string_to_const(const char *str, Oid datatype);
220218
*
221219
* Note: check_partial_indexes() must have been run previously for this rel.
222220
*
223-
* Note: in corner cases involving LATERAL appendrel children, it's possible
224-
* that rel->lateral_relids is nonempty. Currently, we include lateral_relids
225-
* into the parameterization reported for each path, but don't take it into
226-
* account otherwise. The fact that any such rels *must* be available as
227-
* parameter sources perhaps should influence our choices of index quals ...
228-
* but for now, it doesn't seem worth troubling over. In particular, comments
229-
* below about "unparameterized" paths should be read as meaning
230-
* "unparameterized so far as the indexquals are concerned".
221+
* Note: in cases involving LATERAL references in the relation's tlist, it's
222+
* possible that rel->lateral_relids is nonempty. Currently, we include
223+
* lateral_relids into the parameterization reported for each path, but don't
224+
* take it into account otherwise. The fact that any such rels *must* be
225+
* available as parameter sources perhaps should influence our choices of
226+
* index quals ... but for now, it doesn't seem worth troubling over.
227+
* In particular, comments below about "unparameterized" paths should be read
228+
* as meaning "unparameterized so far as the indexquals are concerned".
231229
*/
232230
void
233231
create_index_paths(PlannerInfo *root, RelOptInfo *rel)
@@ -236,7 +234,6 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
236234
List *bitindexpaths;
237235
List *bitjoinpaths;
238236
List *joinorclauses;
239-
Relids lateral_referencers;
240237
IndexClauseSet rclauseset;
241238
IndexClauseSet jclauseset;
242239
IndexClauseSet eclauseset;
@@ -246,23 +243,6 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
246243
if (rel->indexlist == NIL)
247244
return;
248245

249-
/*
250-
* If there are any rels that have LATERAL references to this one, we
251-
* cannot use join quals referencing them as index quals for this one,
252-
* since such rels would have to be on the inside not the outside of a
253-
* nestloop join relative to this one. Create a Relids set listing all
254-
* such rels, for use in checks of potential join clauses.
255-
*/
256-
lateral_referencers = NULL;
257-
foreach(lc, root->lateral_info_list)
258-
{
259-
LateralJoinInfo *ljinfo = (LateralJoinInfo *) lfirst(lc);
260-
261-
if (bms_is_member(rel->relid, ljinfo->lateral_lhs))
262-
lateral_referencers = bms_add_member(lateral_referencers,
263-
ljinfo->lateral_rhs);
264-
}
265-
266246
/* Bitmap paths are collected and then dealt with at the end */
267247
bitindexpaths = bitjoinpaths = joinorclauses = NIL;
268248

@@ -303,15 +283,15 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
303283
* EquivalenceClasses. Also, collect join OR clauses for later.
304284
*/
305285
MemSet(&jclauseset, 0, sizeof(jclauseset));
306-
match_join_clauses_to_index(root, rel, index, lateral_referencers,
286+
match_join_clauses_to_index(root, rel, index,
307287
&jclauseset, &joinorclauses);
308288

309289
/*
310290
* Look for EquivalenceClasses that can generate joinclauses matching
311291
* the index.
312292
*/
313293
MemSet(&eclauseset, 0, sizeof(eclauseset));
314-
match_eclass_clauses_to_index(root, index, lateral_referencers,
294+
match_eclass_clauses_to_index(root, index,
315295
&eclauseset);
316296

317297
/*
@@ -1957,7 +1937,6 @@ match_restriction_clauses_to_index(RelOptInfo *rel, IndexOptInfo *index,
19571937
static void
19581938
match_join_clauses_to_index(PlannerInfo *root,
19591939
RelOptInfo *rel, IndexOptInfo *index,
1960-
Relids lateral_referencers,
19611940
IndexClauseSet *clauseset,
19621941
List **joinorclauses)
19631942
{
@@ -1969,11 +1948,7 @@ match_join_clauses_to_index(PlannerInfo *root,
19691948
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
19701949

19711950
/* Check if clause can be moved to this rel */
1972-
if (!join_clause_is_movable_to(rinfo, rel->relid))
1973-
continue;
1974-
1975-
/* Not useful if it conflicts with any LATERAL references */
1976-
if (bms_overlap(rinfo->clause_relids, lateral_referencers))
1951+
if (!join_clause_is_movable_to(rinfo, rel))
19771952
continue;
19781953

19791954
/* Potentially usable, so see if it matches the index or is an OR */
@@ -1991,7 +1966,6 @@ match_join_clauses_to_index(PlannerInfo *root,
19911966
*/
19921967
static void
19931968
match_eclass_clauses_to_index(PlannerInfo *root, IndexOptInfo *index,
1994-
Relids lateral_referencers,
19951969
IndexClauseSet *clauseset)
19961970
{
19971971
int indexcol;
@@ -2012,7 +1986,7 @@ match_eclass_clauses_to_index(PlannerInfo *root, IndexOptInfo *index,
20121986
index->rel,
20131987
ec_member_matches_indexcol,
20141988
(void *) &arg,
2015-
lateral_referencers);
1989+
index->rel->lateral_referencers);
20161990

20171991
/*
20181992
* We have to check whether the results actually do match the index,
@@ -2644,7 +2618,7 @@ check_partial_indexes(PlannerInfo *root, RelOptInfo *rel)
26442618
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
26452619

26462620
/* Check if clause can be moved to this rel */
2647-
if (!join_clause_is_movable_to(rinfo, rel->relid))
2621+
if (!join_clause_is_movable_to(rinfo, rel))
26482622
continue;
26492623

26502624
clauselist = lappend(clauselist, rinfo);

0 commit comments

Comments
 (0)