8000 Fix planner problems with LATERAL references in PlaceHolderVars. · sqlparser/postgres@517db49 · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit 517db49

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 2505aae commit 517db49

File tree

25 files changed

+838
-309
lines changed

25 files changed

+838
-309
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 3 additions & 28 deletions
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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1917,8 +1917,8 @@ _copyLateralJoinInfo(const LateralJoinInfo *from)
19171917
{
19181918
LateralJoinInfo *newnode = makeNode(LateralJoinInfo);
19191919

1920-
COPY_SCALAR_FIELD(lateral_rhs);
19211920
COPY_BITMAPSET_FIELD(lateral_lhs);
1921+
COPY_BITMAPSET_FIELD(lateral_rhs);
19221922

19231923
return newnode;
19241924
}
@@ -1952,6 +1952,7 @@ _copyPlaceHolderInfo(const PlaceHolderInfo *from)
19521952
COPY_SCALAR_FIELD(phid);
19531953
COPY_NODE_FIELD(ph_var);
19541954
COPY_BITMAPSET_FIELD(ph_eval_at);
1955+
COPY_BITMAPSET_FIELD(ph_lateral);
19551956
COPY_BITMAPSET_FIELD(ph_needed);
19561957
COPY_SCALAR_FIELD(ph_width);
19571958

‎src/backend/nodes/equalfuncs.c

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

@@ -794,8 +798,8 @@ _equalSpecialJoinInfo(const SpecialJoinInfo *a, const SpecialJoinInfo *b)
794798
static bool
795799
_equalLateralJoinInfo(const LateralJoinInfo *a, const LateralJoinInfo *b)
796800
{
797-
COMPARE_SCALAR_FIELD(lateral_rhs);
798801
COMPARE_BITMAPSET_FIELD(lateral_lhs);
802+
COMPARE_BITMAPSET_FIELD(lateral_rhs);
799803

800804
return true;
801805
}
@@ -817,8 +821,9 @@ static bool
817821
_equalPlaceHolderInfo(const PlaceHolderInfo *a, const PlaceHolderInfo *b)
818822
{
819823
COMPARE_SCALAR_FIELD(phid);
820-
COMPARE_NODE_FIELD(ph_var);
824+
COMPARE_NODE_FIELD(ph_var); /* should be redundant */
821825
COMPARE_BITMAPSET_FIELD(ph_eval_at);
826+
COMPARE_BITMAPSET_FIELD(ph_lateral);
822827
COMPARE_BITMAPSET_FIELD(ph_needed);
823828
COMPARE_SCALAR_FIELD(ph_width);
824829

‎src/backend/nodes/outfuncs.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1752,6 +1752,7 @@ _outRelOptInfo(StringInfo str, const RelOptInfo *node)
17521752
WRITE_INT_FIELD(max_attr);
17531753
WRITE_NODE_FIELD(lateral_vars);
17541754
WRITE_BITMAPSET_FIELD(lateral_relids);
1755+
WRITE_BITMAPSET_FIELD(lateral_referencers);
17551756
WRITE_NODE_FIELD(indexlist);
17561757
WRITE_UINT_FIELD(pages);
17571758
WRITE_FLOAT_FIELD(tuples, "%.0f");
@@ -1909,8 +1910,8 @@ _outLateralJoinInfo(StringInfo str, const LateralJoinInfo *node)
19091910
{
19101911
WRITE_NODE_TYPE("LATERALJOININFO");
19111912

1912-
WRITE_UINT_FIELD(lateral_rhs);
19131913
WRITE_BITMAPSET_FIELD(lateral_lhs);
1914+
WRITE_BITMAPSET_FIELD(lateral_rhs);
19141915
}
19151916

19161917
static void
@@ -1934,6 +1935,7 @@ _outPlaceHolderInfo(StringInfo str, const PlaceHolderInfo *node)
19341935
WRITE_UINT_FIELD(phid);
19351936
WRITE_NODE_FIELD(ph_var);
19361937
WRITE_BITMAPSET_FIELD(ph_eval_at);
1938+
WRITE_BITMAPSET_FIELD(ph_lateral);
19371939
WRITE_BITMAPSET_FIELD(ph_needed);
19381940
WRITE_INT_FIELD(ph_width);
19391941
}

‎src/backend/optimizer/README

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

Lines changed: 6 additions & 10 deletions
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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3935,10 +3935,9 @@ set_rel_width(PlannerInfo *root, RelOptInfo *rel)
39353935

39363936
/*
39373937
* Ordinarily, a Var in a rel's reltargetlist must belong to that rel;
3938-
* but there are corner cases involving LATERAL references in
3939-
* appendrel members where that isn't so (see set_append_rel_size()).
3940-
* If the Var has the wrong varno, fall through to the generic case
3941-
* (it doesn't seem worth the trouble to be any smarter).
3938+
* but there are corner cases involving LATERAL references where that
3939+
* isn't so. If the Var has the wrong varno, fall through to the
3940+
* generic case (it doesn't seem worth the trouble to be any smarter).
39423941
*/
39433942
if (IsA(node, Var) &&
39443943
((Var *) node)->varno == rel->relid)

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

Lines changed: 13 additions & 39 deletions
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 4024

26502624
clauselist = lappend(clauselist, rinfo);

0 commit comments

Comments
 (0)
0