Обсуждение: BUG #18953: Planner fails to build plan for complex query with LATERAL references
BUG #18953: Planner fails to build plan for complex query with LATERAL references
От
PG Bug reporting form
Дата:
The following bug has been logged on the website:
Bug reference: 18953
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 18beta1
Operating system: Ubuntu 24.04
Description:
The following query:
create table tbl1(a int);
create table tbl2(b int);
create table tbl3(c int);
select * from tbl1 left join
(select case when a = 0 then 0 else subq_3.cc end from tbl1,
lateral (select 1 from tbl2 t1, tbl2 t2, tbl2 t3, tbl2 t4) subq_1,
lateral (
select tbl3.c as cc from tbl3, tbl2 t1, tbl2 t2,
lateral (select c, a from tbl2 limit 1) subq_2
) as subq_3
) subq_4 on true;
ends up with:
ERROR: XX000: failed to build any 4-way joins
LOCATION: standard_join_search, allpaths.c:3527
where N in "N-way" depends on the number of tbl2 t2, tbl2 t3, ... in
subq_1.
This is a simplified version of a query generated by SQLsmith.
The first commit this error is raised on is acfcd45ca. On acfcd45ca~1, the
select fails with "ERROR: could not devise a query plan for the given
query". The last commit it executed with no error is 4200a9286.
On Wed, Jun 11, 2025 at 3:14 AM PG Bug reporting form
<noreply@postgresql.org> wrote:
> The following query:
> create table tbl1(a int);
> create table tbl2(b int);
> create table tbl3(c int);
> select * from tbl1 left join
> (select case when a = 0 then 0 else subq_3.cc end from tbl1,
> lateral (select 1 from tbl2 t1, tbl2 t2, tbl2 t3, tbl2 t4) subq_1,
> lateral (
> select tbl3.c as cc from tbl3, tbl2 t1, tbl2 t2,
> lateral (select c, a from tbl2 limit 1) subq_2
> ) as subq_3
> ) subq_4 on true;
> ends up with:
> ERROR: XX000: failed to build any 4-way joins
Thanks for the report. Here's a simplified repro.
create table t (a int);
set from_collapse_limit to 2;
select * from t t1 left join
(select coalesce(a+x) from t t2,
lateral (select t3.a as x from t t3, lateral (select t2.a, t3.a
offset 0) s))
on true;
ERROR: failed to build any 2-way joins
In this query, the join between t3 and s is placed into a separate
join sub-problem due to the from_collapse_limit. This join is deemed
not legal by join_is_legal(), as have_dangerous_phv() thinks the PHV
could pose a hazard as described in that function's comment. As a
result, no join could be built for this sub-problem.
It seems to me that there are some loose ends in the logic of
have_dangerous_phv(). In its comment, it says:
* (Note that we can still make use of A's parameterized
* path with pre-joined B+C as the outer rel. have_join_order_restriction()
* ensures that we will consider making such a join even if there are not
* other reasons to do so.)
However, if B and C end up in different sub-joinlists, it becomes
impossible to pre-join B+C.
In the query above, the PHV's minimum eval_at set includes t2 and t3.
But since t2 and t3 belong to different sub-joinlists, we have no way
to pre-join t2+t3 first and then join the result with s.
No idea how to fix this though. Any thoughts?
Thanks
Richard
On Wed, Jun 11, 2025 at 5:33 PM Richard Guo <guofenglinux@gmail.com> wrote: > In this query, the join between t3 and s is placed into a separate > join sub-problem due to the from_collapse_limit. This join is deemed > not legal by join_is_legal(), as have_dangerous_phv() thinks the PHV > could pose a hazard as described in that function's comment. As a > result, no join could be built for this sub-problem. > No idea how to fix this though. Any thoughts? This might be a silly idea, but if we can't find a valid plan for a join sub-problem, perhaps we could consider flattening the sub-joinlist into the higher level to explore a solution in a broader search space. The top-most joinlist should always be able to produce a valid plan, otherwise something must be wrong during planning. This may violate the from_collapse_limit restriction, but such cases are expected to be very rare. Thanks Richard
Richard Guo <guofenglinux@gmail.com> writes:
> Thanks for the report. Here's a simplified repro.
> ...
> In this query, the join between t3 and s is placed into a separate
> join sub-problem due to the from_collapse_limit. This join is deemed
> not legal by join_is_legal(), as have_dangerous_phv() thinks the PHV
> could pose a hazard as described in that function's comment. As a
> result, no join could be built for this sub-problem.
Bleah.
> No idea how to fix this though. Any thoughts?
My thought is that have_dangerous_phv() was never more than a
quick-n-dirty kludge, and what we really ought to do is remove it.
That means cleaning up the technical debt mentioned in 85e5e222b:
In principle we could allow such a PlaceHolderVar to be evaluated at the
lower join node using values passed down from the upper relation along with
values from the join's own outer relation. However, nodeNestloop.c only
supports simple Vars not arbitrary expressions as nestloop parameters.
createplan.c is also a few bricks shy of being able to handle such cases;
it misplaces the PlaceHolderVar parameters in the plan tree, which is why
the visible symptoms of this bug are "plan should not reference subplan's
variable" and "failed to assign all NestLoopParams to plan nodes" planner
errors.
Adding the necessary complexity to make this work doesn't seem like it
would be repaid in significantly better plans, because in cases where such
a PHV exists, there is probably a corresponding join order constraint that
would allow a good plan to be found without using the star-schema exception.
Furthermore, adding complexity to nodeNestloop.c would create a run-time
penalty even for plans where this whole consideration is irrelevant.
So let's just reject such paths instead.
I think that the argument about executor complexity might be a red
herring: if we can get the PHV to be evaluated in the tlist of the
nestloop's outer relation, then the reference to it will still just be
an outer Var in the NestLoopParam structure. I'm still poking at what
we'd have to do to the planner to get that to happen, but my initial
impression is that it might not be very complicated after all.
regards, tom lane
I wrote:
> My thought is that have_dangerous_phv() was never more than a
> quick-n-dirty kludge, and what we really ought to do is remove it.
Here's a WIP patch along that line. It's unfinished in that, for
testing purposes, I just lobotomized have_dangerous_phv() to return
constant false rather than taking it out entirely. But of course
we'd want to clean up all the dead code if we go this way.
I have mixed feelings about whether to back-patch or just make
this change in HEAD. While we're clearly fixing a bug here,
the bug's been there for 10 years, so the lack of field reports
suggests strongly that this is not something ordinary users write.
Two arguments against back-patching are that
(1) the odds of introducing a new bug aren't zero;
(2) removing the have_dangerous_phv() restriction will change
some plan choices, which we generally dislike doing in stable
branches.
I'm still comfortable with shoving this into v18, but maybe we
should leave the back branches alone.
regards, tom lane
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 60d65762b5d..d41702cd398 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -1308,22 +1308,6 @@ bool
have_dangerous_phv(PlannerInfo *root,
Relids outer_relids, Relids inner_params)
{
- ListCell *lc;
-
- foreach(lc, root->placeholder_list)
- {
- PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(lc);
-
- if (!bms_is_subset(phinfo->ph_eval_at, inner_params))
- continue; /* ignore, could not be a nestloop param */
- if (!bms_overlap(phinfo->ph_eval_at, outer_relids))
- continue; /* ignore, not relevant to this join */
- if (bms_is_subset(phinfo->ph_eval_at, outer_relids))
- continue; /* safe, it can be eval'd within outerrel */
- /* Otherwise, it's potentially unsafe, so reject the join */
- return true;
- }
-
/* OK to perform the join */
return false;
}
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 4ad30b7627e..8baf36ba4b7 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4348,9 +4348,11 @@ create_nestloop_plan(PlannerInfo *root,
List *joinrestrictclauses = best_path->jpath.joinrestrictinfo;
List *joinclauses;
List *otherclauses;
- Relids outerrelids;
List *nestParams;
+ List *outer_tlist;
+ bool outer_parallel_safe;
Relids saveOuterRels = root->curOuterRels;
+ ListCell *lc;
/*
* If the inner path is parameterized by the topmost parent of the outer
@@ -4412,9 +4414,47 @@ create_nestloop_plan(PlannerInfo *root,
* Identify any nestloop parameters that should be supplied by this join
* node, and remove them from root->curOuterParams.
*/
- outerrelids = best_path->jpath.outerjoinpath->parent->relids;
- nestParams = identify_current_nestloop_params(root, outerrelids);
+ nestParams = identify_current_nestloop_params(root,
+ best_path->jpath.outerjoinpath);
+
+ /*
+ * While nestloop parameters that are Vars had better be available from
+ * the outer_plan already, there are edge cases where nestloop parameters
+ * that are PHVs won't be. In such cases we must add them to the
+ * outer_plan's tlist, since the executor's NestLoopParam machinery
+ * requires the params to be simple outer-Var references to that tlist.
+ */
+ outer_tlist = outer_plan->targetlist;
+ outer_parallel_safe = outer_plan->parallel_safe;
+ foreach(lc, nestParams)
+ {
+ NestLoopParam *nlp = (NestLoopParam *) lfirst(lc);
+ TargetEntry *tle;
+
+ if (IsA(nlp->paramval, Var))
+ continue; /* nothing to do for simple Vars */
+ if (tlist_member((Expr *) nlp->paramval, outer_tlist))
+ continue; /* already available */
+
+ /* Make a shallow copy of outer_tlist, if we didn't already */
+ if (outer_tlist == outer_plan->targetlist)
+ outer_tlist = list_copy(outer_tlist);
+ /* ... and add the needed expression */
+ tle = makeTargetEntry((Expr *) copyObject(nlp->paramval),
+ list_length(outer_tlist) + 1,
+ NULL,
+ true);
+ outer_tlist = lappend(outer_tlist, tle);
+ /* ... and track whether tlist is (still) parallel-safe */
+ if (outer_parallel_safe)
+ outer_parallel_safe = is_parallel_safe(root,
+ (Node *) nlp->paramval);
+ }
+ if (outer_tlist != outer_plan->targetlist)
+ outer_plan = change_plan_targetlist(outer_plan, outer_tlist,
+ outer_parallel_safe);
+ /* And finally, we can build the join plan node */
join_plan = make_nestloop(tlist,
joinclauses,
otherclauses,
diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c
index 3bd3ce37c8f..9836abf9479 100644
--- a/src/backend/optimizer/util/paramassign.c
+++ b/src/backend/optimizer/util/paramassign.c
@@ -600,7 +600,7 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
/*
* Identify any NestLoopParams that should be supplied by a NestLoop plan
- * node with the specified lefthand rels. Remove them from the active
+ * node with the specified lefthand input path. Remove them from the active
* root->curOuterParams list and return them as the result list.
*
* XXX Here we also hack up the returned Vars and PHVs so that they do not
@@ -626,11 +626,26 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
* subquery, which'd be unduly expensive.
*/
List *
-identify_current_nestloop_params(PlannerInfo *root, Relids leftrelids)
+identify_current_nestloop_params(PlannerInfo *root, Path *leftpath)
{
List *result;
+ Relids leftrelids = leftpath->parent->relids;
+ Relids outerrelids = PATH_REQ_OUTER(leftpath);
+ Relids allleftrelids;
ListCell *cell;
+ /*
+ * We'll be able to evaluate a PHV in the lefthand path if it uses the
+ * lefthand rels plus any available required-outer rels. But don't do so
+ * if it uses *only* required-outer rels; in that case it should be
+ * evaluated higher in the tree. For Vars, no such hair-splitting is
+ * necessary since they depend on only one relid.
+ */
+ if (outerrelids)
+ allleftrelids = bms_union(leftrelids, outerrelids);
+ else
+ allleftrelids = leftrelids;
+
result = NIL;
foreach(cell, root->curOuterParams)
{
@@ -653,18 +668,20 @@ identify_current_nestloop_params(PlannerInfo *root, Relids leftrelids)
leftrelids);
result = lappend(result, nlp);
}
- else if (IsA(nlp->paramval, PlaceHolderVar) &&
- bms_is_subset(find_placeholder_info(root,
- (PlaceHolderVar *) nlp->paramval)->ph_eval_at,
- leftrelids))
+ else if (IsA(nlp->paramval, PlaceHolderVar))
{
PlaceHolderVar *phv = (PlaceHolderVar *) nlp->paramval;
+ Relids eval_at = find_placeholder_info(root, phv)->ph_eval_at;
- root->curOuterParams = foreach_delete_current(root->curOuterParams,
- cell);
- phv->phnullingrels = bms_intersect(phv->phnullingrels,
- leftrelids);
- result = lappend(result, nlp);
+ if (bms_is_subset(eval_at, allleftrelids) &&
+ bms_overlap(eval_at, leftrelids))
+ {
+ root->curOuterParams = foreach_delete_current(root->curOuterParams,
+ cell);
+ phv->phnullingrels = bms_intersect(phv->phnullingrels,
+ leftrelids);
+ result = lappend(result, nlp);
+ }
}
}
return result;
diff --git a/src/include/optimizer/paramassign.h b/src/include/optimizer/paramassign.h
index 59dcb1ff053..d30d20de299 100644
--- a/src/include/optimizer/paramassign.h
+++ b/src/include/optimizer/paramassign.h
@@ -30,7 +30,7 @@ extern Param *replace_nestloop_param_placeholdervar(PlannerInfo *root,
extern void process_subquery_nestloop_params(PlannerInfo *root,
List *subplan_params);
extern List *identify_current_nestloop_params(PlannerInfo *root,
- Relids leftrelids);
+ Path *leftpath);
extern Param *generate_new_exec_param(PlannerInfo *root, Oid paramtype,
int32 paramtypmod, Oid paramcollation);
extern int assign_special_exec_param(PlannerInfo *root);
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index f35a0b18c37..dfff24edf03 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -4035,6 +4035,37 @@ select * from
1 | 2 | 2
(1 row)
+-- This example demonstrates the folly of our old "have_dangerous_phv" logic
+begin;
+set local from_collapse_limit to 2;
+explain (verbose, costs off)
+select * from int8_tbl t1
+ left join
+ (select coalesce(t2.q1 + x, 0) from int8_tbl t2,
+ lateral (select t3.q1 as x from int8_tbl t3,
+ lateral (select t2.q1, t3.q1 offset 0) s))
+ on true;
+ QUERY PLAN
+------------------------------------------------------------------
+ Nested Loop Left Join
+ Output: t1.q1, t1.q2, (COALESCE((t2.q1 + t3.q1), '0'::bigint))
+ -> Seq Scan on public.int8_tbl t1
+ Output: t1.q1, t1.q2
+ -> Materialize
+ Output: (COALESCE((t2.q1 + t3.q1), '0'::bigint))
+ -> Nested Loop
+ Output: COALESCE((t2.q1 + t3.q1), '0'::bigint)
+ -> Seq Scan on public.int8_tbl t2
+ Output: t2.q1, t2.q2
+ -> Nested Loop
+ Output: t3.q1
+ -> Seq Scan on public.int8_tbl t3
+ Output: t3.q1, t3.q2
+ -> Result
+ Output: NULL::bigint, NULL::bigint
+(16 rows)
+
+rollback;
-- Test proper handling of appendrel PHVs during useless-RTE removal
explain (costs off)
select * from
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index cc5128add4d..ebed8bdd152 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1332,6 +1332,18 @@ select * from
(select 1 as x) ss1 left join (select 2 as y) ss2 on (true),
lateral (select ss2.y as z limit 1) ss3;
+-- This example demonstrates the folly of our old "have_dangerous_phv" logic
+begin;
+set local from_collapse_limit to 2;
+explain (verbose, costs off)
+select * from int8_tbl t1
+ left join
+ (select coalesce(t2.q1 + x, 0) from int8_tbl t2,
+ lateral (select t3.q1 as x from int8_tbl t3,
+ lateral (select t2.q1, t3.q1 offset 0) s))
+ on true;
+rollback;
+
-- Test proper handling of appendrel PHVs during useless-RTE removal
explain (costs off)
select * from
On Sat, Jun 14, 2025 at 5:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Here's a WIP patch along that line. It's unfinished in that, for > testing purposes, I just lobotomized have_dangerous_phv() to return > constant false rather than taking it out entirely. But of course > we'd want to clean up all the dead code if we go this way. In identify_current_nestloop_params(), I wonder if we should use the join path's required-outer rels rather than the left path's as the "outerrelids". It seems that there may be cases where the join path is parameterized by some other rel while its left path itself is not parameterized at all. Regarding the test case, I wonder if we should add another test query to specifically test the changes in create_nestloop_plan(). In particular, a case where the inner rel A has a parameter that is a PHV, and that PHV's ph_eval_at includes the outer rel B and some third rel C. The expected plan should show the PHV being added to B's target list (such a case might be tricky to construct though). > I have mixed feelings about whether to back-patch or just make > this change in HEAD. While we're clearly fixing a bug here, > the bug's been there for 10 years, so the lack of field reports > suggests strongly that this is not something ordinary users write. > Two arguments against back-patching are that > (1) the odds of introducing a new bug aren't zero; > (2) removing the have_dangerous_phv() restriction will change > some plan choices, which we generally dislike doing in stable > branches. Agreed. The changes in this patch seem too risky to backport to the stable branches. Thanks Richard
Richard Guo <guofenglinux@gmail.com> writes:
> In identify_current_nestloop_params(), I wonder if we should use the
> join path's required-outer rels rather than the left path's as the
> "outerrelids". It seems that there may be cases where the join path
> is parameterized by some other rel while its left path itself is not
> parameterized at all.
I went back and forth on that. In principle, we shouldn't put such
a calculation into the left path if it's not parameterized by the
rel supplying the required value. In practice, if the required value
is getting passed down to the join path then it's available to the
left path too, so it should work fine whether the left path is marked
like that or not. In the only test case I have that exercises this
logic, both paths are marked with the required_outer anyway, so it
doesn't seem to matter. If we can find a case where that's not so,
then probably we should do it the other way.
> Regarding the test case, I wonder if we should add another test query
> to specifically test the changes in create_nestloop_plan().
We already have one; there's a query in join.sql (originally added to
test the have_dangerous_phv logic) that fails without the additions
to push a PHV into the left path. Admittedly, that's behind the
scenes so maybe we ought to add an EXPLAIN to make it more apparent.
regards, tom lane
On Tue, Jun 17, 2025 at 12:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Richard Guo <guofenglinux@gmail.com> writes: > > In identify_current_nestloop_params(), I wonder if we should use the > > join path's required-outer rels rather than the left path's as the > > "outerrelids". It seems that there may be cases where the join path > > is parameterized by some other rel while its left path itself is not > > parameterized at all. > I went back and forth on that. In principle, we shouldn't put such > a calculation into the left path if it's not parameterized by the > rel supplying the required value. In practice, if the required value > is getting passed down to the join path then it's available to the > left path too, so it should work fine whether the left path is marked > like that or not. In the only test case I have that exercises this > logic, both paths are marked with the required_outer anyway, so it > doesn't seem to matter. If we can find a case where that's not so, > then probably we should do it the other way. I tried to find such a case but didn't succeed; though I suspect that's simply because I haven't tried hard enough. Conceptually, I'm thinking of a query where 1) A laterally references both B and C while B does not reference any other relation, and 2) A has a PHV parameter whose ph_eval_at includes both B and C. In such a query, the B/A join path is parameterized by C, while its left path B is not parameterized at all. In order to add the PHV to B's tlist, we'll need to consider the join path's required-outer rels not just those of the left path. Thanks Richard
Richard Guo <guofenglinux@gmail.com> writes:
> On Tue, Jun 17, 2025 at 12:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In the only test case I have that exercises this
>> logic, both paths are marked with the required_outer anyway, so it
>> doesn't seem to matter. If we can find a case where that's not so,
>> then probably we should do it the other way.
> I tried to find such a case but didn't succeed; though I suspect
> that's simply because I haven't tried hard enough. Conceptually, I'm
> thinking of a query where 1) A laterally references both B and C while
> B does not reference any other relation, and 2) A has a PHV parameter
> whose ph_eval_at includes both B and C. In such a query, the B/A join
> path is parameterized by C, while its left path B is not parameterized
> at all. In order to add the PHV to B's tlist, we'll need to consider
> the join path's required-outer rels not just those of the left path.
After thinking about this for awhile, I'm not seeing how that could
happen. A PHV with ph_eval_at exceeding its syntactic scope could
only be created via something like
B left join lateral (select coalesce(B.x, ...) as Q from D) C
that is we need a non-strict targetlist expression that references
something outside the sub-select proper. In this case all paths
created for C will have required_outer mentioning B, because there's
no way to compute C's reltarget without an outer reference to B.
This might be embedded in
... join A on A.y = C.Q
which gives rise to the situation you describe --- but there's
no possibility of the planner trying to do this in the order
C join (B join A)
because it will think that all paths for C require B on the
outside. It will only consider
B join (C join A)
and the path for C will show B as required_outer.
So I'm inclined to leave that code as I had it. It's notationally
a bit simpler and it doesn't require assuming that we can ignore
the path's required_outer marking at this stage. If I'm wrong,
someone will eventually find a counterexample and we can fix it
then; the changes won't be large.
regards, tom lane
Re: BUG #18953: Planner fails to build plan for complex query with LATERAL references
От
Alexander Lakhin
Дата:
Hello Tom,
17.06.2025 19:29, Tom Lane wrote:
17.06.2025 19:29, Tom Lane wrote:
So I'm inclined to leave that code as I had it. It's notationally a bit simpler and it doesn't require assuming that we can ignore the path's required_outer marking at this stage. If I'm wrong, someone will eventually find a counterexample and we can fix it then; the changes won't be large.
Please look at the following (simplified version of a query generated by
SQLsmith), which produces errors after a16ef313f2:
CREATE TABLE t(b bool); ANALYZE t;
SELECT 1 FROM t
INNER JOIN (SELECT (SELECT true) AS c FROM t, LATERAL (SELECT b LIMIT 1)) ON true
RIGHT JOIN (SELECT true) ON true,
LATERAL (SELECT b WHERE c LIMIT 1)
WHERE t.b;
ERROR: XX000: unrecognized node type: 22
LOCATION: ExecInitExprRec, execExpr.c:2665
Or a slight variation:
SELECT 1 FROM t
INNER JOIN (SELECT (SELECT true) AS c FROM t, LATERAL (SELECT 1 LIMIT 1)) ON true
RIGHT JOIN (SELECT true) ON true,
LATERAL (SELECT b WHERE c LIMIT 1)
WHERE t.b;
ERROR: XX000: failed to assign all NestLoopParams to plan nodes
LOCATION: create_plan, createplan.c:372
Best regards,
Alexander
Alexander Lakhin <exclusion@gmail.com> writes:
> 17.06.2025 19:29, Tom Lane wrote:
>> So I'm inclined to leave that code as I had it. It's notationally
>> a bit simpler and it doesn't require assuming that we can ignore
>> the path's required_outer marking at this stage. If I'm wrong,
>> someone will eventually find a counterexample and we can fix it
>> then; the changes won't be large.
> Please look at the following (simplified version of a query generated by
> SQLsmith), which produces errors after a16ef313f2:
Hah, that didn't take long! Your second case is indeed a
counterexample to my argument. We end up with a PHV having
phrels (b 7 8) which needs to be evaluated here:
{NESTPATH
:jpath.path.pathtype 356
:parent_relids (b 1 6 7)
:required_outer (b 8)
...
:jpath.outerjoinpath
{PATH
:pathtype 339
:parent_relids (b 7)
:required_outer (b)
Doing things the way Richard wanted to (ie using the nestloop's
required_outer) gets past the "failed to assign all NestLoopParams"
error, but then we get the same "unrecognized node type: 22" error
as in the first example.
That error seems considerably nastier to fix. I think it is a
pre-existing problem, though I don't currently have an explanation
why we've not seen it reported before. What is happening is that
we pull up a PHV for "c", containing a SubLink for (SELECT true),
and only some copies of it get put through preprocess_expression,
which is responsible for converting SubLinks to SubPlans among
other essential tasks. It seems that up to now we've always
managed to use only preprocessed copies in the finished plan.
But now a not-preprocessed copy is managing to get through to
the executor, which promptly spits up because it doesn't know
what to do with a SubLink.
We could probably hack this in a localized way by ensuring that
we push a preprocessed copy of the PHV into the left plan's tlist.
(One way to get that would be to look in the placeholder_list for the
correct phid.) However, now that I've seen this I have very little
faith that there aren't other bugs of the same ilk, that somehow
we've not seen up to now. I'm thinking about what we must do to
ensure that all PHVs are preprocessed once and only once, perhaps
at the moment they get put into the placeholder_list.
I don't have a patch for that yet, but attached is something that
gets rid of the "failed to assign all NestLoopParams" problem.
regards, tom lane
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 8baf36ba4b7..81e16e87d62 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4344,6 +4344,7 @@ create_nestloop_plan(PlannerInfo *root,
NestLoop *join_plan;
Plan *outer_plan;
Plan *inner_plan;
+ Relids outerrelids;
List *tlist = build_path_tlist(root, &best_path->jpath.path);
List *joinrestrictclauses = best_path->jpath.joinrestrictinfo;
List *joinclauses;
@@ -4374,8 +4375,8 @@ create_nestloop_plan(PlannerInfo *root,
outer_plan = create_plan_recurse(root, best_path->jpath.outerjoinpath, 0);
/* For a nestloop, include outer relids in curOuterRels for inner side */
- root->curOuterRels = bms_union(root->curOuterRels,
- best_path->jpath.outerjoinpath->parent->relids);
+ outerrelids = best_path->jpath.outerjoinpath->parent->relids;
+ root->curOuterRels = bms_union(root->curOuterRels, outerrelids);
inner_plan = create_plan_recurse(root, best_path->jpath.innerjoinpath, 0);
@@ -4415,7 +4416,8 @@ create_nestloop_plan(PlannerInfo *root,
* node, and remove them from root->curOuterParams.
*/
nestParams = identify_current_nestloop_params(root,
- best_path->jpath.outerjoinpath);
+ outerrelids,
+ PATH_REQ_OUTER((Path *) best_path));
/*
* While nestloop parameters that are Vars had better be available from
@@ -4423,6 +4425,10 @@ create_nestloop_plan(PlannerInfo *root,
* that are PHVs won't be. In such cases we must add them to the
* outer_plan's tlist, since the executor's NestLoopParam machinery
* requires the params to be simple outer-Var references to that tlist.
+ * (This is cheating a little bit, because the outer path's required-outer
+ * relids might not be enough to allow evaluating such a PHV. But in
+ * practice, if we could have evaluated the PHV at the nestloop node, we
+ * can do so in the outer plan too.)
*/
outer_tlist = outer_plan->targetlist;
outer_parallel_safe = outer_plan->parallel_safe;
diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c
index 9836abf9479..5403ae89eab 100644
--- a/src/backend/optimizer/util/paramassign.c
+++ b/src/backend/optimizer/util/paramassign.c
@@ -599,9 +599,10 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
}
/*
- * Identify any NestLoopParams that should be supplied by a NestLoop plan
- * node with the specified lefthand input path. Remove them from the active
- * root->curOuterParams list and return them as the result list.
+ * Identify any NestLoopParams that should be supplied by a NestLoop
+ * plan node with the specified lefthand rels and required-outer rels.
+ * Remove them from the active root->curOuterParams list and return
+ * them as the result list.
*
* XXX Here we also hack up the returned Vars and PHVs so that they do not
* contain nullingrel sets exceeding what is available from the outer side.
@@ -626,11 +627,11 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
* subquery, which'd be unduly expensive.
*/
List *
-identify_current_nestloop_params(PlannerInfo *root, Path *leftpath)
+identify_current_nestloop_params(PlannerInfo *root,
+ Relids leftrelids,
+ Relids outerrelids)
{
List *result;
- Relids leftrelids = leftpath->parent->relids;
- Relids outerrelids = PATH_REQ_OUTER(leftpath);
Relids allleftrelids;
ListCell *cell;
diff --git a/src/include/optimizer/paramassign.h b/src/include/optimizer/paramassign.h
index d30d20de299..bbf7214289b 100644
--- a/src/include/optimizer/paramassign.h
+++ b/src/include/optimizer/paramassign.h
@@ -30,7 +30,8 @@ extern Param *replace_nestloop_param_placeholdervar(PlannerInfo *root,
extern void process_subquery_nestloop_params(PlannerInfo *root,
List *subplan_params);
extern List *identify_current_nestloop_params(PlannerInfo *root,
- Path *leftpath);
+ Relids leftrelids,
+ Relids outerrelids);
extern Param *generate_new_exec_param(PlannerInfo *root, Oid paramtype,
int32 paramtypmod, Oid paramcollation);
extern int assign_special_exec_param(PlannerInfo *root);
On Mon, Jun 23, 2025 at 3:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Lakhin <exclusion@gmail.com> writes:
> > 17.06.2025 19:29, Tom Lane wrote:
> >> So I'm inclined to leave that code as I had it. It's notationally
> >> a bit simpler and it doesn't require assuming that we can ignore
> >> the path's required_outer marking at this stage. If I'm wrong,
> >> someone will eventually find a counterexample and we can fix it
> >> then; the changes won't be large.
> > Please look at the following (simplified version of a query generated by
> > SQLsmith), which produces errors after a16ef313f2:
> Hah, that didn't take long! Your second case is indeed a
> counterexample to my argument.
FWIW, a16ef313f also causes GEQO to encounter the "failed to assign
all NestLoopParams" problem, which can be seen in one of our
regression test queries.
set geqo_threshold to 2;
regression=# explain (verbose, costs off)
select ss2.* from
int4_tbl i41
left join int8_tbl i8
join (select i42.f1 as c1, i43.f1 as c2, 42 as c3
from int4_tbl i42, int4_tbl i43) ss1
on i8.q1 = ss1.c2
on i41.f1 = ss1.c1,
lateral (select i41.*, i8.*, ss1.* from text_tbl limit 1) ss2
where ss1.c2 = 0;
ERROR: failed to assign all NestLoopParams to plan nodes
This problem vanishes with the proposed patch.
Thanks
Richard
I wrote:
> Doing things the way Richard wanted to (ie using the nestloop's
> required_outer) gets past the "failed to assign all NestLoopParams"
> error, but then we get the same "unrecognized node type: 22" error
> as in the first example.
> That error seems considerably nastier to fix. I think it is a
> pre-existing problem, though I don't currently have an explanation
> why we've not seen it reported before. What is happening is that
> we pull up a PHV for "c", containing a SubLink for (SELECT true),
> and only some copies of it get put through preprocess_expression,
> which is responsible for converting SubLinks to SubPlans among
> other essential tasks. It seems that up to now we've always
> managed to use only preprocessed copies in the finished plan.
> But now a not-preprocessed copy is managing to get through to
> the executor, which promptly spits up because it doesn't know
> what to do with a SubLink.
> We could probably hack this in a localized way by ensuring that
> we push a preprocessed copy of the PHV into the left plan's tlist.
> (One way to get that would be to look in the placeholder_list for the
> correct phid.) However, now that I've seen this I have very little
> faith that there aren't other bugs of the same ilk, that somehow
> we've not seen up to now. I'm thinking about what we must do to
> ensure that all PHVs are preprocessed once and only once, perhaps
> at the moment they get put into the placeholder_list.
I've traced through this in more detail, and found that it's
inaccurate to claim that the PHV's expression escaped preprocessing
altogether. (If it had, there would be more failure modes besides
"unprocessed SubLink", and we'd likely have noticed sooner.)
Rather, the problem is that SS_process_sublinks() skips replacement of
SubLinks within PlaceHolderVars that have phlevelsup > 0, expecting
that to be handled later. So the sequence of events that Alexander's
test case causes is:
1. We pull up the first sub-select "(SELECT (SELECT true) AS c FROM t,
LATERAL (SELECT b LIMIT 1))", causing the "WHERE c" in the later
LATERAL sub-select to be replaced by PHV((SELECT true)); a PHV is
needed since the pulled-up sub-select was under an outer join.
The PHV has phlevelsup = 1 since it's pushed down into a sub-select.
2. Expression pre-processing happens in the separately-planned
LATERAL sub-select, and while it does most of what's needed, it
explicitly skips replacement of the PHV's SubLink.
3. SS_replace_correlation_vars pulls the PHV out of the LATERAL
sub-select, replacing it with a Param and adding it to the
plan_params of the outer query level. Now it has phlevelsup = 0,
but there's still a SubLink inside it.
4. The code added by a16ef313f2 copies the PHV verbatim from
the plan_params list (via a subquery rel's subplan_params and
root->curOuterParams) into the outer subplan's tlist. Kaboom!
Now, if the PHV matched something that was already in the
outer subplan's tlist (recall it only needs to match by phid)
then we're safe, because that copy would have gotten fixed up
during extract_lateral_references. But in this example that
doesn't happen, probably because the PHV contains no Vars
so it doesn't get put into any baserel's reltarget.
I'm still trying to wrap my head around whether there are any
related failure modes in released branches. Steps 1-3 certainly
have been happening just like that for a long time, so that there
can be a PHV with an unexpanded SubLink floating around the
outer query level's data structures. Is the a16ef313f2 code
really the only way that that version of the PHV can make its way
into the emitted plan tree? Maybe, but I'm far from convinced.
Anyway, with one eye on the possibility that we'll need to back-patch
this in some form, I went for the localized fix of ensuring that
we use a copy of the PHV that's been through the normal processing.
I do want to look into restructuring the handling of these PHVs
to make this less messy, but that's not a job to undertake for v18
(much less if we have to back-patch).
One thing worth noting is that there's a visible shortcoming in the
generated plans for the new test cases: there are two copies of the
InitPlan that arises from the PHV's SubLink. One of them is
unreferenced and hence doesn't get used at runtime. That happens
because we are performing SS_process_sublinks on the PHV twice.
It's not fatal, but it's not great either. Perhaps a better design
could avoid that. I definitely don't want to adopt a fix that
results in still another InitPlan, so adding YA invocation of
preprocessing doesn't sound good.
regards, tom lane
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 8baf36ba4b7..8011c3c4bc8 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4344,6 +4344,7 @@ create_nestloop_plan(PlannerInfo *root,
NestLoop *join_plan;
Plan *outer_plan;
Plan *inner_plan;
+ Relids outerrelids;
List *tlist = build_path_tlist(root, &best_path->jpath.path);
List *joinrestrictclauses = best_path->jpath.joinrestrictinfo;
List *joinclauses;
@@ -4374,8 +4375,8 @@ create_nestloop_plan(PlannerInfo *root,
outer_plan = create_plan_recurse(root, best_path->jpath.outerjoinpath, 0);
/* For a nestloop, include outer relids in curOuterRels for inner side */
- root->curOuterRels = bms_union(root->curOuterRels,
- best_path->jpath.outerjoinpath->parent->relids);
+ outerrelids = best_path->jpath.outerjoinpath->parent->relids;
+ root->curOuterRels = bms_union(root->curOuterRels, outerrelids);
inner_plan = create_plan_recurse(root, best_path->jpath.innerjoinpath, 0);
@@ -4415,7 +4416,8 @@ create_nestloop_plan(PlannerInfo *root,
* node, and remove them from root->curOuterParams.
*/
nestParams = identify_current_nestloop_params(root,
- best_path->jpath.outerjoinpath);
+ outerrelids,
+ PATH_REQ_OUTER((Path *) best_path));
/*
* While nestloop parameters that are Vars had better be available from
@@ -4423,32 +4425,77 @@ create_nestloop_plan(PlannerInfo *root,
* that are PHVs won't be. In such cases we must add them to the
* outer_plan's tlist, since the executor's NestLoopParam machinery
* requires the params to be simple outer-Var references to that tlist.
+ * (This is cheating a little bit, because the outer path's required-outer
+ * relids might not be enough to allow evaluating such a PHV. But in
+ * practice, if we could have evaluated the PHV at the nestloop node, we
+ * can do so in the outer plan too.)
*/
outer_tlist = outer_plan->targetlist;
outer_parallel_safe = outer_plan->parallel_safe;
foreach(lc, nestParams)
{
NestLoopParam *nlp = (NestLoopParam *) lfirst(lc);
+ PlaceHolderVar *phv;
TargetEntry *tle;
if (IsA(nlp->paramval, Var))
continue; /* nothing to do for simple Vars */
- if (tlist_member((Expr *) nlp->paramval, outer_tlist))
+ /* Otherwise it must be a PHV */
+ phv = castNode(PlaceHolderVar, nlp->paramval);
+
+ if (tlist_member((Expr *) phv, outer_tlist))
continue; /* already available */
+ /*
+ * Deal with an even edgier edge case: if the PHV was pulled up out of
+ * a subquery and it contains a subquery that was originally pushed
+ * down from this query level, then that will still be represented as
+ * a SubLink, because SS_process_sublinks won't recurse into outer
+ * PHVs, so it didn't get transformed during expression preprocessing
+ * in the subquery. We need a version of the PHV that has a SubPlan,
+ * which we can get from the current query level's placeholder_list.
+ * This is quite grotty of course, but dealing with it earlier in the
+ * handling of subplan params would be just as grotty, and it might
+ * end up being a waste of cycles if we don't decide to treat the PHV
+ * as a NestLoopParam. (Perhaps that whole mechanism should be
+ * redesigned someday, but today is not that day.)
+ *
+ * It's not important to change the NestLoopParam tree, because the
+ * two PHVs will still be considered equal() and thus setrefs.c will
+ * still be able to replace the NestLoopParam's PHV with an outer-plan
+ * reference Var.
+ *
+ * In either case, make sure we have a copy of the PHV to put into
+ * outer_tlist, just for safety.
+ */
+ if (root->parse->hasSubLinks)
+ {
+ PlaceHolderInfo *phinfo = find_placeholder_info(root, phv);
+ PlaceHolderVar *newphv = copyObject(phinfo->ph_var);
+
+ /*
+ * The ph_var will have empty nullingrels, but we need the right
+ * nullingrels in the PHV we push into outer_tlist. Other fields
+ * should match already.
+ */
+ newphv->phnullingrels = phv->phnullingrels;
+ phv = newphv;
+ }
+ else
+ phv = copyObject(phv);
+
/* Make a shallow copy of outer_tlist, if we didn't already */
if (outer_tlist == outer_plan->targetlist)
outer_tlist = list_copy(outer_tlist);
/* ... and add the needed expression */
- tle = makeTargetEntry((Expr *) copyObject(nlp->paramval),
+ tle = makeTargetEntry((Expr *) phv,
list_length(outer_tlist) + 1,
NULL,
true);
outer_tlist = lappend(outer_tlist, tle);
/* ... and track whether tlist is (still) parallel-safe */
if (outer_parallel_safe)
- outer_parallel_safe = is_parallel_safe(root,
- (Node *) nlp->paramval);
+ outer_parallel_safe = is_parallel_safe(root, (Node *) phv);
}
if (outer_tlist != outer_plan->targetlist)
outer_plan = change_plan_targetlist(outer_plan, outer_tlist,
diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c
index 9836abf9479..5403ae89eab 100644
--- a/src/backend/optimizer/util/paramassign.c
+++ b/src/backend/optimizer/util/paramassign.c
@@ -599,9 +599,10 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
}
/*
- * Identify any NestLoopParams that should be supplied by a NestLoop plan
- * node with the specified lefthand input path. Remove them from the active
- * root->curOuterParams list and return them as the result list.
+ * Identify any NestLoopParams that should be supplied by a NestLoop
+ * plan node with the specified lefthand rels and required-outer rels.
+ * Remove them from the active root->curOuterParams list and return
+ * them as the result list.
*
* XXX Here we also hack up the returned Vars and PHVs so that they do not
* contain nullingrel sets exceeding what is available from the outer side.
@@ -626,11 +627,11 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
* subquery, which'd be unduly expensive.
*/
List *
-identify_current_nestloop_params(PlannerInfo *root, Path *leftpath)
+identify_current_nestloop_params(PlannerInfo *root,
+ Relids leftrelids,
+ Relids outerrelids)
{
List *result;
- Relids leftrelids = leftpath->parent->relids;
- Relids outerrelids = PATH_REQ_OUTER(leftpath);
Relids allleftrelids;
ListCell *cell;
diff --git a/src/include/optimizer/paramassign.h b/src/include/optimizer/paramassign.h
index d30d20de299..bbf7214289b 100644
--- a/src/include/optimizer/paramassign.h
+++ b/src/include/optimizer/paramassign.h
@@ -30,7 +30,8 @@ extern Param *replace_nestloop_param_placeholdervar(PlannerInfo *root,
extern void process_subquery_nestloop_params(PlannerInfo *root,
List *subplan_params);
extern List *identify_current_nestloop_params(PlannerInfo *root,
- Path *leftpath);
+ Relids leftrelids,
+ Relids outerrelids);
extern Param *generate_new_exec_param(PlannerInfo *root, Oid paramtype,
int32 paramtypmod, Oid paramcollation);
extern int assign_special_exec_param(PlannerInfo *root);
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index c292f04fdba..6266820b69a 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -4119,6 +4119,91 @@ select * from int8_tbl t1
(16 rows)
rollback;
+-- PHVs containing SubLinks are quite tricky to get right
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+ QUERY PLAN
+----------------------------------------------------------------
+ Nested Loop
+ Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2)
+ InitPlan 1
+ -> Result
+ Output: true
+ InitPlan 2
+ -> Result
+ Output: true
+ -> Seq Scan on public.int4_tbl i4
+ Output: i4.f1
+ Filter: (i4.f1 = 0)
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Subquery Scan on ss1
+ Output: ss1.y, (InitPlan 1).col1
+ -> Limit
+ Output: NULL::integer
+ -> Result
+ Output: NULL::integer
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Seq Scan on public.int8_tbl i8
+ Output: i8.q1, i8.q2
+ Filter: (i8.q2 = 123)
+ -> Limit
+ Output: (i8.q2)
+ -> Result
+ Output: i8.q2
+ One-Time Filter: ((InitPlan 1).col1)
+(29 rows)
+
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select 1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+ QUERY PLAN
+----------------------------------------------------------------
+ Nested Loop
+ Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2)
+ InitPlan 1
+ -> Result
+ Output: true
+ InitPlan 2
+ -> Result
+ Output: true
+ -> Limit
+ Output: NULL::integer
+ -> Result
+ Output: NULL::integer
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Seq Scan on public.int4_tbl i4
+ Output: i4.f1, (InitPlan 1).col1
+ Filter: (i4.f1 = 0)
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Seq Scan on public.int8_tbl i8
+ Output: i8.q1, i8.q2
+ Filter: (i8.q2 = 123)
+ -> Limit
+ Output: (i8.q2)
+ -> Result
+ Output: i8.q2
+ One-Time Filter: ((InitPlan 1).col1)
+(27 rows)
+
-- Test proper handling of appendrel PHVs during useless-RTE removal
explain (costs off)
select * from
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 88d2204e447..6b7a26cf295 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1361,6 +1361,29 @@ select * from int8_tbl t1
on true;
rollback;
+-- PHVs containing SubLinks are quite tricky to get right
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select 1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+
-- Test proper handling of appendrel PHVs during useless-RTE removal
explain (costs off)
select * from
Re: BUG #18953: Planner fails to build plan for complex query with LATERAL references
От
Alexander Lakhin
Дата:
Hello Tom and Richard,
23.06.2025 21:24, Tom Lane wrote:
23.06.2025 21:24, Tom Lane wrote:
Anyway, with one eye on the possibility that we'll need to back-patch this in some form, I went for the localized fix of ensuring that we use a copy of the PHV that's been through the normal processing. I do want to look into restructuring the handling of these PHVs to make this less messy, but that's not a job to undertake for v18 (much less if we have to back-patch).
I've managed to discover one more anomaly introduced by a16ef313f, that is
not fixed by fix-bug-18953-some-more:
CREATE TABLE t(i int PRIMARY KEY);
MERGE INTO t
USING
(SELECT 1 AS j FROM generate_series(1, 1))
RIGHT JOIN (SELECT 1) ON true
LEFT JOIN (SELECT 1 FROM (SELECT 1 FROM generate_series(1, 1))) ON false
ON i = j
WHEN NOT MATCHED THEN INSERT VALUES (1);
ERROR: XX000: wrong phnullingrels (b) (expected (b 4)) for PlaceHolderVar 1
LOCATION: search_indexed_tlist_for_phv, setrefs.c:2958
Thank you for spending your time on this!
Best regards,
Alexander
Alexander Lakhin <exclusion@gmail.com> writes:
> I've managed to discover one more anomaly introduced by a16ef313f, that is
> not fixed by fix-bug-18953-some-more:
Just to note that I am studying this. It looks to me like the
issue is that identify_current_nestloop_params() is handing back
a PHV with too few nullingrel bits set for the place that we want
to put it, as is acknowledged to be possible in its comments.
We thought we could get away with that, but in this context setrefs.c
will complain. I'm inclined to try to make it set the bits more
accurately, rather than further weaken setrefs.c's cross-checks.
I'm wondering again whether this isn't a case that is reachable
before a16ef313f. Perhaps the have_dangerous_phv check prevented
forming plan trees that could have this issue, but it's not real
clear why.
regards, tom lane
On Tue, Jun 24, 2025 at 3:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Now, if the PHV matched something that was already in the > outer subplan's tlist (recall it only needs to match by phid) > then we're safe, because that copy would have gotten fixed up > during extract_lateral_references. But in this example that > doesn't happen, probably because the PHV contains no Vars > so it doesn't get put into any baserel's reltarget. I think the reason the PHV isn't already included in the outer subplan's tlist is that it's supposed to be evaluated at (7 8), while the outer rel includes only (7). If the planner chooses a join order where (7 8) are joined first, the PHV would appear in that join rel's tlist -- but that's not the case here. > I'm still trying to wrap my head around whether there are any > related failure modes in released branches. Steps 1-3 certainly > have been happening just like that for a long time, so that there > can be a PHV with an unexpanded SubLink floating around the > outer query level's data structures. Is the a16ef313f2 code > really the only way that that version of the PHV can make its way > into the emitted plan tree? Maybe, but I'm far from convinced. Prior to a16ef313f, have_dangerous_phv() ensures that we don't choose a join order where the outer rel includes only (7). Instead, the planner will always join (7 8) first, allowing the well-preprocessed PHVs to be included in the tlist of that join rel. The unpreprocessed PHVs in NestLoop.nestParams don't cause problems because they can always be replaced with OUTER_VAR vars by setrefs.c. So it seems to me that we're good in released branches, but I could be wrong. Thanks Richard
On Fri, Jun 27, 2025 at 3:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Lakhin <exclusion@gmail.com> writes:
> > I've managed to discover one more anomaly introduced by a16ef313f, that is
> > not fixed by fix-bug-18953-some-more:
> Just to note that I am studying this. It looks to me like the
> issue is that identify_current_nestloop_params() is handing back
> a PHV with too few nullingrel bits set for the place that we want
> to put it, as is acknowledged to be possible in its comments.
> We thought we could get away with that, but in this context setrefs.c
> will complain. I'm inclined to try to make it set the bits more
> accurately, rather than further weaken setrefs.c's cross-checks.
>
> I'm wondering again whether this isn't a case that is reachable
> before a16ef313f. Perhaps the have_dangerous_phv check prevented
> forming plan trees that could have this issue, but it's not real
> clear why.
I can reproduce this error with the following query.
select * from t t1
left join (select 1 as x, * from t t2) t2 on t1.i = t2.i
left join t t3 on false
left join t t4 on t4.i > t2.x;
ERROR: wrong phnullingrels (b) (expected (b 3)) for PlaceHolderVar 1
There are two versions of the join clause "t4.i > t2.x" due to
outer-join identity 3: one involving PHV(t2.x) with an empty
phnullingrels, and another where PHV(t2.x) is nulled by the t1/t2
join.
When creating index paths for t4, we need to identify its join clauses
that match the index. The join_clause_is_movable_to() check there
rejects is_clone versions, assuming that "generating one path from the
minimally-parameterized has_clone version is sufficient". As a
result, only the version with empty phnullingrels appears in the t4's
scan_clauses (via its ParamPathInfo.ppi_clauses). That version of PHV
eventually gets pulled into nestParams by replace_nestloop_params().
Prior to a16ef313f, this does not cause problems because we are aware
that Vars/PHVs seen in NestLoopParams may have nullingrels that are
just a subset of those in the Vars/PHVs actually available from the
outer side. Accordingly, when fixing up NestLoopParams, we only
require a NRM_SUBSET match.
Starting from a16ef313f, create_nestloop_plan() might insert PHVs from
NestLoopParams into the outer subplan's tlist if they are not already
present there. To check for presence, it uses equal(), which compares
phnullingrels as well. So in this case, the PHV with empty
phnullingrels is added to the t1/t2/t3 join's tlist, even though a
semantically equivalent one already exists there with a different
phnullingrels. This causes setrefs.c to complain when fixing up the
tlist for the t1/t2/t3 join, because it requires a NRM_SUPERSET match.
I think we should avoid adding the PHV to the outer subplan's tlist if
there is already an equivalent one there. Ideally, we should fix this
by making the PHVs in the NestLoopParam expressions have accurate
nullingrels, but that doesn't seem like a trivial task. As a band-aid
fix, I think we could match on phid only in create_nestloop_plan()
when checking whether the PHV is already available, like below.
- if (tlist_member((Expr *) phv, outer_tlist))
+ foreach(lc2, outer_tlist)
+ {
+ TargetEntry *tlentry = (TargetEntry *) lfirst(lc2);
+ PlaceHolderVar *subphv = (PlaceHolderVar *) tlentry->expr;
+
+ if (IsA(subphv, PlaceHolderVar) &&
+ phv->phid == subphv->phid)
+ break;
+ }
+ if (lc2 != NULL)
continue; /* already available */
Thanks
Richard
Richard Guo <guofenglinux@gmail.com> writes:
> I think we should avoid adding the PHV to the outer subplan's tlist if
> there is already an equivalent one there. Ideally, we should fix this
> by making the PHVs in the NestLoopParam expressions have accurate
> nullingrels, but that doesn't seem like a trivial task. As a band-aid
> fix, I think we could match on phid only in create_nestloop_plan()
> when checking whether the PHV is already available, like below.
I don't love this solution, because it's not clear that it'd avoid
a failure if there *isn't* an equivalent PHV already there.
Also it seems like it's piling another level of band-aid on what's
already a mess.
I experimented with making identify_current_nestloop_params compute
the correct nullingrels "from scratch", and that seems to resolve
the problem. Now this is arguably not an ideal answer, because the
point of the setrefs.c cross-checks is to verify that we placed
Vars and PHVs at safe places to calculate correctly-nulled values,
and we're basically defeating those checks. But this feels like
the least crufty answer, and it's also confining the cruft to the
right place: if we can think of a more principled answer for
computing the nestloop params' nullingrels, we just have to fix
identify_current_nestloop_params to do that instead.
BTW, having done this it seems like we could get rid of the use
of NRM_SUBSET and instead use NRM_EQUAL for processing nestloop
params in setrefs.c. I tried that, and it gets through check-world,
but I'm too chicken to risk such a change in v18 at this stage.
I propose applying the attached for now and then removing NRM_SUBSET
when v19 opens for business.
For ease of review, 0001 attached is the same patch as before and
then 0002 is the new stuff. I'd squash them to one patch for
commit, though.
regards, tom lane
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 8baf36ba4b7..8011c3c4bc8 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4344,6 +4344,7 @@ create_nestloop_plan(PlannerInfo *root,
NestLoop *join_plan;
Plan *outer_plan;
Plan *inner_plan;
+ Relids outerrelids;
List *tlist = build_path_tlist(root, &best_path->jpath.path);
List *joinrestrictclauses = best_path->jpath.joinrestrictinfo;
List *joinclauses;
@@ -4374,8 +4375,8 @@ create_nestloop_plan(PlannerInfo *root,
outer_plan = create_plan_recurse(root, best_path->jpath.outerjoinpath, 0);
/* For a nestloop, include outer relids in curOuterRels for inner side */
- root->curOuterRels = bms_union(root->curOuterRels,
- best_path->jpath.outerjoinpath->parent->relids);
+ outerrelids = best_path->jpath.outerjoinpath->parent->relids;
+ root->curOuterRels = bms_union(root->curOuterRels, outerrelids);
inner_plan = create_plan_recurse(root, best_path->jpath.innerjoinpath, 0);
@@ -4415,7 +4416,8 @@ create_nestloop_plan(PlannerInfo *root,
* node, and remove them from root->curOuterParams.
*/
nestParams = identify_current_nestloop_params(root,
- best_path->jpath.outerjoinpath);
+ outerrelids,
+ PATH_REQ_OUTER((Path *) best_path));
/*
* While nestloop parameters that are Vars had better be available from
@@ -4423,32 +4425,77 @@ create_nestloop_plan(PlannerInfo *root,
* that are PHVs won't be. In such cases we must add them to the
* outer_plan's tlist, since the executor's NestLoopParam machinery
* requires the params to be simple outer-Var references to that tlist.
+ * (This is cheating a little bit, because the outer path's required-outer
+ * relids might not be enough to allow evaluating such a PHV. But in
+ * practice, if we could have evaluated the PHV at the nestloop node, we
+ * can do so in the outer plan too.)
*/
outer_tlist = outer_plan->targetlist;
outer_parallel_safe = outer_plan->parallel_safe;
foreach(lc, nestParams)
{
NestLoopParam *nlp = (NestLoopParam *) lfirst(lc);
+ PlaceHolderVar *phv;
TargetEntry *tle;
if (IsA(nlp->paramval, Var))
continue; /* nothing to do for simple Vars */
- if (tlist_member((Expr *) nlp->paramval, outer_tlist))
+ /* Otherwise it must be a PHV */
+ phv = castNode(PlaceHolderVar, nlp->paramval);
+
+ if (tlist_member((Expr *) phv, outer_tlist))
continue; /* already available */
+ /*
+ * Deal with an even edgier edge case: if the PHV was pulled up out of
+ * a subquery and it contains a subquery that was originally pushed
+ * down from this query level, then that will still be represented as
+ * a SubLink, because SS_process_sublinks won't recurse into outer
+ * PHVs, so it didn't get transformed during expression preprocessing
+ * in the subquery. We need a version of the PHV that has a SubPlan,
+ * which we can get from the current query level's placeholder_list.
+ * This is quite grotty of course, but dealing with it earlier in the
+ * handling of subplan params would be just as grotty, and it might
+ * end up being a waste of cycles if we don't decide to treat the PHV
+ * as a NestLoopParam. (Perhaps that whole mechanism should be
+ * redesigned someday, but today is not that day.)
+ *
+ * It's not important to change the NestLoopParam tree, because the
+ * two PHVs will still be considered equal() and thus setrefs.c will
+ * still be able to replace the NestLoopParam's PHV with an outer-plan
+ * reference Var.
+ *
+ * In either case, make sure we have a copy of the PHV to put into
+ * outer_tlist, just for safety.
+ */
+ if (root->parse->hasSubLinks)
+ {
+ PlaceHolderInfo *phinfo = find_placeholder_info(root, phv);
+ PlaceHolderVar *newphv = copyObject(phinfo->ph_var);
+
+ /*
+ * The ph_var will have empty nullingrels, but we need the right
+ * nullingrels in the PHV we push into outer_tlist. Other fields
+ * should match already.
+ */
+ newphv->phnullingrels = phv->phnullingrels;
+ phv = newphv;
+ }
+ else
+ phv = copyObject(phv);
+
/* Make a shallow copy of outer_tlist, if we didn't already */
if (outer_tlist == outer_plan->targetlist)
outer_tlist = list_copy(outer_tlist);
/* ... and add the needed expression */
- tle = makeTargetEntry((Expr *) copyObject(nlp->paramval),
+ tle = makeTargetEntry((Expr *) phv,
list_length(outer_tlist) + 1,
NULL,
true);
outer_tlist = lappend(outer_tlist, tle);
/* ... and track whether tlist is (still) parallel-safe */
if (outer_parallel_safe)
- outer_parallel_safe = is_parallel_safe(root,
- (Node *) nlp->paramval);
+ outer_parallel_safe = is_parallel_safe(root, (Node *) phv);
}
if (outer_tlist != outer_plan->targetlist)
outer_plan = change_plan_targetlist(outer_plan, outer_tlist,
diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c
index 9836abf9479..5403ae89eab 100644
--- a/src/backend/optimizer/util/paramassign.c
+++ b/src/backend/optimizer/util/paramassign.c
@@ -599,9 +599,10 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
}
/*
- * Identify any NestLoopParams that should be supplied by a NestLoop plan
- * node with the specified lefthand input path. Remove them from the active
- * root->curOuterParams list and return them as the result list.
+ * Identify any NestLoopParams that should be supplied by a NestLoop
+ * plan node with the specified lefthand rels and required-outer rels.
+ * Remove them from the active root->curOuterParams list and return
+ * them as the result list.
*
* XXX Here we also hack up the returned Vars and PHVs so that they do not
* contain nullingrel sets exceeding what is available from the outer side.
@@ -626,11 +627,11 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
* subquery, which'd be unduly expensive.
*/
List *
-identify_current_nestloop_params(PlannerInfo *root, Path *leftpath)
+identify_current_nestloop_params(PlannerInfo *root,
+ Relids leftrelids,
+ Relids outerrelids)
{
List *result;
- Relids leftrelids = leftpath->parent->relids;
- Relids outerrelids = PATH_REQ_OUTER(leftpath);
Relids allleftrelids;
ListCell *cell;
diff --git a/src/include/optimizer/paramassign.h b/src/include/optimizer/paramassign.h
index d30d20de299..bbf7214289b 100644
--- a/src/include/optimizer/paramassign.h
+++ b/src/include/optimizer/paramassign.h
@@ -30,7 +30,8 @@ extern Param *replace_nestloop_param_placeholdervar(PlannerInfo *root,
extern void process_subquery_nestloop_params(PlannerInfo *root,
List *subplan_params);
extern List *identify_current_nestloop_params(PlannerInfo *root,
- Path *leftpath);
+ Relids leftrelids,
+ Relids outerrelids);
extern Param *generate_new_exec_param(PlannerInfo *root, Oid paramtype,
int32 paramtypmod, Oid paramcollation);
extern int assign_special_exec_param(PlannerInfo *root);
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index c292f04fdba..6266820b69a 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -4119,6 +4119,91 @@ select * from int8_tbl t1
(16 rows)
rollback;
+-- PHVs containing SubLinks are quite tricky to get right
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+ QUERY PLAN
+----------------------------------------------------------------
+ Nested Loop
+ Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2)
+ InitPlan 1
+ -> Result
+ Output: true
+ InitPlan 2
+ -> Result
+ Output: true
+ -> Seq Scan on public.int4_tbl i4
+ Output: i4.f1
+ Filter: (i4.f1 = 0)
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Subquery Scan on ss1
+ Output: ss1.y, (InitPlan 1).col1
+ -> Limit
+ Output: NULL::integer
+ -> Result
+ Output: NULL::integer
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Seq Scan on public.int8_tbl i8
+ Output: i8.q1, i8.q2
+ Filter: (i8.q2 = 123)
+ -> Limit
+ Output: (i8.q2)
+ -> Result
+ Output: i8.q2
+ One-Time Filter: ((InitPlan 1).col1)
+(29 rows)
+
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select 1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+ QUERY PLAN
+----------------------------------------------------------------
+ Nested Loop
+ Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2)
+ InitPlan 1
+ -> Result
+ Output: true
+ InitPlan 2
+ -> Result
+ Output: true
+ -> Limit
+ Output: NULL::integer
+ -> Result
+ Output: NULL::integer
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Seq Scan on public.int4_tbl i4
+ Output: i4.f1, (InitPlan 1).col1
+ Filter: (i4.f1 = 0)
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Seq Scan on public.int8_tbl i8
+ Output: i8.q1, i8.q2
+ Filter: (i8.q2 = 123)
+ -> Limit
+ Output: (i8.q2)
+ -> Result
+ Output: i8.q2
+ One-Time Filter: ((InitPlan 1).col1)
+(27 rows)
+
-- Test proper handling of appendrel PHVs during useless-RTE removal
explain (costs off)
select * from
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 88d2204e447..6b7a26cf295 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1361,6 +1361,29 @@ select * from int8_tbl t1
on true;
rollback;
+-- PHVs containing SubLinks are quite tricky to get right
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select 1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+
-- Test proper handling of appendrel PHVs during useless-RTE removal
explain (costs off)
select * from
diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c
index 5403ae89eab..b1128d62371 100644
--- a/src/backend/optimizer/util/paramassign.c
+++ b/src/backend/optimizer/util/paramassign.c
@@ -604,25 +604,17 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
* Remove them from the active root->curOuterParams list and return
* them as the result list.
*
- * XXX Here we also hack up the returned Vars and PHVs so that they do not
- * contain nullingrel sets exceeding what is available from the outer side.
- * This is needed if we have applied outer join identity 3,
- * (A leftjoin B on (Pab)) leftjoin C on (Pb*c)
- * = A leftjoin (B leftjoin C on (Pbc)) on (Pab)
- * and C contains lateral references to B. It's still safe to apply the
- * identity, but the parser will have created those references in the form
- * "b*" (i.e., with varnullingrels listing the A/B join), while what we will
- * have available from the nestloop's outer side is just "b". We deal with
- * that here by stripping the nullingrels down to what is available from the
- * outer side according to leftrelids.
- *
- * That fixes matters for the case of forward application of identity 3.
- * If the identity was applied in the reverse direction, we will have
- * parameter Vars containing too few nullingrel bits rather than too many.
- * Currently, that causes no problems because setrefs.c applies only a
- * subset check to nullingrels in NestLoopParams, but we'd have to work
- * harder if we ever want to tighten that check. This is all pretty annoying
- * because it greatly weakens setrefs.c's cross-check, but the alternative
+ * Vars and PHVs appearing in the result list must have nullingrel sets
+ * that could validly appear in the lefthand rel's output. Ordinarily that
+ * would be true already, but if we have applied outer join identity 3,
+ * there could be more or fewer nullingrel bits in the nodes appearing in
+ * curOuterParams than are in the nominal leftrelids. We deal with that by
+ * forcing their nullingrel sets to include exactly the outer-join relids
+ * that appear in leftrelids and can null the respective Var or PHV.
+ * This fix is a bit ad-hoc and intellectually unsatisfactory, because it's
+ * essentially jumping to the conclusion that we've placed evaluation of
+ * the nestloop parameters correctly, and thus it defeats the intent of the
+ * subsequent nullingrel cross-checks in setrefs.c. But the alternative
* seems to be to generate multiple versions of each laterally-parameterized
* subquery, which'd be unduly expensive.
*/
@@ -662,25 +654,28 @@ identify_current_nestloop_params(PlannerInfo *root,
bms_is_member(nlp->paramval->varno, leftrelids))
{
Var *var = (Var *) nlp->paramval;
+ RelOptInfo *rel = root->simple_rel_array[var->varno];
root->curOuterParams = foreach_delete_current(root->curOuterParams,
cell);
- var->varnullingrels = bms_intersect(var->varnullingrels,
+ var->varnullingrels = bms_intersect(rel->nulling_relids,
leftrelids);
result = lappend(result, nlp);
}
else if (IsA(nlp->paramval, PlaceHolderVar))
{
PlaceHolderVar *phv = (PlaceHolderVar *) nlp->paramval;
- Relids eval_at = find_placeholder_info(root, phv)->ph_eval_at;
+ PlaceHolderInfo *phinfo = find_placeholder_info(root, phv);
+ Relids eval_at = phinfo->ph_eval_at;
if (bms_is_subset(eval_at, allleftrelids) &&
bms_overlap(eval_at, leftrelids))
{
root->curOuterParams = foreach_delete_current(root->curOuterParams,
cell);
- phv->phnullingrels = bms_intersect(phv->phnullingrels,
- leftrelids);
+ phv->phnullingrels =
+ bms_intersect(get_placeholder_nulling_relids(root, phinfo),
+ leftrelids);
result = lappend(result, nlp);
}
}
diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c
index 41a4c81e94a..e1cd00a72fb 100644
--- a/src/backend/optimizer/util/placeholder.c
+++ b/src/backend/optimizer/util/placeholder.c
@@ -545,3 +545,43 @@ contain_placeholder_references_walker(Node *node,
return expression_tree_walker(node, contain_placeholder_references_walker,
context);
}
+
+/*
+ * Compute the set of outer-join relids that can null a placeholder.
+ *
+ * This is analogous to RelOptInfo.nulling_relids for Vars, but we compute it
+ * on-the-fly rather than saving it somewhere. Currently the value is needed
+ * at most once per query, so there's little value in doing otherwise. If it
+ * ever gains more widespread use, perhaps we should cache the result in
+ * PlaceHolderInfo.
+ */
+Relids
+get_placeholder_nulling_relids(PlannerInfo *root, PlaceHolderInfo *phinfo)
+{
+ Relids result = NULL;
+ int relid = -1;
+
+ /*
+ * Form the union of all potential nulling OJs for each baserel included
+ * in ph_eval_at.
+ */
+ while ((relid = bms_next_member(phinfo->ph_eval_at, relid)) > 0)
+ {
+ RelOptInfo *rel = root->simple_rel_array[relid];
+
+ /* ignore the RTE_GROUP RTE */
+ if (relid == root->group_rtindex)
+ continue;
+
+ if (rel == NULL) /* must be an outer join */
+ {
+ Assert(bms_is_member(relid, root->outer_join_rels));
+ continue;
+ }
+ result = bms_add_members(result, rel->nulling_relids);
+ }
+
+ /* Now remove any OJs already included in ph_eval_at, and we're done. */
+ result = bms_del_members(result, phinfo->ph_eval_at);
+ return result;
+}
diff --git a/src/include/optimizer/placeholder.h b/src/include/optimizer/placeholder.h
index d351045e2e0..db92d8861ba 100644
--- a/src/include/optimizer/placeholder.h
+++ b/src/include/optimizer/placeholder.h
@@ -30,5 +30,7 @@ extern void add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
SpecialJoinInfo *sjinfo);
extern bool contain_placeholder_references_to(PlannerInfo *root, Node *clause,
int relid);
+extern Relids get_placeholder_nulling_relids(PlannerInfo *root,
+ PlaceHolderInfo *phinfo);
#endif /* PLACEHOLDER_H */
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 6266820b69a..67793292054 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -4118,6 +4118,44 @@ select * from int8_tbl t1
Output: NULL::bigint, NULL::bigint
(16 rows)
+rollback;
+-- A related failure
+begin;
+create temp table t(i int primary key);
+explain (verbose, costs off)
+select * from t t1
+ left join (select 1 as x, * from t t2(i2)) t2ss on t1.i = t2ss.i2
+ left join t t3(i3) on false
+ left join t t4(i4) on t4.i4 > t2ss.x;
+ QUERY PLAN
+----------------------------------------------------------
+ Nested Loop Left Join
+ Output: t1.i, (1), t2.i2, i3, t4.i4
+ -> Nested Loop Left Join
+ Output: t1.i, t2.i2, (1), i3
+ Join Filter: false
+ -> Hash Left Join
+ Output: t1.i, t2.i2, (1)
+ Inner Unique: true
+ Hash Cond: (t1.i = t2.i2)
+ -> Seq Scan on pg_temp.t t1
+ Output: t1.i
+ -> Hash
+ Output: t2.i2, (1)
+ -> Seq Scan on pg_temp.t t2
+ Output: t2.i2, 1
+ -> Result
+ Output: i3
+ One-Time Filter: false
+ -> Memoize
+ Output: t4.i4
+ Cache Key: (1)
+ Cache Mode: binary
+ -> Index Only Scan using t_pkey on pg_temp.t t4
+ Output: t4.i4
+ Index Cond: (t4.i4 > (1))
+(25 rows)
+
rollback;
-- PHVs containing SubLinks are quite tricky to get right
explain (verbose, costs off)
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 6b7a26cf295..21747841808 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1361,6 +1361,16 @@ select * from int8_tbl t1
on true;
rollback;
+-- A related failure
+begin;
+create temp table t(i int primary key);
+explain (verbose, costs off)
+select * from t t1
+ left join (select 1 as x, * from t t2(i2)) t2ss on t1.i = t2ss.i2
+ left join t t3(i3) on false
+ left join t t4(i4) on t4.i4 > t2ss.x;
+rollback;
+
-- PHVs containing SubLinks are quite tricky to get right
explain (verbose, costs off)
select *
... BTW, looking at these two patches again, I wonder if it'd be
better to put the hackery for SubLink conversion into
identify_current_nestloop_params? That'd remove the need to argue
that we don't have to fix both copies of a nestloop-parameter PHV.
The responsibility doesn't clearly belong to
identify_current_nestloop_params, but it doesn't clearly belong
to create_nestloop_plan either, so neither choice seems superior
from a modularity standpoint.
regards, tom lane
Re: BUG #18953: Planner fails to build plan for complex query with LATERAL references
От
Alexander Lakhin
Дата:
Hello Tom and Richard,
28.06.2025 00:37, Tom Lane wrote:
28.06.2025 00:37, Tom Lane wrote:
... BTW, looking at these two patches again, I wonder if it'd be better to put the hackery for SubLink conversion into identify_current_nestloop_params? That'd remove the need to argue that we don't have to fix both copies of a nestloop-parameter PHV. The responsibility doesn't clearly belong to identify_current_nestloop_params, but it doesn't clearly belong to create_nestloop_plan either, so neither choice seems superior from a modularity standpoint.
I've discovered a query which fails with an error after a16ef313f with
both v2 patches applied:
create temp table t(i int primary key);
select * from
(select k from
(select i, coalesce(i, j) as k from
(select i from t union all select 1)
join (select 1 as j limit 1) on i = j)
right join (select 1) on true
join (select 1) on i is not null
),
lateral (select k offset 1);
ERROR: XX000: variable not found in subplan target list
LOCATION: fix_upper_expr_mutator, setrefs.c:3310
Thank you for working on this!
Best regards,
Alexander
Alexander Lakhin <exclusion@gmail.com> writes:
> 28.06.2025 00:37, Tom Lane wrote:
>> ... BTW, looking at these two patches again, I wonder if it'd be
>> better to put the hackery for SubLink conversion into
>> identify_current_nestloop_params? That'd remove the need to argue
>> that we don't have to fix both copies of a nestloop-parameter PHV.
> I've discovered a query which fails with an error after a16ef313f with
> both v2 patches applied:
Thank you! That demonstrates an unrelated oversight in a16ef313f:
we need to apply replace_nestloop_params() to the expression of a PHV
that is chosen to be a nestloop param. That's because it could
contain references to Vars/PHVs that will be supplied by some upper
nestloop.
In v3 attached, I moved the SubLink hacking into
identify_current_nestloop_params as suggested above. But
replace_nestloop_params is local to createplan.c, and anyway we can't
run it until all of the current nestloop's NLPs have been removed from
root->curOuterParams, else it might make invalid replacements. So
that has to be done in create_nestloop_plan.
I squashed everything into one patch, too.
regards, tom lane
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 8baf36ba4b7..4d059eba3f0 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4344,6 +4344,7 @@ create_nestloop_plan(PlannerInfo *root,
NestLoop *join_plan;
Plan *outer_plan;
Plan *inner_plan;
+ Relids outerrelids;
List *tlist = build_path_tlist(root, &best_path->jpath.path);
List *joinrestrictclauses = best_path->jpath.joinrestrictinfo;
List *joinclauses;
@@ -4374,8 +4375,8 @@ create_nestloop_plan(PlannerInfo *root,
outer_plan = create_plan_recurse(root, best_path->jpath.outerjoinpath, 0);
/* For a nestloop, include outer relids in curOuterRels for inner side */
- root->curOuterRels = bms_union(root->curOuterRels,
- best_path->jpath.outerjoinpath->parent->relids);
+ outerrelids = best_path->jpath.outerjoinpath->parent->relids;
+ root->curOuterRels = bms_union(root->curOuterRels, outerrelids);
inner_plan = create_plan_recurse(root, best_path->jpath.innerjoinpath, 0);
@@ -4415,7 +4416,8 @@ create_nestloop_plan(PlannerInfo *root,
* node, and remove them from root->curOuterParams.
*/
nestParams = identify_current_nestloop_params(root,
- best_path->jpath.outerjoinpath);
+ outerrelids,
+ PATH_REQ_OUTER((Path *) best_path));
/*
* While nestloop parameters that are Vars had better be available from
@@ -4423,32 +4425,57 @@ create_nestloop_plan(PlannerInfo *root,
* that are PHVs won't be. In such cases we must add them to the
* outer_plan's tlist, since the executor's NestLoopParam machinery
* requires the params to be simple outer-Var references to that tlist.
+ * (This is cheating a little bit, because the outer path's required-outer
+ * relids might not be enough to allow evaluating such a PHV. But in
+ * practice, if we could have evaluated the PHV at the nestloop node, we
+ * can do so in the outer plan too.)
*/
outer_tlist = outer_plan->targetlist;
outer_parallel_safe = outer_plan->parallel_safe;
foreach(lc, nestParams)
{
NestLoopParam *nlp = (NestLoopParam *) lfirst(lc);
+ PlaceHolderVar *phv;
TargetEntry *tle;
if (IsA(nlp->paramval, Var))
continue; /* nothing to do for simple Vars */
- if (tlist_member((Expr *) nlp->paramval, outer_tlist))
+ /* Otherwise it must be a PHV */
+ phv = castNode(PlaceHolderVar, nlp->paramval);
+
+ if (tlist_member((Expr *) phv, outer_tlist))
continue; /* already available */
+ /*
+ * It's possible that nestloop parameter PHVs selected to evaluate
+ * here contain references to surviving root->curOuterParams items
+ * (that is, they reference values that will be supplied by some
+ * higher-level nestloop). Those need to be converted to Params now.
+ */
+ phv = (PlaceHolderVar *) replace_nestloop_params(root, (Node *) phv);
+
+ /*
+ * It's not actually necessary to update the NestLoopParam entry,
+ * because the two PHVs would be equal() anyway and thus setrefs.c
+ * would still replace the NestLoopParam's PHV with an outer-plan
+ * reference Var. (This is also why we needn't run
+ * replace_nestloop_params before checking tlist_member.) But we
+ * might as well do it for cleanliness.
+ */
+ nlp->paramval = (Var *) phv;
+
/* Make a shallow copy of outer_tlist, if we didn't already */
if (outer_tlist == outer_plan->targetlist)
outer_tlist = list_copy(outer_tlist);
/* ... and add the needed expression */
- tle = makeTargetEntry((Expr *) copyObject(nlp->paramval),
+ tle = makeTargetEntry((Expr *) copyObject(phv),
list_length(outer_tlist) + 1,
NULL,
true);
outer_tlist = lappend(outer_tlist, tle);
/* ... and track whether tlist is (still) parallel-safe */
if (outer_parallel_safe)
- outer_parallel_safe = is_parallel_safe(root,
- (Node *) nlp->paramval);
+ outer_parallel_safe = is_parallel_safe(root, (Node *) phv);
}
if (outer_tlist != outer_plan->targetlist)
outer_plan = change_plan_targetlist(outer_plan, outer_tlist,
diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c
index 9836abf9479..4c13c5931b4 100644
--- a/src/backend/optimizer/util/paramassign.c
+++ b/src/backend/optimizer/util/paramassign.c
@@ -599,38 +599,31 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
}
/*
- * Identify any NestLoopParams that should be supplied by a NestLoop plan
- * node with the specified lefthand input path. Remove them from the active
- * root->curOuterParams list and return them as the result list.
+ * Identify any NestLoopParams that should be supplied by a NestLoop
+ * plan node with the specified lefthand rels and required-outer rels.
+ * Remove them from the active root->curOuterParams list and return
+ * them as the result list.
*
- * XXX Here we also hack up the returned Vars and PHVs so that they do not
- * contain nullingrel sets exceeding what is available from the outer side.
- * This is needed if we have applied outer join identity 3,
- * (A leftjoin B on (Pab)) leftjoin C on (Pb*c)
- * = A leftjoin (B leftjoin C on (Pbc)) on (Pab)
- * and C contains lateral references to B. It's still safe to apply the
- * identity, but the parser will have created those references in the form
- * "b*" (i.e., with varnullingrels listing the A/B join), while what we will
- * have available from the nestloop's outer side is just "b". We deal with
- * that here by stripping the nullingrels down to what is available from the
- * outer side according to leftrelids.
- *
- * That fixes matters for the case of forward application of identity 3.
- * If the identity was applied in the reverse direction, we will have
- * parameter Vars containing too few nullingrel bits rather than too many.
- * Currently, that causes no problems because setrefs.c applies only a
- * subset check to nullingrels in NestLoopParams, but we'd have to work
- * harder if we ever want to tighten that check. This is all pretty annoying
- * because it greatly weakens setrefs.c's cross-check, but the alternative
+ * Vars and PHVs appearing in the result list must have nullingrel sets
+ * that could validly appear in the lefthand rel's output. Ordinarily that
+ * would be true already, but if we have applied outer join identity 3,
+ * there could be more or fewer nullingrel bits in the nodes appearing in
+ * curOuterParams than are in the nominal leftrelids. We deal with that by
+ * forcing their nullingrel sets to include exactly the outer-join relids
+ * that appear in leftrelids and can null the respective Var or PHV.
+ * This fix is a bit ad-hoc and intellectually unsatisfactory, because it's
+ * essentially jumping to the conclusion that we've placed evaluation of
+ * the nestloop parameters correctly, and thus it defeats the intent of the
+ * subsequent nullingrel cross-checks in setrefs.c. But the alternative
* seems to be to generate multiple versions of each laterally-parameterized
* subquery, which'd be unduly expensive.
*/
List *
-identify_current_nestloop_params(PlannerInfo *root, Path *leftpath)
+identify_current_nestloop_params(PlannerInfo *root,
+ Relids leftrelids,
+ Relids outerrelids)
{
List *result;
- Relids leftrelids = leftpath->parent->relids;
- Relids outerrelids = PATH_REQ_OUTER(leftpath);
Relids allleftrelids;
ListCell *cell;
@@ -661,25 +654,58 @@ identify_current_nestloop_params(PlannerInfo *root, Path *leftpath)
bms_is_member(nlp->paramval->varno, leftrelids))
{
Var *var = (Var *) nlp->paramval;
+ RelOptInfo *rel = root->simple_rel_array[var->varno];
root->curOuterParams = foreach_delete_current(root->curOuterParams,
cell);
- var->varnullingrels = bms_intersect(var->varnullingrels,
+ var->varnullingrels = bms_intersect(rel->nulling_relids,
leftrelids);
result = lappend(result, nlp);
}
else if (IsA(nlp->paramval, PlaceHolderVar))
{
PlaceHolderVar *phv = (PlaceHolderVar *) nlp->paramval;
- Relids eval_at = find_placeholder_info(root, phv)->ph_eval_at;
+ PlaceHolderInfo *phinfo = find_placeholder_info(root, phv);
+ Relids eval_at = phinfo->ph_eval_at;
if (bms_is_subset(eval_at, allleftrelids) &&
bms_overlap(eval_at, leftrelids))
{
root->curOuterParams = foreach_delete_current(root->curOuterParams,
cell);
- phv->phnullingrels = bms_intersect(phv->phnullingrels,
- leftrelids);
+
+ /*
+ * Deal with an edge case: if the PHV was pulled up out of a
+ * subquery and it contains a subquery that was originally
+ * pushed down from this query level, then that will still be
+ * represented as a SubLink, because SS_process_sublinks won't
+ * recurse into outer PHVs, so it didn't get transformed
+ * during expression preprocessing in the subquery. We need a
+ * version of the PHV that has a SubPlan, which we can get
+ * from the current query level's placeholder_list. This is
+ * quite grotty of course, but dealing with it earlier in the
+ * handling of subplan params would be just as grotty, and it
+ * might end up being a waste of cycles if we don't decide to
+ * treat the PHV as a NestLoopParam. (Perhaps that whole
+ * mechanism should be redesigned someday, but today is not
+ * that day.)
+ */
+ if (root->parse->hasSubLinks)
+ {
+ phv = copyObject(phinfo->ph_var);
+
+ /*
+ * The ph_var will have empty nullingrels, but that
+ * doesn't matter since we're about to overwrite
+ * phv->phnullingrels. Other fields should be OK already.
+ */
+ nlp->paramval = (Var *) phv;
+ }
+
+ phv->phnullingrels =
+ bms_intersect(get_placeholder_nulling_relids(root, phinfo),
+ leftrelids);
+
result = lappend(result, nlp);
}
}
diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c
index 41a4c81e94a..e1cd00a72fb 100644
--- a/src/backend/optimizer/util/placeholder.c
+++ b/src/backend/optimizer/util/placeholder.c
@@ -545,3 +545,43 @@ contain_placeholder_references_walker(Node *node,
return expression_tree_walker(node, contain_placeholder_references_walker,
context);
}
+
+/*
+ * Compute the set of outer-join relids that can null a placeholder.
+ *
+ * This is analogous to RelOptInfo.nulling_relids for Vars, but we compute it
+ * on-the-fly rather than saving it somewhere. Currently the value is needed
+ * at most once per query, so there's little value in doing otherwise. If it
+ * ever gains more widespread use, perhaps we should cache the result in
+ * PlaceHolderInfo.
+ */
+Relids
+get_placeholder_nulling_relids(PlannerInfo *root, PlaceHolderInfo *phinfo)
+{
+ Relids result = NULL;
+ int relid = -1;
+
+ /*
+ * Form the union of all potential nulling OJs for each baserel included
+ * in ph_eval_at.
+ */
+ while ((relid = bms_next_member(phinfo->ph_eval_at, relid)) > 0)
+ {
+ RelOptInfo *rel = root->simple_rel_array[relid];
+
+ /* ignore the RTE_GROUP RTE */
+ if (relid == root->group_rtindex)
+ continue;
+
+ if (rel == NULL) /* must be an outer join */
+ {
+ Assert(bms_is_member(relid, root->outer_join_rels));
+ continue;
+ }
+ result = bms_add_members(result, rel->nulling_relids);
+ }
+
+ /* Now remove any OJs already included in ph_eval_at, and we're done. */
+ result = bms_del_members(result, phinfo->ph_eval_at);
+ return result;
+}
diff --git a/src/include/optimizer/paramassign.h b/src/include/optimizer/paramassign.h
index d30d20de299..bbf7214289b 100644
--- a/src/include/optimizer/paramassign.h
+++ b/src/include/optimizer/paramassign.h
@@ -30,7 +30,8 @@ extern Param *replace_nestloop_param_placeholdervar(PlannerInfo *root,
extern void process_subquery_nestloop_params(PlannerInfo *root,
List *subplan_params);
extern List *identify_current_nestloop_params(PlannerInfo *root,
- Path *leftpath);
+ Relids leftrelids,
+ Relids outerrelids);
extern Param *generate_new_exec_param(PlannerInfo *root, Oid paramtype,
int32 paramtypmod, Oid paramcollation);
extern int assign_special_exec_param(PlannerInfo *root);
diff --git a/src/include/optimizer/placeholder.h b/src/include/optimizer/placeholder.h
index d351045e2e0..db92d8861ba 100644
--- a/src/include/optimizer/placeholder.h
+++ b/src/include/optimizer/placeholder.h
@@ -30,5 +30,7 @@ extern void add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
SpecialJoinInfo *sjinfo);
extern bool contain_placeholder_references_to(PlannerInfo *root, Node *clause,
int relid);
+extern Relids get_placeholder_nulling_relids(PlannerInfo *root,
+ PlaceHolderInfo *phinfo);
#endif /* PLACEHOLDER_H */
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index c292f04fdba..390aabfb34b 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -4119,6 +4119,164 @@ select * from int8_tbl t1
(16 rows)
rollback;
+-- ... not that the initial replacement didn't have some bugs too
+begin;
+create temp table t(i int primary key);
+explain (verbose, costs off)
+select * from t t1
+ left join (select 1 as x, * from t t2(i2)) t2ss on t1.i = t2ss.i2
+ left join t t3(i3) on false
+ left join t t4(i4) on t4.i4 > t2ss.x;
+ QUERY PLAN
+----------------------------------------------------------
+ Nested Loop Left Join
+ Output: t1.i, (1), t2.i2, i3, t4.i4
+ -> Nested Loop Left Join
+ Output: t1.i, t2.i2, (1), i3
+ Join Filter: false
+ -> Hash Left Join
+ Output: t1.i, t2.i2, (1)
+ Inner Unique: true
+ Hash Cond: (t1.i = t2.i2)
+ -> Seq Scan on pg_temp.t t1
+ Output: t1.i
+ -> Hash
+ Output: t2.i2, (1)
+ -> Seq Scan on pg_temp.t t2
+ Output: t2.i2, 1
+ -> Result
+ Output: i3
+ One-Time Filter: false
+ -> Memoize
+ Output: t4.i4
+ Cache Key: (1)
+ Cache Mode: binary
+ -> Index Only Scan using t_pkey on pg_temp.t t4
+ Output: t4.i4
+ Index Cond: (t4.i4 > (1))
+(25 rows)
+
+explain (verbose, costs off)
+select * from
+ (select k from
+ (select i, coalesce(i, j) as k from
+ (select i from t union all select 0)
+ join (select 1 as j limit 1) on i = j)
+ right join (select 2 as x) on true
+ join (select 3 as y) on i is not null
+ ),
+ lateral (select k as kl limit 1);
+ QUERY PLAN
+-------------------------------------------------------------------
+ Nested Loop
+ Output: COALESCE(t.i, (1)), ((COALESCE(t.i, (1))))
+ -> Limit
+ Output: 1
+ -> Result
+ Output: 1
+ -> Nested Loop
+ Output: t.i, ((COALESCE(t.i, (1))))
+ -> Result
+ Output: t.i, COALESCE(t.i, (1))
+ -> Append
+ -> Index Only Scan using t_pkey on pg_temp.t
+ Output: t.i
+ Index Cond: (t.i = (1))
+ -> Result
+ Output: 0
+ One-Time Filter: ((1) = 0)
+ -> Limit
+ Output: ((COALESCE(t.i, (1))))
+ -> Result
+ Output: (COALESCE(t.i, (1)))
+(21 rows)
+
+rollback;
+-- PHVs containing SubLinks are quite tricky to get right
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+ QUERY PLAN
+----------------------------------------------------------------
+ Nested Loop
+ Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2)
+ InitPlan 1
+ -> Result
+ Output: true
+ InitPlan 2
+ -> Result
+ Output: true
+ -> Seq Scan on public.int4_tbl i4
+ Output: i4.f1
+ Filter: (i4.f1 = 0)
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Subquery Scan on ss1
+ Output: ss1.y, (InitPlan 1).col1
+ -> Limit
+ Output: NULL::integer
+ -> Result
+ Output: NULL::integer
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Seq Scan on public.int8_tbl i8
+ Output: i8.q1, i8.q2
+ Filter: (i8.q2 = 123)
+ -> Limit
+ Output: (i8.q2)
+ -> Result
+ Output: i8.q2
+ One-Time Filter: ((InitPlan 1).col1)
+(29 rows)
+
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select 1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+ QUERY PLAN
+----------------------------------------------------------------
+ Nested Loop
+ Output: i8.q1, i8.q2, (InitPlan 1).col1, false, (i8.q2)
+ InitPlan 1
+ -> Result
+ Output: true
+ InitPlan 2
+ -> Result
+ Output: true
+ -> Limit
+ Output: NULL::integer
+ -> Result
+ Output: NULL::integer
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Seq Scan on public.int4_tbl i4
+ Output: i4.f1, (InitPlan 1).col1
+ Filter: (i4.f1 = 0)
+ -> Nested Loop
+ Output: i8.q1, i8.q2, (i8.q2)
+ -> Seq Scan on public.int8_tbl i8
+ Output: i8.q1, i8.q2
+ Filter: (i8.q2 = 123)
+ -> Limit
+ Output: (i8.q2)
+ -> Result
+ Output: i8.q2
+ One-Time Filter: ((InitPlan 1).col1)
+(27 rows)
+
-- Test proper handling of appendrel PHVs during useless-RTE removal
explain (costs off)
select * from
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 88d2204e447..f6e7070db65 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1361,6 +1361,52 @@ select * from int8_tbl t1
on true;
rollback;
+-- ... not that the initial replacement didn't have some bugs too
+begin;
+create temp table t(i int primary key);
+
+explain (verbose, costs off)
+select * from t t1
+ left join (select 1 as x, * from t t2(i2)) t2ss on t1.i = t2ss.i2
+ left join t t3(i3) on false
+ left join t t4(i4) on t4.i4 > t2ss.x;
+
+explain (verbose, costs off)
+select * from
+ (select k from
+ (select i, coalesce(i, j) as k from
+ (select i from t union all select 0)
+ join (select 1 as j limit 1) on i = j)
+ right join (select 2 as x) on true
+ join (select 3 as y) on i is not null
+ ),
+ lateral (select k as kl limit 1);
+
+rollback;
+
+-- PHVs containing SubLinks are quite tricky to get right
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select i4.f1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+
+explain (verbose, costs off)
+select *
+from int8_tbl i8
+ inner join
+ (select (select true) as x
+ from int4_tbl i4, lateral (select 1 as y limit 1) ss1
+ where i4.f1 = 0) ss2 on true
+ right join (select false as z) ss3 on true,
+ lateral (select i8.q2 as q2l where x limit 1) ss4
+where i8.q2 = 123;
+
-- Test proper handling of appendrel PHVs during useless-RTE removal
explain (costs off)
select * from