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