On Tue, Oct 29, 2019 at 7:29 PM Etsuro Fujita <[email protected]> wrote: > On Fri, Oct 25, 2019 at 6:59 PM amul sul <[email protected]> wrote: > > On Wed, Oct 16, 2019 at 6:20 PM Etsuro Fujita <[email protected]> > > 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=azunhg6...@mail.gmail.com
modify-partbounds-changes-2.patch
Description: Binary data
