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

On Tue, Oct 29, 2019 at 7:29 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Fri, Oct 25, 2019 at 6:59 PM amul sul <sulamul@gmail.com> wrote:
> > On Wed, Oct 16, 2019 at 6:20 PM Etsuro Fujita <etsuro.fujita@gmail.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