On Fri, Jun 8, 2018 at 10:26 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: > David Rowley <david.row...@2ndquadrant.com> writes: >> On 8 June 2018 at 08:22, Tom Lane <t...@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...@mail.gmail.com [2] https://postgrespro.com/list/id/cafjfprcmwwepj-do1otxq-gapgpsz1fmh7yqvqtwzqogczq...@mail.gmail.com -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company