Re: [HACKERS] advanced partition matching algorithm for partition-wise join

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

> >> On Tue, Jan 28, 2020 at 1:39 PM Mark Dilger
> >> <mark(dot)dilger(at)enterprisedb(dot)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, not mine.
> >>>
> >>> The v31-0002-… patch adds more regression tests for range partitioning. The commit message contains my comments about that.
> >>>
> >>> The v31-0003-… patch adds more regression tests for list partitioning, and again, the commit message contains my comments about 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-wise join patch should do anything differently in this case. Again, it sounds like this is beyond the scope of the 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 partitioning logic to verify that did not confuse the code in your patch.

OK, thanks for the testing!

Best regards,
Etsuro Fujita