Thread: Re: Prologue of set_append_rel_size() and partitionedtables

Re: Prologue of set_append_rel_size() and partitionedtables

From
Amit Langote
Date:
On 2017/03/29 15:20, Ashutosh Bapat wrote:
> The prologue of set_append_rel_size() mentions
> 
>  * ....  Note that in the inheritance case,
>  * the first member relation is actually the same table as is mentioned in
>  * the parent RTE ... but it has a different RTE and RelOptInfo.
> 
> This isn't true about partitioned tables anymore. We do not create
> RelOptInfo of the partitioned table and thus is not first member
> relation.

My bad.

> We could argue that inheritance in case of partitioned
> tables is just an implementation detail and partitioned table is not
> "inherited" in true sense. So "inheritance case" referred to here does
> not cover partitioning and so the sentence still holds. But I guess,
> this needs some change so that we do not expect first member to be
> same as partitioned table. I am not able to craft an elegant sentence
> but how about something like attached?

I think we *should* update the comment somwhow.  Since now there are a few
places using "non-partitioned inheritance" to refer to regular parent
tables, why not use that term here too?  So:

  * The passed-in rel and RTE represent the entire append relation.  The
- * relation's contents are computed by appending together the output of
- * the individual member relations.  Note that in the inheritance case,
- * the first member relation is actually the same table as is mentioned in
- * the parent RTE ... but it has a different RTE and RelOptInfo.  This is
+ * relation's contents are computed by appending together the output of the
+ * individual member relations.  Note that in the non-partitioned inheritance
+ * case, the first member relation is actually the same table as is mentioned
+ * in the parent RTE ... but it has a different RTE and RelOptInfo.  This is

Update patch attached.

Thanks,
Amit

Attachment

Re: Prologue of set_append_rel_size() and partitioned tables

From
Ashutosh Bapat
Date:
On Wed, Mar 29, 2017 at 12:23 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/03/29 15:20, Ashutosh Bapat wrote:
>> The prologue of set_append_rel_size() mentions
>>
>>  * ....  Note that in the inheritance case,
>>  * the first member relation is actually the same table as is mentioned in
>>  * the parent RTE ... but it has a different RTE and RelOptInfo.
>>
>> This isn't true about partitioned tables anymore. We do not create
>> RelOptInfo of the partitioned table and thus is not first member
>> relation.
>
> My bad.
>
>> We could argue that inheritance in case of partitioned
>> tables is just an implementation detail and partitioned table is not
>> "inherited" in true sense. So "inheritance case" referred to here does
>> not cover partitioning and so the sentence still holds. But I guess,
>> this needs some change so that we do not expect first member to be
>> same as partitioned table. I am not able to craft an elegant sentence
>> but how about something like attached?
>
> I think we *should* update the comment somwhow.  Since now there are a few
> places using "non-partitioned inheritance" to refer to regular parent
> tables, why not use that term here too? So:
>
>   * The passed-in rel and RTE represent the entire append relation.  The
> - * relation's contents are computed by appending together the output of
> - * the individual member relations.  Note that in the inheritance case,
> - * the first member relation is actually the same table as is mentioned in
> - * the parent RTE ... but it has a different RTE and RelOptInfo.  This is
> + * relation's contents are computed by appending together the output of the
> + * individual member relations.  Note that in the non-partitioned inheritance
> + * case, the first member relation is actually the same table as is mentioned
> + * in the parent RTE ... but it has a different RTE and RelOptInfo.  This is
>
> Update patch attached.

Looks good to me.

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