Re: Query with high planning time at version 11.1 compared versions 10.5 and 11.0

(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