Re: Partitioned tables and relfilenode

On 2017/02/23 16:48, Ashutosh Bapat wrote:
> On Thu, Feb 23, 2017 at 1:08 PM, Amit Langote wrote:
>> @@ -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.
>
> Hmm, you are right. But they are two different functions and the code
> blocks are located away from each other. So, I thought it would be
> better to have complete comment there, instead of pointing to other
> places. I am fine, if we can reword it without compromising
> readability.

I was saying in general. I agree that different sites in the optimizer
may have different considerations for why partitioned tables are to be
handled specially, common theme being that we do not have to scan the
parent relation. If the implementation changes someday such that we don't
manipulate child tables (especially, partitions) through most planning
phases anymore, then maybe we will start using some different terminology
where we don't have to stress this fact too much. We're not there yet though.

>> + * 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?
>
> May be we could says "If we have added duplicate RTE for the parent
> table, it is harmless ...". Does that sound good?

Duplicate RTE added in the non-partitioned table case is harmless, so we
don't bother to get rid of it; ditto for the useless PlanRowMark node.

Thanks,
Amit