Thread
Commits
-
Fix some issues with wrong placement of pseudo-constant quals.
- a75ff55c8334 16.0 landed
-
wrong query result due to wang plan
Tender Wang <tndrwang@gmail.com> — 2023-02-16T07:16:23Z
Hi hackers, I have this query as shown below: select ref_1.r_comment as c0, subq_0.c1 as c1 from public.region as sample_0 right join public.partsupp as sample_1 right join public.lineitem as sample_2 on (cast(null as path) = cast(null as path)) on (cast(null as "timestamp") < cast(null as "timestamp")) inner join public.lineitem as ref_0 on (true) left join (select sample_3.ps_availqty as c1, sample_3.ps_comment as c2 from public.partsupp as sample_3 where false order by c1, c2 ) as subq_0 on (sample_1.ps_supplycost = subq_0.c1 ) right join public.region as ref_1 on (sample_1.ps_availqty = ref_1.r_regionkey ) where ref_1.r_comment is not NULL order by c0, c1; *This query has different result on pg12.12 and on HEAD*, on pg12.12: c0 | c1 -----------------------------------------------------------------------------------------------------------------+---- even, ironic theodolites according to the bold platelets wa | furiously unusual packages use carefully above the unusual, exp | silent, bold requests sleep slyly across the quickly sly dependencies. furiously silent instructions alongside | special, bold deposits haggle foxes. platelet | special Tiresias about the furiously even dolphins are furi | (5 rows) its plan : QUERY PLAN ------------------------------------------------------ Sort Sort Key: ref_1.r_comment, c1 -> Hash Left Join Hash Cond: (ref_1.r_regionkey = ps_availqty) -> Seq Scan on region ref_1 Filter: (r_comment IS NOT NULL) -> Hash -> Result One-Time Filter: false (9 rows) But on HEAD(pg16devel), its results below: c0 | c1 ----+---- (0 rows) its plan: QUERY PLAN ---------------------------------------- Sort Sort Key: ref_1.r_comment, subq_0.c1 -> Result One-Time Filter: false (4 rows) Attached file included table schema info. regards, tender wang -
Re: wrong query result due to wang plan
Richard Guo <guofenglinux@gmail.com> — 2023-02-16T09:50:59Z
On Thu, Feb 16, 2023 at 3:16 PM tender wang <tndrwang@gmail.com> wrote: > select ref_1.r_comment as c0, subq_0.c1 as c1 from public.region as > sample_0 right join public.partsupp as sample_1 right join public.lineitem > as sample_2 on (cast(null as path) = cast(null as path)) on (cast(null as > "timestamp") < cast(null as "timestamp")) inner join public.lineitem as > ref_0 on (true) left join (select sample_3.ps_availqty as c1, > sample_3.ps_comment as c2 from public.partsupp as sample_3 where false > order by c1, c2 ) as subq_0 on (sample_1.ps_supplycost = subq_0.c1 ) right > join public.region as ref_1 on (sample_1.ps_availqty = ref_1.r_regionkey ) > where ref_1.r_comment is not NULL order by c0, c1; > The repro can be reduced to the query below. create table t (a int, b int); # explain (costs off) select * from t t1 left join (t t2 inner join t t3 on false left join t t4 on t2.b = t4.b) on t1.a = t2.a; QUERY PLAN -------------------------- Result One-Time Filter: false (2 rows) As we can see, the joinrel at the final level is marked as dummy, which is wrong. I traced this issue down to distribute_qual_to_rels() when we handle variable-free clause. If such a clause is not an outer-join clause, and it contains no volatile functions either, we assign it the full relid set of the current JoinDomain. I doubt this is always correct. Such as in the query above, the clause 'false' is assigned relids {t2, t3, t4, t2/t4}. And that makes it a pushed down restriction to the second left join. This is all right if we plan this query in the user-given order. But if we've commuted the two left joins, which is legal, this pushed down and constant false restriction would make the final joinrel be dummy. It seems we still need to check whether a variable-free qual comes from somewhere that is below the nullable side of an outer join before we decide that it can be evaluated at join domain level, just like we did before. So I wonder if we can add a 'below_outer_join' flag in JoinTreeItem, fill its value during deconstruct_recurse, and check it in distribute_qual_to_rels() like /* eval at join domain level if not below outer join */ - relids = bms_copy(jtitem->jdomain->jd_relids); + relids = jtitem->below_outer_join ? + bms_copy(qualscope) : bms_copy(jtitem->jdomain->jd_relids); Thanks Richard -
Re: wrong query result due to wang plan
Richard Guo <guofenglinux@gmail.com> — 2023-02-16T10:14:40Z
On Thu, Feb 16, 2023 at 5:50 PM Richard Guo <guofenglinux@gmail.com> wrote: > It seems we still need to check whether a variable-free qual comes from > somewhere that is below the nullable side of an outer join before we > decide that it can be evaluated at join domain level, just like we did > before. So I wonder if we can add a 'below_outer_join' flag in > JoinTreeItem, fill its value during deconstruct_recurse, and check it in > distribute_qual_to_rels() like > > /* eval at join domain level if not below outer join */ > - relids = bms_copy(jtitem->jdomain->jd_relids); > + relids = jtitem->below_outer_join ? > + bms_copy(qualscope) : bms_copy(jtitem->jdomain->jd_relids); > To be concrete, I mean something like attached. Thanks Richard
-
Re: wrong query result due to wang plan
Richard Guo <guofenglinux@gmail.com> — 2023-02-17T07:35:28Z
On Thu, Feb 16, 2023 at 5:50 PM Richard Guo <guofenglinux@gmail.com> wrote: > It seems we still need to check whether a variable-free qual comes from > somewhere that is below the nullable side of an outer join before we > decide that it can be evaluated at join domain level, just like we did > before. So I wonder if we can add a 'below_outer_join' flag in > JoinTreeItem, fill its value during deconstruct_recurse, and check it in > distribute_qual_to_rels() > It occurs to me that we can leverage JoinDomain to tell if we are below the nullable side of a higher-level outer join if the clause is not an outer-join clause. If we are belew outer join, the current JoinDomain is supposed to be a proper subset of top JoinDomain. Otherwise the current JoinDomain must be equal to top JoinDomain. And that leads to a fix as code changes below. --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -2269,8 +2269,16 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause, } else { - /* eval at join domain level */ - relids = bms_copy(jtitem->jdomain->jd_relids); + JoinDomain *top_jdomain = + linitial_node(JoinDomain, root->join_domains); + + /* + * eval at original syntactic level if we are below an outer join, + * otherwise eval at join domain level, which is actually the top + * of tree + */ + relids = jtitem->jdomain == top_jdomain ? + bms_copy(jtitem->jdomain->jd_relids) : bms_copy(qualscope); Thanks Richard -
Re: wrong query result due to wang plan
Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-17T20:26:27Z
Richard Guo <guofenglinux@gmail.com> writes: > It occurs to me that we can leverage JoinDomain to tell if we are below > the nullable side of a higher-level outer join if the clause is not an > outer-join clause. If we are belew outer join, the current JoinDomain > is supposed to be a proper subset of top JoinDomain. Otherwise the > current JoinDomain must be equal to top JoinDomain. And that leads to a > fix as code changes below. That doesn't look right at all: surely this situation can occur further down in a join tree, not only just below top level. I thought about this some more and realized that the whole business of trying to push a qual up the join tree is probably obsolete. In the first place, there's more than one problem with assigning the ON FALSE condition the required_relids {t2, t3, t4, t2/t4}. As you say, it causes bogus conclusions about which joinrel can be considered empty if we commute the outer joins' order. But also, doing it this way loses the information that t2/t3 can be considered empty, if we do the joins in an order where that is useful to know. In the second place, I think recording the info that t2/t3 is empty is probably sufficient now, because of the mark_dummy_rel/is_dummy_rel bookkeeping in joinrels.c (which did not exist when we first added this "push to top of tree" hack). If we know t2/t3 is empty then we will propagate that knowledge to {t2, t3, t4, t2/t4} when it's formed, without needing to have a clause that can be applied at that join level. So that leads to a conclusion that we could just forget the whole thing and always use the syntactic qualscope here. I tried that and it doesn't break any existing regression tests, which admittedly doesn't prove a lot in this area :-(. However, we'd then need to decide what to do in process_implied_equality, which is annotated as * "qualscope" is the nominal syntactic level to impute to the restrictinfo. * This must contain at least all the rels used in the expressions, but it * is used only to set the qual application level when both exprs are * variable-free. (Hence, it should usually match the join domain in which * the clause applies.) and indeed equivclass.c is passing a join domain relid set. It's not hard to demonstrate that that path is also broken, for instance explain (costs off) select * from int8_tbl t1 left join (int8_tbl t2 inner join int8_tbl t3 on (t2.q1-t3.q2) = 0 and (t2.q1-t3.q2) = 1 left join int8_tbl t4 on t2.q2 = t4.q2) on t1.q1 = t2.q1; One idea I played with is that we could take the join domain relid set and subtract off the OJ relid and RHS relids of any fully-contained SpecialJoinInfos, reasoning that we can reconstruct the fact that those won't make the join domain's overall result nondummy, and thereby avoiding the possibility of confusion if we end up commuting with one of those joins. This feels perhaps overly complicated, though, compared to the brain-dead-simple "use the syntactic scope" approach. Maybe we can get equivclass.c to do something equivalent to that? Or maybe we only need to use this complex rule in process_implied_equality? (I'm also starting to realize that the current basically-syntactic definition of join domains isn't really going to work with commutable outer joins, so maybe the ultimate outcome is that the join domain itself is defined in this more narrowly scoped way. But that feels like a task to tackle later.) regards, tom lane -
Re: wrong query result due to wang plan
Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-19T19:01:13Z
I wrote: > So that leads to a conclusion that we could just forget the whole > thing and always use the syntactic qualscope here. I tried that > and it doesn't break any existing regression tests, which admittedly > doesn't prove a lot in this area :-(. Hmm, I thought it worked, but when I tried it again just now I see a failure (well, a less efficient plan) for one of the recently added test cases. I also tried the idea of stripping off lower outer joins, but that doesn't work in distribute_qual_to_rels, because we haven't yet formed the SpecialJoinInfos for all the outer joins that are below the top of the join domain. So for now I agree with your solution in distribute_qual_to_rels. It's about equivalent to what we did in older branches, and it's not real clear that cases with pseudoconstant quals in lower join domains are common enough to be worth sweating over. We still have to fix process_implied_equality though, and in that context we do have all the SpecialJoinInfos, so the strip-the-outer-joins fix seems to work. See attached. regards, tom lane
-
Re: wrong query result due to wang plan
Richard Guo <guofenglinux@gmail.com> — 2023-02-20T09:51:30Z
On Mon, Feb 20, 2023 at 3:01 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > We still have to fix process_implied_equality though, and in that > context we do have all the SpecialJoinInfos, so the strip-the-outer-joins > fix seems to work. See attached. Yeah, process_implied_equality is also broken for variable-free clause. I failed to notice that :-(. I'm looking at the strip-the-outer-joins codes. At first I wondered why it only removes JOIN_LEFT outer joins from below a JoinDomain's relids. After a second thought I think it's no problem here since only left outer joins have chance to commute with joins outside the JoinDomain. I'm thinking that maybe we can do the strip-the-outer-joins work only when it's not the top JoinDomain. When we are in the top JoinDomain, it seems to me that it's safe to push the qual to the top of the tree. - /* eval at join domain level */ - relids = bms_copy(qualscope); + /* eval at join domain's safe level */ + if (!bms_equal(qualscope, + ((JoinDomain *) linitial(root->join_domains))->jd_relids)) + relids = get_join_domain_min_rels(root, qualscope); + else + relids = bms_copy(qualscope); Thanks Richard
-
Re: wrong query result due to wang plan
Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-22T17:41:52Z
Richard Guo <guofenglinux@gmail.com> writes: > I'm thinking that maybe we can do the strip-the-outer-joins work only > when it's not the top JoinDomain. When we are in the top JoinDomain, it > seems to me that it's safe to push the qual to the top of the tree. Yeah, because there's nothing to commute with. Might as well do that for consistency with the distribute_qual_to_rels behavior, although I felt it was better to put it inside get_join_domain_min_rels. Pushed, thanks for the report! regards, tom lane