Fujita-san, 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? >>> >>> Will do. >> >> Thank you. > > 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. Thanks, Amit
