(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.
Thanks again!
Best regards,
Etsuro Fujita
