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

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, alvherre(at)2ndquadrant(dot)com, pryzby(at)telsasoft(dot)com, sanyo(dot)moura(at)tatic(dot)net, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, david(dot)rowley(at)2ndquadrant(dot)com, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Query with high planning time at version 11.1 compared versions 10.5 and 11.0
Date: 2019-01-18 12:55:28
Message-ID: 5C41CCC0.5030702@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers pgsql-performance

(2019/01/16 14:45), Etsuro Fujita wrote:
> (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.

I updated the patch as such and rebased it to the latest HEAD. I also
added the commit message. Attached is an updated patch. Does that make
sense? If there are no objections, I'll push that patch early next week.

Best regards,
Etsuro Fujita

Attachment Content-Type Size
0001-Postpone-generating-tlists-and-EC-members-for-inheri.patch text/x-patch 9.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Adrien NAYRAT 2019-01-18 13:22:50 Re: Log a sample of transactions
Previous Message Masahiko Sawada 2019-01-18 12:41:42 Re: [HACKERS] Block level parallel vacuum

Browse pgsql-performance by date

  From Date Subject
Next Message Luis Carril 2019-01-18 14:12:23 JIT overhead slowdown
Previous Message Mariel Cherkassky 2019-01-17 20:18:29 Re: autovacuum doesnt run on the pg_toast_id table