On Wed, Jun 6, 2018 at 11:27 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
>
> I was trying to be realistic for something we can do to fix v11. It's
> probably better to minimise the risky surgery on this code while in
> beta. What I proposed was intended to fix a performance regression new
> in v11. I'm not sure what you're proposing has the same intentions.
Agreed that we want a less risky fix at this stage. What I am worried
is with your implementation there are two ways to get AppendRelInfo
corresponding to a child, append_rel_list and append_rel_array. Right
now we will change all the code to use append_rel_array but there is
no guarantee that some code in future will use append_rel_list. Bugs
would rise if these two get out of sync esp. considering
append_rel_list is a list which can be easily modified. I think we
should avoid that. See what we do to rt_fetch() and
planner_rt_fetch(), but we still have two ways to get an RTE. That's a
source of future bugs.
>
> Probably, if you do want an efficient way to store the children
> belonging to a parent we could just have another array of Bitmapsets
> which would just serve as a set of indexes into the array I've added.
>
I was actually wrong when I proposed that we store AppendRelInfos
indexed by parent_id in the same array. You are right; there can be
multiple children with same parent id. I like your solution of having
an array of bitmapsets, members within which are indexes into
append_rel_array.
> I'd prefer to get a committers thoughts on this before doing any further work.
Yes. I think so too.
>
> I've attached a cleaned up patch from the last one. This just adds
> some sanity checks to make sure we get an ERROR if we do ever see two
> AppendRelInfos with the same child relation id. I've also made it so
> the append_rel_array is only allocated when there are append rels.
>
In find_appinfos_by_relids(), we should Assert that the child_id in
the AppendRelInfo obtained from the array is same as its index. My
guess is that we could actually get rid of child_relid member of
AppendRelInfo altogether if we directly store AppendRelInfos in the
array without creating a list first. But that may be V12 material.
Also in the same function we want to Assert that cnt never exceeds *nappinfo.
+ Assert(root->append_rel_array);
root->append_rel_array != NULL seems to be a preferred style in the code.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company