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

On Fri, Nov 15, 2019 at 6:10 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> On Fri, Nov 15, 2019 at 2:21 PM amul sul <sulamul(at)gmail(dot)com> wrote:
> > Thank you Fujita san for the patch & the enhancements. I am fine with your
> > delta patch.
>
> OK, I'll merge the delta patch with the main one in the next version,
> if no objections from others.
>
> > I would like to share some thought for other code:
> >
> > File: partbounds.c:
> > 3298 static void
> > 3299 get_merged_range_bounds(int partnatts, FmgrInfo *partsupfuncs,
> > 3300 Oid *partcollations, JoinType jointype,
> > 3301 PartitionRangeBound *left_lb,
> > 3302 PartitionRangeBound *left_ub,
> > 3303 PartitionRangeBound *right_lb,
> > 3304 PartitionRangeBound *right_ub,
> > 3305 PartitionRangeBound *merged_lb,
> > 3306 PartitionRangeBound *merged_ub)
> >
> > Can we rename these argument as inner_* & outer_* like we having for the
> > functions, like 0003 patch?
>
> +1 (Actually, I too was thinking about that.)
>
> > File: partbounds.c:
> > 3322
> > 3323 case JOIN_INNER:
> > 3324 case JOIN_SEMI:
> > 3325 if (compare_range_bounds(partnatts, partsupfuncs, partcollations,
> > 3326 left_ub, right_ub) < 0)
> > 3327 *merged_ub = *left_ub;
> > 3328 else
> > 3329 *merged_ub = *right_ub;
> > 3330
> > 3331 if (compare_range_bounds(partnatts, partsupfuncs, partcollations,
> > 3332 left_lb, right_lb) > 0)
> > 3333 *merged_lb = *left_lb;
> > 3334 else
> > 3335 *merged_lb = *right_lb;
> > 3336 break;
> > 3337
> >
> > How about reusing ub_cmpval & lb_cmpval instead of calling
> > compare_range_bounds() inside get_merged_range_bounds(), like 0004 patch?
>
> Good idea! So, +1
>
> > Apart from this, I would like to propose 0005 cleanup patch where I have
> > rearranged function arguments & code to make sure the arguments & the code
> > related to lower bound should come first and then the upper bound.
>
> +1
>
> I'll merge these changes as well into the main patch, if no objections
> of others.

Done. I modified compare_range_partitions() as well to match its the
variable names with others. Attached is a new version of the patch.

I reviewed the rest of the partbounds.c changes. Here are my review comments:

* I don't think this analysis is correct:

+/*
+ * merge_null_partitions
+ *
+ * Merge NULL partitions, i.e. a partition that can hold NULL values for a lis\
t
+ * partitioned table, if any. Find the index of merged partition to which the
+ * NULL values would belong in the join result. If one joining relation has a
+ * NULL partition but not the other, try matching it with the default partitio\
n
+ * from the other relation since the default partition may have rows with NULL
+ * partition key. We can eliminate a NULL partition when it appears only on th\
e
+ * inner side of the join and the outer side doesn't have a default partition.
+ *
+ * When the equality operator used for join is strict, two NULL values will no\
t
+ * be considered as equal, and thus a NULL partition can be eliminated for an
+ * inner join. But we don't check the strictness operator here.
+ */

First of all, I think we can assume here that the equality operator is
*strict*, because 1) have_partkey_equi_join(), which is executed
before calling merge_null_partitions(), requires that the
corresponding clause is mergejoinable, and 2) currently, we assume
that mergejoinable operators are strict (see MJEvalOuterValues() and
MJEvalInnerValues()). So I don't think it's needed to try merging a
NULL partition on one side with the default partition on the other
side as above. (merge_null_partitions() tries merging NULL partitions
as well, but in some cases I don't think we need to do that, either.)
So I rewrote merge_null_partitions() completely. Another change is
that I eliminated the NULL partition of the joinrel in more cases.
Attached is a patch (v28-0002-modify-merge_null_partitions.patch) for
that created on top of the main patch. I might be missing something,
though.

Other changes in that patch:

* I fixed memory leaks in partition_list_bounds_merge() and
partition_range_bounds_merge().

* I noticed this in merge_default_partitions():

+ Assert(outer_has_default && inner_has_default);
+
+ *default_index = map_and_merge_partitions(outer_map,
+ inner_map,
+ outer_default,
+ inner_default,
+ next_index);
+ if (*default_index == -1)
+ return false;

I think the merging should be done successfully, because of 1) this in
process_outer_partition():

+ if (inner_has_default)
+ {
+ Assert(inner_default >= 0);
+
+ /*
+ * If the outer side has the default partition as well, we need to
+ * merge the default partitions (see merge_default_partitions()); give
+ * up on it.
+ */
+ if (outer_has_default)
+ return false;

and 2) this in process_inner_partition():

+ if (outer_has_default)
+ {
+ Assert(outer_default >= 0);
+
+ /*
+ * If the inner side has the default partition as well, we need to
+ * merge the default partitions (see merge_default_partitions()); give
+ * up on it.
+ */
+ if (inner_has_default)
+ return false;

So I removed the above if test (ie, *default_index == -1). I also
modified that function a bit further, including comments.

* I simplified process_outer_partition() and process_inner_partition()
a bit, changing the APIs to match that of map_and_merge_partitions().
Also, I think this in these functions is a bit ugly;

+ /* Don't add this index to the list of merged indexes. */
+ *merged_index = -1;

so I removed it, and modified the condition on whether or not we add
merged bounds to the lists in partition_list_bounds_merge() and
partition_range_bounds_merge(), instead.

That's it. Sorry for the delay.

Best regards,
Etsuro Fujita