On Thu, Feb 23, 2017 at 1:08 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks for the review.
>
> On 2017/02/23 15:44, Ashutosh Bapat wrote:
>> On Thu, Feb 23, 2017 at 11:19 AM, Amit Langote wrote:
>>> 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.
>
> What was there sounds grammatically correct to me, but fine.
>
>>>> 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.
>
> @@ -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.
>
> + * 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?
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
