Message-ID: <5C3EC41B.6040404@lab.ntt.co.jp>
Date: Wed, 16 Jan 2019 14:41:47 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: Query with high planning time at version 11.1 compared versions
 10.5 and 11.0
References: <8db4eca8-6e76-70ff-f934-e0228a1cc298@lab.ntt.co.jp>
 <20181206075726.pig32e6thltb56hr@alvherre.pgsql>
 <CAExHW5umxU5FOoT8FBcqp-GWKMtZhnuEy7U6bNH8QfzvYwUgEQ@mail.gmail.com>
 <CAExHW5uFwP_1N7RV6s8piQBvygGH+rppS60nFNHB9DKEM04ShA@mail.gmail.com>
 <5C34846E.9090600@lab.ntt.co.jp>
 <96720c99-ffa0-01ad-c594-0504c8eda708@lab.ntt.co.jp>
 <5C35D912.8040200@lab.ntt.co.jp>
 <7354b5bf-5932-4c0c-81ee-d9550de18995@lab.ntt.co.jp>
 <5C36E105.2020902@lab.ntt.co.jp>
 <dd1de042-dca5-cfb9-bc66-2e39020fbf6a@lab.ntt.co.jp>
 <5C387848.5070809@lab.ntt.co.jp>
 <a56432be-9b88-1525-410f-be0f9abc1703@lab.ntt.co.jp>
In-Reply-To: <a56432be-9b88-1525-410f-be0f9abc1703@lab.ntt.co.jp>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, alvherre@2ndquadrant.com,
        pryzby@telsasoft.com, sanyo.moura@tatic.net,
        Jeff Janes <jeff.janes@gmail.com>, david.rowley@2ndquadrant.com,
        Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org
Precedence: bulk

Amit-san,

(2019/01/15 10:46), Amit Langote wrote:
> On 2019/01/11 20:04, Etsuro Fujita wrote:
>> (2019/01/11 13:46), Amit Langote wrote:

>> 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).
>
> Partition pruning uses part_scheme and pruning can occur even if a
> partitioned table is under UNION ALL, so it *is* needed in that case.

Ah, you are right.  Thanks for pointing that out!

>>> I think
>>> enable_partitionwise_join should only be checked in relnode.c or
>>> joinrels.c.
>>
>> Sorry, I don't understand this.
>
> What I was trying to say is that we should check the GUC close to where
> partitionwise join is actually implemented even though there is no such
> hard and fast rule.  Or maybe I'm just a bit uncomfortable with setting
> consider_partitionwise_join based on the GUC.

I didn't think so.  Consider the consider_parallel flag.  I think the 
way of setting it deviates from that rule already; it is set essentially 
based on a GUC and is set in set_base_rel_sizes() (ie, before 
implementing parallel paths).  When adding the 
consider_partitionwise_join flag, I thought it would be a good idea to 
set consider_partitionwise_join in a similar way to consider_parallel, 
keeping build_simple_rel() simple.

>>> 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).
>
> As I said above, partition pruning can occur even if a partitioned table
> happens to be under UNION ALL.  However, we *can* avoid creating
> part_scheme and setting other partitioning properties if *all* of
> enable_partition_pruning, enable_partitionwise_join, and
> enable_partitionwise_aggregate are turned off.

Yeah, I think so.

>>> 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.
>
> Sure, no problem with committing it separately if at all.

OK

Thanks!

Best regards,
Etsuro Fujita