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

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

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:

Reply via email to