On Wed, Sep 13, 2017 at 9:39 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Sep 12, 2017 at 5:47 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>> On 5 September 2017 at 14:04, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> I started with a quick review ... a couple of comments below :
>>
>> - * If this is a baserel, consider gathering any partial paths we may have
>> - * created for it. (If we tried to gather inheritance children, we could
>> + * If this is a baserel and not the only rel, consider gathering any
>> + * partial paths we may have created for it. (If we tried to gather
>>
>> /* Create GatherPaths for any useful partial paths for rel */
>> - generate_gather_paths(root, rel);
>> + if (lev < levels_needed)
>> + generate_gather_paths(root, rel, NULL);
>>
>> I think at the above two places, and may be in other place also, it's
>> better to mention the reason why we should generate the gather path
>> only if it's not the only rel.
>>
>
> I think the comment you are looking is present where we are calling
> generate_gather_paths in grouping_planner. Instead of adding same or
> similar comment at multiple places, how about if we just say something
> like "See in grouping_planner where we generate gather paths" at all
> other places?
>
>> ----------
>>
>> - if (rel->reloptkind == RELOPT_BASEREL)
>> - generate_gather_paths(root, rel);
>> + if (rel->reloptkind == RELOPT_BASEREL &&
>> root->simple_rel_array_size > 2)
>> + generate_gather_paths(root, rel, NULL);
>>
>> Above, in case it's a partitioned table, root->simple_rel_array_size
>> includes the child rels. So even if it's a simple select without a
>> join rel, simple_rel_array_size would be > 2, and so gather path would
>> be generated here for the root table, and again in grouping_planner().
>>
>
> Yeah, that could be a problem. I think we should ensure that there is
> no append rel list by checking root->append_rel_list. Can you think
> of a better way to handle it?
>
The attached patch fixes both the review comments as discussed above.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers