(2019/01/11 13:46), Amit Langote wrote:
On 2019/01/10 15:07, Etsuro Fujita wrote:
(The name of the flag isn't
good? If so, that would be my fault because I named that flag.)
If it's really just to store the fact that the relation's targetlist
contains expressions that partitionwise join currently cannot handle, then
setting it like this in set_append_rel_size seems a bit misleading:
if (enable_partitionwise_join&&
rel->reloptkind == RELOPT_BASEREL&&
rte->relkind == RELKIND_PARTITIONED_TABLE&&
rel->attr_needed[InvalidAttrNumber - rel->min_attr] == NULL)
rel->consider_partitionwise_join = true;
Sorry, I wasn't around to comment on the patch which got committed in
7cfdc77023a, but checking the value of enable_partitionwise_join and other
things in set_append_rel_size() to set the value of
consider_partitionwise_join seems a bit odd to me. Perhaps,
consider_partitionwise_join should be initially set to true for a relation
(actually, to rel->part_scheme != NULL) and only set it to false if the
relation's targetlist is found to contain unsupported expressions.
One thing I intended in that commit was to set the flag to false for
partitioned tables contained in inheritance trees where the top parent
is a UNION ALL subquery, because we don't consider PWJ for those tables.
Actually we wouldn't need to care about that, because we don't do PWJ
for those tables regardless of what the flag is set, but I think that
would make the code a bit cleaner. 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).
That
way, it becomes easier to think what it means imho.
May be or may not be.
I think
enable_partitionwise_join should only be checked in relnode.c or
joinrels.c.
Sorry, I don't understand this.
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).
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.
Please find attached an updated version of the patch. I modified your
version so that building tlists for child dummy rels are also postponed
until after they actually participate in partitionwise-joins, to avoid
that possibly-useless work as well. I haven't done any performance tests
yet though.
Thanks for updating the patch. I tested your patch (test setup described
below) and it has almost the same performance as my previous version:
552ms (vs. 41159ms on HEAD vs. 253ms on PG 10) for the query also
mentioned below.
I also tested the patch with your script:
253.559 ms (vs. 85776.515 ms on HEAD vs. 206.066 ms on PG 10)
Oh, PG 11 doesn't appear as bad compared to PG 10 with your numbers as it
did on my machine. That's good anyway.
Yeah, that's a good result!
Best regards,
Etsuro Fujita
