Robert Haas <robertmhaas@gmail.com> writes:
> OK. Maybe I shouldn't be worrying about the table_open() /
> table_close() here, because I see that you are right that
> has_subclass() is nearby, which admittedly does not involve opening
> the relation, but it does involve fetching from the syscache a tuple
> that we wouldn't need to fetch if we had a Relation available at that
> point. And I see now that expand_virtual_generated_columns() is also
> in that area and works similar to your proposed function in that it
> just opens and closes all the relations. Perhaps that's just the way
> we do things at this very early stage of the planner? But I feel like
> it might make sense to do some reorganization of this code, though, so
> that it more resembles the phase 1 and phase 2 as you describe them.
Indeed, I think those are hacks that we should get rid of, not
emulate. Note in particular that expand_virtual_generated_columns
is new in v18 and has exactly zero credibility as precedent. In fact,
I'm probably going to harass Peter about fixing it before v18 ships.
Randomly adding table_opens in the planner is not a route to high
planning performance.
> A related point that I'm noticing is that you record the not-NULL
> information in the RangeTblEntry.
Did we not just complain about that w.r.t. the v1 version of this
patch? RangeTblEntry is not where to store this info. We need
a new data structure, and IMO the data structure should be a hashtable
based on table OID, not relid. That way we can amortize across
multiple references to the same table within a query.
regards, tom lane
