Re: [HACKERS] advanced partition matching algorithm forpartition-wise join - Mailing list pgsql-hackers

On Thu, Feb 6, 2020 at 2:03 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> > On Feb 5, 2020, at 4:51 AM, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

> >> On Tue, Jan 28, 2020 at 1:39 PM Mark Dilger
> >> <mark.dilger@enterprisedb.com> wrote:
> >>> I have added tests checking correctness and showing some partition pruning limitations.  Find three patches,
attached.
> >>>
> >>> The v31-0001-… patch merely applies your patches as a starting point for the next two patches.  It is your work,
notmine. 
> >>>
> >>> The v31-0002-… patch adds more regression tests for range partitioning.  The commit message contains my comments
aboutthat. 
> >>>
> >>> The v31-0003-… patch adds more regression tests for list partitioning, and again, the commit message contains my
commentsabout that. 

> > I tested the new test patches.  The patches are applied to the
> > partition_join.sql regression test cleanly, but the new tests failed
> > in my environment (even with make check LANG=C).  I think I should set
> > the right locale for the testing.  Which one did you use?
>
> I did not set a locale in the tests.  My environment has:
>
> LANG="en_US.UTF-8"
> LC_COLLATE="en_US.UTF-8"
> LC_CTYPE="en_US.UTF-8"
> LC_MESSAGES="en_US.UTF-8"
> LC_MONETARY="en_US.UTF-8"
> LC_NUMERIC="en_US.UTF-8"
> LC_TIME="en_US.UTF-8"
> LC_ALL=

Thanks for the information!

> >  Maybe I'm
> > missing something, but let me comment on the new tests.  This is the
> > one proposed in the v31-0002 patch:
> >
> > +EXPLAIN (COSTS OFF)
> > +SELECT t1.a, t2.a FROM alpha t1 INNER JOIN beta t2 ON (t1.a = t2.a)
> > WHERE t1.a IN ('äbç', 'ὀδυσσεύς');
> > +                            QUERY PLAN
> > +------------------------------------------------------------------
> > + Hash Join
> > +   Hash Cond: (t2.a = t1.a)
> > +   ->  Append
> > +         ->  Seq Scan on beta_a t2_1
> > +         ->  Seq Scan on beta_b t2_2
> > +         ->  Seq Scan on beta_c t2_3
> > +         ->  Seq Scan on beta_d t2_4
> > +         ->  Seq Scan on beta_e t2_5
> > +         ->  Seq Scan on beta_f t2_6
> > +         ->  Seq Scan on beta_g t2_7
> > +         ->  Seq Scan on beta_h t2_8
> > +         ->  Seq Scan on beta_default t2_9
> > +   ->  Hash
> > +         ->  Append
> > +               ->  Seq Scan on alpha_e t1_1
> > +                     Filter: (a = ANY ('{äbç,ὀδυσσεύς}'::text[]))
> > +               ->  Seq Scan on alpha_default t1_2
> > +                     Filter: (a = ANY ('{äbç,ὀδυσσεύς}'::text[]))
> > +(18 rows)
> >
> > The commit message says:
> >
> >    When joining with
> >
> >        alpha.a = beta.a and alpha.a IN ('äbç', 'ὀδυσσεύς')
> >
> >    the planner does the right thing for one side of the query, but
> >    hits all partitions for the other side, which it doesn't need to
> >    do.
> >
> > Yeah, I agree that the resulting plan is not efficient.  The reason
> > for that would be that the planner doesn't generate a qual on the
> > outer side matching the ScalarArrayOpExpr qual "a = ANY
> > ('{äbç,ὀδυσσεύς}'::text[])" on the inner side, which I think would be
> > a restriction caused by the equivalence machinery not by the
> > partitionwise join logic IIUC.
>
> It’s fine if this is beyond the scope of the patch.
>
> >  I think the critique would be useful,
> > so I don't object to adding this test case, but the critique would be
> > more about query planning that is actually not related to the
> > partitionwise join logic, so I'm not sure that the partition_join.sql
> > regression test is the best place to add it.  I guess that there would
> > be much better places than partition_join.sql.
>
> You don’t need to add the test anywhere.  It’s enough for me that you looked at it and considered whether the
partition-wisejoin patch should do anything differently in this case.  Again, it sounds like this is beyond the scope
ofthe patch. 

OK

> > (This is nitpicking;
> > but another thing I noticed about this test case is that the join
> > query contains only a single join condition "t1.a = t2.a", but the
> > queried tables alpha and beta are range-partitioned by multiple
> > columns a and b, so the query should have a join condition for each of
> > the columns like "t1.a = t2.a AND t1.b = t2.b" if adding this as a
> > test case for partitionwise join.)
>
> Well, it is important that partition-wise join does not break such queries.  I added the column ‘b’ to the
partitioninglogic to verify that did not confuse the code in your patch. 

OK, thanks for the testing!

Best regards,
Etsuro Fujita