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

On Sun, Jul 15, 2018 at 11:13 PM, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>> On Thu, 28 Jun 2018 at 07:54, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>
>> On 2018/06/27 22:21, Ashutosh Bapat wrote:
>> > On Wed, Jun 27, 2018 at 12:28 PM, Amit Langote
>> >> Ah, okay. I thought of reporting this because I felt the errors may have
>> >> to do with changes to the related code in HEAD between May 14 when you
>> >> last posted the patches and today that you may need to account for in you
>> >> patches. For instance, there are many diffs like this:
>> >>
>> >> Looks like the Result node on top of Append is no longer there after
>> >> applying your patch.
>> >
>> > Yes. They are coming because of a commit which removed Result node on
>> > top of an Append node. I don't remember exactly which.
>> >
>> > I wouldn't worry about those diffs at this time. As I have mentioned
>> > in earlier mails, the expected output from 0006 is quite large and is
>> > not supposed to be committed. So, I don't see much value in fixing the
>> > plans in that output.
>> >
>> > Do you see that as a hindrance in reviewing the code changes and tests in 0005?
>>
>> I think not. I'll ignore 0006 for now and focus on other patches.
>
> Hi,
>
> Sorry for my irregular reviews. Unfortunately, the patch need to be rebased
> again.

I rebased the patches without any problem on top of commit
commit f7cb2842bf47715133b40e4a503f35dbe60d1b72
Author: Peter Eisentraut <peter_e(at)gmx(dot)net>
Date: Mon Jul 16 13:35:41 2018 +0200

Add plan_cache_mode setting

This allows overriding the choice of custom or generic plan.

Author: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Discussion:
https://www.postgresql.org/message-id/flat/CAFj8pRAGLaiEm8ur5DWEBo7qHRWTk9HxkuUAz00CZZtJj-LkCA%40mail.gmail.com

I didn't get any problem rebasing the patches. They compiled cleanly.
The tests added by commit 4513d3a4be0bb7d0141f8b7eaf669a55c08e41b0
failed since this patch-set changes the partitions of the tables used
in the test. Adjusted those tests accordingly.

> In the meantime I have few more commentaries about range_bounds_merge:
>
> * From what I see partition_range_bound_cmp is executed on the same bounds few
> times to find whether they are overlapping and during the range merging, is
> it necessary? Probably it would be enough just to compare current ranges once
> per iteration.

The bounds that are compared in those cases are different. Any two
bounds are compared only once per iteration. Remember each range has
two bounds, thus there are total four comparisons possible. In case of
overlap, we do all four comparisons.

>
> * Just to clarify - the iterating through all the partitions, is it the best
> way of finding matching ranges? Correct me if I'm wrong, but from what I see
> in the comments for PartitionBoundInfoData, all the ranges are arranged in
> increasing order - maybe then it's possible to use binary search for finding
> a matching range?

The loop in partition_range_bounds_merge() runs as many times as the
maximum of number of datums from the given partition bounds. So the
complexity is O(n) where n is the maximum of number of datums. With
binary search the complexity will increase to O(nlogn). I might be
missing something here.

>
> * I'm not sure why in this case partition wise join is not applied? Maybe I'm
> wrong, but I was under the impression that it should work here
>
> =# \d+ test1;
> Table "public.test1"
> Column | Type | Collation | Nullable | Default | Storage |
> --------+---------+-----------+----------+---------+----------+
> data | jsonb | | | | extended |
> id | integer | | | | plain |
> Partition key: RANGE (id)
> Indexes:
> "test1_idx" btree (id)
> Partitions: test11 FOR VALUES FROM (0) TO (100),
> test12 FOR VALUES FROM (100) TO (200),
> test13 FOR VALUES FROM (200) TO (300)
>
> =# \d+ test2;
> Table "public.test2"
> Column | Type | Collation | Nullable | Default | Storage |
> --------+---------+-----------+----------+---------+----------+
> data | jsonb | | | | extended |
> id | integer | | | | plain |
> Partition key: RANGE (id)
> Indexes:
> "test2_idx" btree (id)
> Partitions: test21 FOR VALUES FROM (10) TO (110),
> test22 FOR VALUES FROM (110) TO (210),
> test23 FOR VALUES FROM (210) TO (310)
>
>
> I'm confused, since there is only one-to-one mapping of partitions.

In this case, test21 overlaps test11 (10-100) and test12 (100-110),
test22 overlaps test12 (110-200) and test13(200-210). So, there is no
one-to-one mapping.

PFA rebased patches with tests fixed as described above.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company