On Tue, Mar 7, 2017 at 12:11 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> I see that all the changes by Amit and myself to what was earlier 0003
> patch are now part of 0002 patch. The patch looks ready for committer.
Reviewing 0002:
This patch seems to have falsified the header comments for
expand_inherited_rtentry.
I don't quite understand the need for the change to set_rel_size().
The rte->inh case is handled before reaching the new code, and IIUC
the !rte->inh case should never happen given the changes to
expand_inherited_rtentry. Or am I confused? If not, I think we
should just add an Assert() to the "plain relation" case that this is
not RELKIND_PARTITIONED_TABLE (with a comment explaining why it can't
happen).
In inheritance_planner, this comment addition does not seem adequate:
+ * If the parent is a partitioned table, we already set the nominal
+ * relation.
That basically contradicts the previous paragraph. I think you need
to adjust the previous paragraph instead of adding a new one, probably
in multiple places. For example, the parenthesized bit now only
applies in the non-partitioned case.
- rel = mtstate->resultRelInfo->ri_RelationDesc;
+ nominalRTE = rt_fetch(node->nominalRelation, estate->es_range_table);
+ nominalRel = heap_open(nominalRTE->relid, NoLock);
No lock?
Another thing that bothers me about this is that, apparently, the use
of nominalRelation is no longer strictly for EXPLAIN, as the comments
in plannodes.h/relation.h still claim that it is. I'm not sure how to
adjust that exactly; there's not a lot of room in those comments to
give all the details. Maybe they should simply say something like /*
Parent RT index */ instead of /* Parent RT index for use of EXPLAIN
*/. But we can't just go around changing the meaning of things
without updating the comments accordingly. A related question is
whether we should really be using nominalRelation for this or whether
there's some other way we should be getting at the parent -- I don't
have another idea, though.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
