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

On Fri, Nov 22, 2019 at 10:08 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> 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.

Moved to the next CF.

Best regards,
Etsuro Fujita