On Thu, Feb 23, 2017 at 11:19 AM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > 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.
Thanks. > >> 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. > */ > I reworded those comments a bit and corrected grammar. Please check in the attached patch. > >> 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. On a second thought, I added a boolean to check if there were any children created and then reset rte->inh based on that value. That's better than relying on appinfos length now. Let me know what you think. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
0003-Avoid-creating-scan-nodes-for-partitioned-tables_v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
