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

On Tue, Oct 29, 2019 at 7:29 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> 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.

> > 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.

The overhead might be small, but isn't zero, so I still think that we
should call these functions if necessary.

> > 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.

Done.

> > 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.

Done.

I also added some assertions to process_outer_partition() and
process_inner_partition(), including ones as proposed in your patch.
Attached is an updated version. If no objections, I'll merge this
with the main patch [1].

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/CAPmGK14WHKckT1P6UJV2B63TZAxPyMn8iZJ99XF=AZuNhG6vow@mail.gmail.com