From: | Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> |
---|---|
To: | amul sul <sulamul(at)gmail(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, 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: | 2019-10-29 10:29:32 |
Message-ID: | CAPmGK176r4_KXCHvmtXZqC9TEhJn-WjROGo_pwS08MCD16zBmQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi Amul,
On Fri, Oct 25, 2019 at 6:59 PM amul sul <sulamul(at)gmail(dot)com> wrote:
> On Wed, Oct 16, 2019 at 6:20 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
>> 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:
Thanks for the review!
> 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?
I'm not sure that's a good idea; these functions might be called many
times, so I just thought it would be better to call these functions
conditionally, to avoid useless function call overhead.
> 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.
Seems like a good idea.
> 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.
Thanks for the patch!
> 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
> --
Good catch! Will fix.
Best regards,
Etsuro Fujita
From | Date | Subject | |
---|---|---|---|
Next Message | Vik Fearing | 2019-10-29 10:47:48 | Join Correlation Name |
Previous Message | Peter Eisentraut | 2019-10-29 10:06:05 | Add support for automatically updating Unicode derived files |