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.
> Do you see that as something for PG11?
We already broke the API of these functions for v11. I don't much
want to break them again in v12. Let's get it right the first time.
regards, tom lane