Thanks for the review. On 2017/02/23 15:44, Ashutosh Bapat wrote: > On Thu, Feb 23, 2017 at 11:19 AM, Amit Langote wrote: >> 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.
What was there sounds grammatically correct to me, but fine. >>> 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. @@ -996,10 +996,20 @@ inheritance_planner(PlannerInfo *root) /* + * Partitioned tables do not have storage for themselves and should not be + * scanned. @@ -1450,6 +1451,21 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti) /* + * Partitioned tables themselves do not have any storage and should not + * be scanned. So, do not create child relations for those. + */ I guess we should not have to repeat "partitioned tables do not have storage" in all these places. + * a partitioned relation as dummy. The duplicate RTE we added for the + * parent table is harmless, so we don't bother to get rid of it; ditto for + * the useless PlanRowMark node. There is no duplicate RTE in the partitioned table case, which even my original comment failed to consider. Can you, maybe? Thanks, Amit -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers