Amit-san,
(2019/01/15 10:46), Amit Langote wrote:
> On 2019/01/11 20:04, Etsuro Fujita wrote:
>> (2019/01/11 13:46), Amit Langote wrote:
>> However, what you proposed here as-is would
>> not keep that behavior, because rel->part_scheme is created for those
>> tables as well
>> (even though there would be no need IIUC).
>
> Partition pruning uses part_scheme and pruning can occur even if a
> partitioned table is under UNION ALL, so it *is* needed in that case.
Ah, you are right. Thanks for pointing that out!
>>> I think
>>> enable_partitionwise_join should only be checked in relnode.c or
>>> joinrels.c.
>>
>> Sorry, I don't understand this.
>
> What I was trying to say is that we should check the GUC close to where
> partitionwise join is actually implemented even though there is no such
> hard and fast rule. Or maybe I'm just a bit uncomfortable with setting
> consider_partitionwise_join based on the GUC.
I didn't think so. Consider the consider_parallel flag. I think the
way of setting it deviates from that rule already; it is set essentially
based on a GUC and is set in set_base_rel_sizes() (ie, before
implementing parallel paths). When adding the
consider_partitionwise_join flag, I thought it would be a good idea to
set consider_partitionwise_join in a similar way to consider_parallel,
keeping build_simple_rel() simple.
>>> I've attached a patch to show what I mean. Can you please
>>> take a look?
>>
>> Thanks for the patch! Maybe I'm missing something, but I don't have a
>> strong opinion about that change. I'd rather think to modify
>> build_simple_rel so that it doesn't create rel->part_scheme if unnecessary
>> (ie, partitioned tables contained in inheritance trees where the top
>> parent is a UNION ALL subquery).
>
> As I said above, partition pruning can occur even if a partitioned table
> happens to be under UNION ALL. However, we *can* avoid creating
> part_scheme and setting other partitioning properties if *all* of
> enable_partition_pruning, enable_partitionwise_join, and
> enable_partitionwise_aggregate are turned off.
Yeah, I think so.
>>> If you think that this patch is a good idea, then you'll need to
>>> explicitly set consider_partitionwise_join to false for a dummy partition
>>> rel in set_append_rel_size(), because the assumption of your patch that
>>> such partition's rel's consider_partitionwise_join would be false (as
>>> initialized with the current code) would be broken by my patch. But that
>>> might be a good thing to do anyway as it will document the special case
>>> usage of consider_partitionwise_join variable more explicitly, assuming
>>> you'll be adding a comment describing why it's being set to false
>>> explicitly.
>>
>> I'm not sure we need this as part of a fix for the issue reported on this
>> thread. I don't object to what you proposed here, but that would be
>> rather an improvement, so I think we should leave that for another patch.
>
> Sure, no problem with committing it separately if at all.
OK
Thanks!
Best regards,
Etsuro Fujita
