Re: [HACKERS] Partitioned tables and relfilenode - Mailing list pgsql-hackers

From Ashutosh Bapat
Subject Re: [HACKERS] Partitioned tables and relfilenode
Date
Msg-id CAFjFpRd0Pa2y_KJ1CPQ4V4vw8_dXo4h53aL7P1CkGj5Toa0M1A@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] Partitioned tables and relfilenode  (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>)
Responses Re: [HACKERS] Partitioned tables and relfilenode
List pgsql-hackers
On Thu, Feb 23, 2017 at 11:19 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks for the review.
>
> On 2017/02/22 21:58, Ashutosh Bapat wrote:
>>>> Also we should add tests to make sure the scans on partitioned tables
>>>> without any partitions do not get into problems. PFA patch which adds
>>>> those tests.
>>>
>>> I added the test case you suggest, but kept just the first one.
>>
>> The second one was testing multi-level partitioning case, which
>> deserves a testcase by its own.
>
> Ah, okay.  Added it back.

Thanks.

>
>> Some more comments
>>
>> The comment below seems too verbose. All it can say is "A partitioned table
>> without any partitions results in a dummy relation." I don't think we need to
>> be explicit about rte->inh. But it's more of personal preference. We will leave
>> it to the committer, if you don't agree.
>> +                   /*
>> +                    * An empty partitioned table, i.e., one without any leaf
>> +                    * partitions, as signaled by rte->inh being set to false
>> +                    * by the prep phase (see expand_inherited_rtentry).
>> +                    */
>
> It does seem poorly worded.  How about:
>
>                 /*
>                  * A partitioned table without leaf partitions is marked
>                  * as a dummy rel.
>                  */
>
>> We don't need this comment as well. Rather than repeating what's been said at
>> the top of the function, it should just refer to it like "nominal relation has
>> been set above for partitioned tables. For other parent relations, we'll use
>> the first child ...".
>> +        *
>> +        * If the parent is a partitioned table, we do not have a separate RTE
>> +        * representing it as a member of the inheritance set, because we do
>> +        * not create a scan plan for it.  As mentioned at the top of this
>> +        * function, we make the parent RTE itself the nominal relation.
>>          */
>
> 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.

>
>> 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.

Let me know what you think.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

pgsql-hackers by date:

Previous
From: Fabien COELHO
Date:
Subject: Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless)
Next
From: Jim Nasby
Date:
Subject: Re: [HACKERS] case_preservation_and_insensitivity = on