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

Dmitry Dolgov <9erthalion6@gmail.com>

From: Dmitry Dolgov <9erthalion6@gmail.com>

To: ashutosh.bapat.oss@gmail.com

Cc: Robert Haas <robertmhaas@gmail.com>, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>, Antonin Houska <ah@cybertec.at>, PostgreSQL-development <pgsql-hackers@postgresql.org>

Date: 2018-10-25T21:19:09Z

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 →

Suppress unused-variable warning.
- 401418ca6a68 13.0 landed
Allow partitionwise joins in more cases.
- c8434d64ce03 13.0 landed
Avoid crash in partitionwise join planning under GEQO.
- 7ad6498fd5a6 12.0 cited
- d70c147fa217 11.3 cited
Disable support for partitionwise joins in problematic cases.
- 7cfdc77023ad 12.0 cited
Add plan_cache_mode setting
- f7cb2842bf47 12.0 cited
Add test for partitionwise join involving default partition.
- 4513d3a4be0b 12.0 cited
Revise API for partition_rbound_cmp/partition_rbound_datum_cmp.
- b0229235564f 11.0 landed

Attachments

0001-Hash-partition-bound-equality-refactoring-v12.patch (application/octet-stream) patch v12-0001
0002-Targetlist-of-a-child-join-is-produced-by-translating-v12.patch (application/octet-stream) patch v12-0002
0003-Partition-wise-join-for-1-1-1-0-0-1-partition-matching-v12.patch (application/octet-stream) patch v12-0003
0004-Tests-for-0-1-1-1-and-1-0-partition-matching-v12.patch (application/octet-stream) patch v12-0004

> On Mon, 17 Sep 2018 at 11:20, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>
> I am fine with that. It was never meant to be committed. I used to run those
> tests to make sure that any changes to the core logic do not break any
> working scenarios. Whenever I found a new failure in the extra tests which
> wasn't there in tests to be committed, I used to move that test from the
> first to the second. Over the time, the number of new failures in extra has
> reduced and recently I didn't see any extra failures. So, may be it's time
> for the extra tests to be dropped. I will suggest that keep the extra tests
> running from time to time and certainly around the time the feature gets
> committed.

Great, that's exactly what I'm doing right now - I keep these tests locally
to monitor any significant failures except any changes in plans, but without
including it into the patch series.

Since most of my complaints about the patch were related to code readability,
I modified it a bit to show more clearly what I have in mind. I haven't changed
anything about the functionality, just adjusted some parts of it:

* removed some unused arguments (looks like they were added for consistency in
  all higher level branches, but not all of them were actually in use)

* replaced representation for partition mapping (instead of int arrays there is
  a structure, that allows to replace 0/1 with more meaningful from/to)

* tried to make naming a bit more consistent - so, if a function doesn't
  explicitely say anything about outer/inner, we have partmap1/partmap2,
  otherwise outermap/innermap. I don't really like this style with
  partmap1/partmap2 or index1/index2, but it seems consistent with the rest of
  the code in partbounds.c

* removed some state representation flags, e.g. merged - instead, if there is a
  situation when we can't merge, functions will return NULL right away

* removed questionable debugging statement

Ashutosh, can you please take a look at it and confirm (or not) that you also
think these changes have improved readability a bit. If we're on the same page
about that, I'll continue in this direction.