On Thu, Mar 22, 2018 at 4:32 AM, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>> On 12 March 2018 at 06:00, Ashutosh Bapat <ashutosh.ba...@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
