Re: 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