Re: [HACKERS] Partitioned tables and relfilenode

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