From acc5821e4dcb6b7df6ad1f806459f95fcaebadfc Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 10 Feb 2023 13:31:00 -0500 Subject: [PATCH] Further fixes in qual nullingrel adjustment for outer join commutation. One of the add_nulling_relids calls in deconstruct_distribute_oj_quals added an OJ relid to too few Vars, while the other added it to too many. We should consider the syntactic structure not min_left/righthand while deciding which Vars to decorate, and when considering pushing up a lower outer join pursuant to transforming the second form of OJ identity 3 to the first form, we only want to decorate Vars coming from its LHS. In a related bug, I realized that make_outerjoininfo was failing to check a very basic property that's needed to apply OJ identity 3: the syntactically-upper outer join clause can't refer to the lower join's LHS. This didn't break the join order restriction logic, but it led to setting bogus commute_xxx bits, possibly resulting in bogus nullingrel markings in modified quals. Richard Guo and Tom Lane Discussion: https://postgr.es/m/CAMbWs497CmBruMx1SOjepWEz+T5NWa4scqbdE9v7ZzSXqH_gQw@mail.gmail.com Discussion: https://postgr.es/m/CAEP4nAx9C5gXNBfEA0JBfz7B+5f1Bawt-RWQWyhev-wdps8BZA@mail.gmail.com --- src/backend/optimizer/plan/initsplan.c | 21 ++++++++++++++++----- src/test/regress/expected/join.out | 25 +++++++++++++++++++++++++ src/test/regress/sql/join.sql | 9 +++++++++ 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 904f710d59..2b2c6af9ef 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -1540,7 +1540,8 @@ make_outerjoininfo(PlannerInfo *root, } else if (jointype == JOIN_LEFT && otherinfo->jointype == JOIN_LEFT && - bms_overlap(strict_relids, otherinfo->min_righthand)) + bms_overlap(strict_relids, otherinfo->min_righthand) && + !bms_overlap(clause_relids, otherinfo->syn_lefthand)) { /* Identity 3 applies, so remove the ordering restriction */ min_lefthand = bms_del_member(min_lefthand, otherinfo->ojrelid); @@ -1985,12 +1986,18 @@ deconstruct_distribute_oj_quals(PlannerInfo *root, /* * When we are looking at joins above sjinfo, we are envisioning * pushing sjinfo to above othersj, so add othersj's nulling bit - * before distributing the quals. + * before distributing the quals. We should add it to Vars coming + * from the current join's LHS: we want to transform the second + * form of OJ identity 3 to the first form, in which Vars of + * relation B will appear nulled by the syntactically-upper OJ + * within the Pbc clause, but those of relation C will not. (In + * the notation used by optimizer/README, we're converting a qual + * of the form Pbc to Pb*c.) */ if (above_sjinfo) quals = (List *) add_nulling_relids((Node *) quals, - othersj->min_righthand, + sjinfo->syn_lefthand, bms_make_singleton(othersj->ojrelid)); /* Compute qualscope and ojscope for this join level */ @@ -2041,12 +2048,16 @@ deconstruct_distribute_oj_quals(PlannerInfo *root, /* * Adjust qual nulling bits for next level up, if needed. We * don't want to put sjinfo's own bit in at all, and if we're - * above sjinfo then we did it already. + * above sjinfo then we did it already. Here, we should mark all + * Vars coming from the lower join's RHS. (Again, we are + * converting a qual of the form Pbc to Pb*c, but now we are + * putting back bits that were there in the parser output and were + * temporarily stripped above.) */ if (below_sjinfo) quals = (List *) add_nulling_relids((Node *) quals, - othersj->min_righthand, + othersj->syn_righthand, bms_make_singleton(othersj->ojrelid)); /* ... and track joins processed so far */ diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 75f03fcf30..98d611412d 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -5068,6 +5068,31 @@ where current_user is not null; -- this is to add a Result node -> Seq Scan on int4_tbl i4 (6 rows) +-- and further discussion of bug #17781 +explain (costs off) +select * +from int8_tbl t1 + left join (int8_tbl t2 left join onek t3 on t2.q1 > t3.unique1) + on t1.q2 = t2.q2 + left join onek t4 + on t2.q2 < t3.unique2; + QUERY PLAN +------------------------------------------------- + Nested Loop Left Join + Join Filter: (t2.q2 < t3.unique2) + -> Nested Loop Left Join + Join Filter: (t2.q1 > t3.unique1) + -> Hash Left Join + Hash Cond: (t1.q2 = t2.q2) + -> Seq Scan on int8_tbl t1 + -> Hash + -> Seq Scan on int8_tbl t2 + -> Materialize + -> Seq Scan on onek t3 + -> Materialize + -> Seq Scan on onek t4 +(13 rows) + -- check that join removal works for a left join when joining a subquery -- that is guaranteed to be unique by its GROUP BY clause explain (costs off) diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 2e764cdd51..e50a769606 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1820,6 +1820,15 @@ from (select f1/2 as x from int4_tbl i4 left join a on a.id = i4.f1) ss1 right join int8_tbl i8 on true where current_user is not null; -- this is to add a Result node +-- and further discussion of bug #17781 +explain (costs off) +select * +from int8_tbl t1 + left join (int8_tbl t2 left join onek t3 on t2.q1 > t3.unique1) + on t1.q2 = t2.q2 + left join onek t4 + on t2.q2 < t3.unique2; + -- check that join removal works for a left join when joining a subquery -- that is guaranteed to be unique by its GROUP BY clause explain (costs off)