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

From: Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
To: ashutosh(dot)bapat(dot)oss(at)gmail(dot)com
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Antonin Houska <ah(at)cybertec(dot)at>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] advanced partition matching algorithm for partition-wise join
Date: 2018-10-25 21:19:09
Message-ID: CA+q6zcUoEnZO0WLcunr5Z3QzC-raZkF6tGOGY73M=kAbT9J-EA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

> On Mon, 17 Sep 2018 at 11:20, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)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.

Attachment Content-Type Size
0001-Hash-partition-bound-equality-refactoring-v12.patch application/octet-stream 5.1 KB
0002-Targetlist-of-a-child-join-is-produced-by-translating-v12.patch application/octet-stream 3.5 KB
0003-Partition-wise-join-for-1-1-1-0-0-1-partition-matching-v12.patch application/octet-stream 69.5 KB
0004-Tests-for-0-1-1-1-and-1-0-partition-matching-v12.patch application/octet-stream 220.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2018-10-25 22:29:45 PostgreSQL Limits and lack of documentation about them.
Previous Message Nikolay Samokhvalov 2018-10-25 21:10:02 Re: Using old master as new replica after clean switchover