Re: [HACKERS] advanced partition matching algorithm forpartition-wise join - Mailing list pgsql-hackers

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