Horton

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

amul sul <sulamul@gmail.com>

View thread
From: amul sul <sulamul@gmail.com>
To: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>, Dmitry Dolgov <9erthalion6@gmail.com>, Thomas Munro <thomas.munro@enterprisedb.com>, Antonin Houska <ah@cybertec.at>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2019-10-25T09:59:03Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Suppress unused-variable warning.

  2. Allow partitionwise joins in more cases.

  3. Avoid crash in partitionwise join planning under GEQO.

  4. Disable support for partitionwise joins in problematic cases.

  5. Add plan_cache_mode setting

  6. Add test for partitionwise join involving default partition.

  7. Revise API for partition_rbound_cmp/partition_rbound_datum_cmp.

Attachments

On Wed, Oct 16, 2019 at 6:20 PM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

> On Wed, Sep 25, 2019 at 12:59 AM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> > I will continue to review the rest of the patch.
>
> I've been reviewing the rest of the patch.  Here are my review comments:
>
[....]

> So I'd like to propose to introduce separate functions like
> process_outer_partition() and process_inner_partition() in the
> attached, instead of handle_missing_partition().  (I added a fast path
> to these functions that if both outer/inner sides have the default
> partitions, give up on merging partitions.  Also, I modified the
> caller sites of proposed functions so that they call these if
> necessary.)
>

Agree -- process_outer_partition() & process_inner_partition() approach
looks
much cleaner than before.

Here are the few comments:

Note that LHS numbers are the line numbers in your previously posted
patch[1].

 455 +               if (inner_has_default ||
 456 +                   jointype == JOIN_LEFT ||
 457 +                   jointype == JOIN_ANTI ||
 458 +                   jointype == JOIN_FULL)
 459 +               {
 460 +                   if (!process_outer_partition(&outer_map,

 512 +               if (outer_has_default || jointype == JOIN_FULL)
 513 +               {
 514 +                   if (!process_inner_partition(&outer_map,

How about adding these conditions to the else block of
process_outer_partition()
& process_inner_partition() function respectively so that these functions
can be
called unconditionally?  Thoughts/Comments?
---

 456 +                   jointype == JOIN_LEFT ||
 457 +                   jointype == JOIN_ANTI ||
 458 +                   jointype == JOIN_FULL)

Also, how about using IS_OUTER_JOIN() instead. But we need an assertion to
restrict JOIN_RIGHT or something.

For the convenience, I did both aforesaid changes in the attached delta
patch that
can be applied atop of your previously posted patch[1].  Kindly have a look
& share
your thoughts, thanks.
--

1273 + * *next_index is incremented when creating a new merged partition
associated
1274 + * with the given outer partition.
1275 + */

Correction: s/outer/inner
---

1338 +        * In range partitioning, if the given outer partition is
already
1339 +        * merged (eg, because we found an overlapping range earlier),
we know
1340 +        * where it fits in the join result; nothing to do in that
case.  Else
1341 +        * create a new merged partition.

Correction: s/outer/inner.
---

1712         /*
1713          * If the NULL partition was missing from the inner side of
the join,

s/inner side/outer side
--

Regards,
Amul

1]
https://postgr.es/m/CAPmGK145V8DNCNQ2gQBgNE3QqoJGjsmK5WMwaA3FMirNM723KQ%40mail.gmail.com