(2018/08/13 11:57), Robert Haas wrote:
> On Mon, Aug 6, 2018 at 8:30 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> In the above I used the test whether the relation's reloptkind is
>> RELOPT_BASEREL or not, but I noticed that I had overlooked the case of a
>> multi-level partitioned table. So I fixed that and added regression test
>> cases for that. I also revised comments a bit. Attached is an updated
>> version of the patch.
>
> + /* If so, consider partitionwise joins for that join. */
> + if (IS_PARTITIONED_REL(joinrel))
> + joinrel->consider_partitionwise_join = true;
>
> Maybe this should assert that the inner and outer rels have
> consider_partitionwise_join set. There is an Assert quite a bit
> earlier in the function that the parent join have it set, but I think
> it might make sense to check the children have it set whenever we set
> the flag.
Agreed. Done.
One thing I noticed might be an improvement is to skip
build_joinrel_partition_info if the given joinrel will be to have
consider_partitionwise_join=false; in the previous patch, that function
created the joinrel's partition info such as part_scheme and part_rels
if the joinrel is considered as partitioned, independently of the flag
consider_partitionwise_join for it, but if that flag is false, we don't
generate PWJ paths for the joinrel, so we would not need to create that
partition info at all. This would not only avoid unnecessary processing
in that function, but also make unnecessary the changes I made to
try_partitionwise_join, generate_partitionwise_join_paths,
apply_scanjoin_target_to_paths, and create_ordinary_grouping_paths. So
I updated the patch that way. Please find attached an updated version
of the patch.
Thanks for the review!
Best regards,
Etsuro Fujita
