On 8 June 2018 at 08:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So that's basically what David's patch does, and it seems fine as far
> as it goes, although I disapprove of shoving the responsibility into
> setup_simple_rel_arrays() without so much as a comment change.
> I'd make a separate function for that, I think. (BTW, perhaps instead
> we should do what the comment quoted above contemplates, and set up a
> link in the child's RelOptInfo?)
I had originally wanted to stuff these into RelOptInfo, but I
discovered it was not really possible because of how the inheritance
planner works. It replaces the parent with the child RelOptInfo for
each child and replans the query over and over, meaning we'd never
have a complete list of AppendRelInfos to work with.
> I'm still of the opinion that find_appinfos_by_relids() needs to be
> nuked from orbit. It has nothing to recommend it either from the
> standpoint of performance or that of intellectual coherency (or maybe
> that problem is just inadequate documentation). The places consuming
> its results are no better.
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.
Do you see that as something for PG11? Keep in mind, I did code the
patch in a way to minimise invasiveness having considered that PG11 is
now in beta. I'm willing to write a patch that gets rid of
find_appinfos_by_relids completely. There are a few places, e.g.
create_partitionwise_grouping_paths that make use of the appinfos
multiple times, but the direct lookup is probably fast enough that
this would not matter.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services