Horton

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

Kuntal Ghosh <kuntalghosh.2007@gmail.com>

View thread
From: Kuntal Ghosh <kuntalghosh.2007@gmail.com>
To: Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>
Cc: Etsuro Fujita <etsuro.fujita@gmail.com>, Tomas Vondra <tomas.vondra@2ndquadrant.com>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, amul sul <sulamul@gmail.com>, Robert Haas <robertmhaas@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>, Dmitry Dolgov <9erthalion6@gmail.com>, Antonin Houska <ah@cybertec.at>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2020-04-08T15:06:19Z
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.

Hello Ashutosh, Fujita,

On Wed, Apr 8, 2020 at 3:49 PM Ashutosh Bapat
<ashutosh.bapat@2ndquadrant.com> wrote:
> On Wed, 8 Apr 2020 at 15:42, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> On Wed, Apr 8, 2020 at 4:30 PM Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
>> > I'm getting the following warning during compilation.
>> >
>> > partbounds.c: In function ‘partition_bounds_merge’:
>> > partbounds.c:1024:21: warning: unused variable ‘inner_binfo’ [-Wunused-variable]
>> >   PartitionBoundInfo inner_binfo = inner_rel->boundinfo;
>> >                      ^
>> > For fixing the same, we can declare inner_binfo as
>> > PG_USED_FOR_ASSERTS_ONLY as it is not used for any other purpose.
>>
>> I'd propose to remove an assertion causing this (and  the
>> outer_binfo/inner_binfo variables) from partition_bounds_merge(),
>> rather than doing so, because the assertion is redundant, as we have
>> the same assertion in merge_list_bounds() and merge_range_bounds().
>> Please find attached a patch.
>
>
> I think it's better to have the assertion in all the three places and also in merge_hash_bounds() whenever that comes along. The assertion in merge_*_bounds() will be good to in case those functions are called from places other than partition_bounds_merge(). The assertion in partition_bounds_merge() will make sure that when the individual merge_*_bounds() functions are called based on one of the bounds both of the bounds have same strategy.

Both of your patches fix the problem. I don't have much exposure in
this area to comment on whether we should keep/remove the assertion
from the code. But, here is my opinion:

The code structure looks like following:
Assert(condition A);
if (Condition B)
    merge_*_bounds(....);

Inside merge_*_bounds(), you have both the above assert and the if
condition as another assert:
Assert(condition A and Condition B);

And, merge_*_bounds() are called from only one place. So, something is
redundant here and I'm inclined towards removal of the assert
condition. Another thing I noticed:

/* The partitioning strategies should be the same. */
Assert(outer_binfo->strategy == inner_binfo->strategy);

The comment just reads the assertion aloud which looks unnecessary.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com