Re: [HACKERS] Partitioned tables and relfilenode

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.

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


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

Thanks,
Amit

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