On Tue, Mar 7, 2017 at 12:11 AM, Ashutosh Bapat <ashutosh.ba...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers