Re: Performance regression with PostgreSQL 11 and partitioning

On Fri, Jun 8, 2018 at 10:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> On 8 June 2018 at 08:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm still of the opinion that find_appinfos_by_relids() needs to be
>>> nuked from orbit.
>
>> Yeah, I agree it's not nice that it pallocs an array then pfrees it
>> again. adjust_appendrel_attrs and adjust_child_relids could probably
>> just accept a RelIds of childrels and find the AppendRelInfos itself,
>> similar to how I coded find_appinfos_by_relids.
>
> Why do they need to accept any additional parameters at all?
>
> The pre-v11 incarnation of those functions took a single AppendRelInfo,
> specifying an exact translation from one parent relid to one child
> relid.  The fundamental problem I've got with the current code, entirely
> independently of any performance issues, is that it's completely unclear
> -- or at least undocumented -- which translation(s) are supposed to occur.
> If we assume that the code isn't 100% broken (which I'm hardly convinced
> of, but we'll take it as read for the moment) then it must be that the
> translations are constrained to be well-determined by the query structure
> as represented by the totality of the AppendRelInfo data.  So I'm thinking
> maybe we don't need those parameters at all.  In the pre-v11 situation,
> we were saving lookups by passing the only applicable AppendRelInfo to
> the translation functions.  But if the translation functions have to
> look up the right translation anyway, let's just make them do that,
> and create whatever infrastructure we have to have to make it fast.
>

Pre-v11 we required to translate an expressions only for one
parent-child pair using adjust_appendrel_attrs() since only base
relations could have "other" relations. With partition-wise join and
aggregates, we have to do that for multiple parent-child pairs. If we
were to use the same pre-v11 interfaces, we have to do those
adjustments one pair at a time. But the way adjust_appendrel_attrs()
works, it creates a copy of whole expression tree replacing only the
nodes that adjust_appendrel_attrs() cares about. Doing adjustments one
pair at at time meant that all translated expression trees leaked
memory except the last one, which will be used. It was natural to pass
multiple AppendRelInfos, one per parent-child pair, to
adjust_appendrel_attrs() so that it carries out the translations in
one go. This was discussed at length over an year ago. I can see some
references starting [1] and following mails. The original mail thread
started at [2]. You will also find discussion about why we chose to
pass an array instead of list. We didn't do inside
adjust_appendrel_attrs() or adjust_child_relids() because in some
functions those were called multiple times and scanning list those
many times for same set of AppendRelInfos would have been waste of CPU
cycles.

[1] https://postgrespro.com/list/id/CAFjFpRcMWwepj-Do1otxQ-GApGPSZ1FmH7YQvQTwzQOGczq_sw@mail.gmail.com
[2] https://postgrespro.com/list/id/CAFjFpRcMWwepj-Do1otxQ-GApGPSZ1FmH7YQvQTwzQOGczq_sw@mail.gmail.com

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company