From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
Cc: | Robert Haas <robertmhaas(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-02-23 05:49:31 |
Message-ID: | a34313a6-6a50-4690-79f6-0691bb87db83@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for the review.
On 2017/02/22 21:58, Ashutosh Bapat wrote:
>>> Also we should add tests to make sure the scans on partitioned tables
>>> without any partitions do not get into problems. PFA patch which adds
>>> those tests.
>>
>> I added the test case you suggest, but kept just the first one.
>
> The second one was testing multi-level partitioning case, which
> deserves a testcase by its own.
Ah, okay. Added it back.
> Some more comments
>
> The comment below seems too verbose. All it can say is "A partitioned table
> without any partitions results in a dummy relation." I don't think we need to
> be explicit about rte->inh. But it's more of personal preference. We will leave
> it to the committer, if you don't agree.
> + /*
> + * An empty partitioned table, i.e., one without any leaf
> + * partitions, as signaled by rte->inh being set to false
> + * by the prep phase (see expand_inherited_rtentry).
> + */
It does seem poorly worded. How about:
/*
* A partitioned table without leaf partitions is marked
* as a dummy rel.
*/
> We don't need this comment as well. Rather than repeating what's been said at
> the top of the function, it should just refer to it like "nominal relation has
> been set above for partitioned tables. For other parent relations, we'll use
> the first child ...".
> + *
> + * If the parent is a partitioned table, we do not have a separate RTE
> + * representing it as a member of the inheritance set, because we do
> + * not create a scan plan for it. As mentioned at the top of this
> + * function, we make the parent RTE itself the nominal relation.
> */
Rewrote that comment block as:
*
* If the parent is a partitioned table, we already set the nominal
* relation.
*/
> Following condition is not very readable. It's not evident that it's of the
> form (A && B) || C, at a glance it looks like it's A && (B || C).
> + if ((rte->relkind != RELKIND_PARTITIONED_TABLE &&
> + list_length(appinfos) < 2) || list_length(appinfos) < 1)
>
> Instead you may rearrage it as
> min_child_rels = (rte->relkind == RELKIND_PARTITIONED_TABLE ? 1 : 2);
> if (list_length(appinfos) < min_child_rels)
OK, done that way.
Thanks,
Amit
Attachment | Content-Type | Size |
---|---|---|
0001-Partitioned-tables-are-empty-themselves.patch | text/x-diff | 12.9 KB |
0002-Do-not-allocate-storage-for-partitioned-tables.patch | text/x-diff | 3.1 KB |
0003-Avoid-creating-scan-nodes-for-partitioned-tables.patch | text/x-diff | 16.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2017-02-23 06:22:38 | Re: Measuring replay lag |
Previous Message | Corey Huinker | 2017-02-23 05:11:22 | Re: \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless) |