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

On Thu, Mar 22, 2018 at 4:32 AM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>> On 12 March 2018 at 06:00, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote:
>> Thanks for the note. Here are rebased patches.
>
> Since I started to look at this patch, I can share few random notes (although
> it's not a complete review, I'm in the progress now), maybe it can be helpful.
>
> In `partition_range_bounds_merge`
>
>     + if (!merged)
>     +     break;
>
> is a bit redundant I think, because every time `merged` set to false it
> followed by break.

Yes, right now. May be I should turn it into Assert(merged); What do you think?

>
> At the end same function
>
>     + if (merged)
>     +     merged = merge_default_partitions(outer_bi, outer_pmap, outer_mmap,
>     +         inner_bi, inner_pmap, inner_mmap,
>     +         jointype, &next_index,
>     +         &default_index);
>
> Looks like I misunderstand something in the algorithm, but aren't default
> partitions were already processed before e.g. here:
>
>     + /*
>     +  * Default partition on the outer side forms the default
>     +  * partition of the join result.
>     +  */

The default partition handling in the loop handles the cases of
missing partitions as explained in a comment
        /*
         * If a range appears in one of the joining relations but not the other
         * (as a whole or part), the rows in the corresponding partition will
         * not have join partners in the other relation, unless the other
         * relation has a default partition.

But merge_default_partitions() tries to map the default partitions
from both the relations.

>
> Also in here
>
>     + /* Free any memory we used in this function. */
>     + list_free(merged_datums);
>     + list_free(merged_indexes);
>     + pfree(outer_pmap);
>     + pfree(inner_pmap);
>     + pfree(outer_mmap);
>     + pfree(inner_mmap);
>
> I think `merged_kinds` is missing.

Done.

>
> I've noticed that in some places `IS_PARTITIONED_REL` was replaced
>
>     - if (!IS_PARTITIONED_REL(joinrel))
>     + if (joinrel->part_scheme == NULL)
>
> but I'm not quite follow why? Is it because `boundinfo` is not available
> anymore at this point? If so, maybe it makes sense to update the commentary for
> this macro and mention to not use for joinrel.

This is done in try_partitionwise_join(). As explained in the comment
    /*
     * Get the list of matching partitions to be joined along with the
     * partition bounds of the join relation. Because of the restrictions
     * imposed by partition matching algorithm, not every pair of joining
     * relations for this join will be able to use partition-wise join. But all
     * those pairs which can use partition-wise join will produce the same
     * partition bounds for the join relation.
     */
boundinfo for the join relation is built in this function. So, we
don't have join relation's partitioning information fully set up yet.
So we can't use IS_PARTITIONED_REL() there. joinrel->part_scheme if
set tells that the joining relations have matching partition schemes
and thus the join relation can possibly use partition-wise join
technique. If it's not set, then we can't use partition-wise join.

But IS_PARTITIONED_REL() is still useful at a number of other places,
where it's known to encounter a RelOptInfo whose partitioning
properties are fully setup. So, I don't think we should change the
macro or the comments above it.

>
> Also, as for me it would be helpful to have some commentary about this new
> `partsupfunc`, what is exactly the purpose of it (but it's maybe just me
> missing some obvious things from partitioning context)
>
>     + FmgrInfo   *partsupfunc;

It's just copied from Relation::PartitionKey as is. It stores the
functions required to compare two partition key datums. Since we use
those functions frequently their FmgrInfo is cached.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company