Re: Partitioned tables and relfilenode

On Mon, Mar 6, 2017 at 12:48 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Thanks for the review.
>
> On 2017/03/06 15:41, Michael Paquier wrote:
>> On Fri, Mar 3, 2017 at 10:02 AM, Amit Langote
>> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> Thanks. I noticed that 'and' is duplicated in a line added by the commit
>>> to analyze.sgml. Attached 0001 fixes that. 0002 and 0003 same as the
>>> last version.
>>
>> /*
>> - * If all the children were temp tables, pretend it's a non-inheritance
>> - * situation. 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.
>> + * If all the children were temp tables or if the parent is a partitioned
>> + * table without any leaf partitions, pretend it's a non-inheritance
>> + * situation. The duplicate RTE for the parent table we 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.
>> */
>> - if (list_length(appinfos) < 2)
>> + if (!has_child)
>> This comment is not completely correct. Children can be temp tables,
>> they just cannot be temp tables of other backends. It seems to me that
>> you could still keep this code simple and remove has_child..
>
> I updated the comment. I recall having posted a patch for that once, but
> perhaps went unnoticed.

The existing comment only specifies "temp tables" and not "temp table
of other backends". The new comment keeps that part same and adds
partitioned table case. So, I don't see any reason to change the "temp
tables" to "temp table of other backends" in this patch.

>
> About has_child, the other option is to make the minimum length of
> appinfos list relkind-based, but the condition quickly becomes ugly. Do
> you have a suggestion?
>
+1.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company