(2019/01/16 14:45), Etsuro Fujita wrote:
> (2019/01/15 11:42), Amit Langote wrote:
>> On 2019/01/11 21:50, Etsuro Fujita wrote:
>>>>> (2019/01/10 10:41), Amit Langote wrote:
>>>>>> That's a loaded meaning and abusing it to mean something else can be
>>>>>> challenged, but we can live with that if properly documented.
>>>>>> Speaking of
>>>>>> which:
>>>>>>
>>>>>> /* used by partitionwise joins: */
>>>>>> bool consider_partitionwise_join; /* consider
>>>>>> partitionwise join
>>>>>> * paths? (if
>>>>>> partitioned
>>>>>> rel) */
>>>>>>
>>>>>> Maybe, mention here how it will be abused in back-branches for
>>>>>> non-partitioned relations?
>
>>> I know we don't yet reach a consensus on what to do in details to
>>> address
>>> this issue, but for the above, how about adding comments like this to
>>> set_append_rel_size(), instead of the header file:
>>>
>>> /*
>>> * If we consider partitionwise joins with the parent rel, do the
>>> same
>>> * for partitioned child rels.
>>> *
>>> * Note: here we abuse the consider_partitionwise_join flag for child
>>> * rels that are not partitioned, to tell try_partitionwise_join()
>>> * that their targetlists and EC entries have been generated.
>>> */
>>> if (rel->consider_partitionwise_join)
>>> childrel->consider_partitionwise_join = true;
>>>
>>> ISTM that that would be more clearer than the header file.
>>
>> Thanks for updating the patch. I tend to agree that it might be better to
>> add such details here than in the header as it's better to keep the
>> latter
>> more stable.
>>
>> About the comment you added, I think we could clarify the note further
>> as:
>>
>> Note: here we abuse the consider_partitionwise_join flag by setting it
>> *even* for child rels that are not partitioned. In that case, we set it
>> to tell try_partitionwise_join() that it doesn't need to generate their
>> targetlists and EC entries as they have already been generated here, as
>> opposed to the dummy child rels for which the flag is left set to
>> false so
>> that it will generate them.
>>
>> Maybe it's a bit wordy, but it helps get the intention across more
>> clearly.
>
> I think that is well-worded, so +1 from me.
I updated the patch as such and rebased it to the latest HEAD. I also
added the commit message. Attached is an updated patch. Does that make
sense? If there are no objections, I'll push that patch early next week.
Best regards,
Etsuro Fujita
