From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
Cc: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Partitioned tables and relfilenode |
Date: | 2017-03-07 17:27:27 |
Message-ID: | CA+TgmobMuL-wfhO5kJtk-du3m4nu4YrQ6HFFGHwa3hek2SH6kw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Mar 7, 2017 at 12:11 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2017-03-07 17:55:19 | Bizarre choice of case for RELKIND_PARTITIONED_TABLE |
Previous Message | Andres Freund | 2017-03-07 17:04:12 | Re: Patch to improve performance of replay of AccessExclusiveLock |