On 5 September 2017 at 14:04, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Fri, Aug 25, 2017 at 10:08 PM, Robert Haas <robertmh...@gmail.com> wrote:
>> On Mon, Aug 21, 2017 at 5:08 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
>>> (b) I have changed the costing of gather path for path target in
>>> generate_gather_paths which I am not sure is the best way. Another
>>> possibility could have been that I change the code in
>>> apply_projection_to_path as done in the previous patch and just call
>>> it from generate_gather_paths. I have not done that because of your
>>> comment above thread ("is probably unsafe, because it might confuse
>>> code that reaches the modified-in-place path through some other
>>> pointer (e.g. code which expects the RelOptInfo's paths to still be
>>> sorted by cost)."). It is not clear to me what exactly is bothering
>>> you if we directly change costing in apply_projection_to_path.
>>
>> The point I was trying to make is that if you retroactively change the
>> cost of a path after you've already done add_path(), it's too late to
>> change your mind about whether to keep the path. At least according
>> to my current understanding, that's the root of this problem in the
>> first place. On top of that, add_path() and other routines that deal
>> with RelOptInfo path lists expect surviving paths to be ordered by
>> descending cost; if you frob the cost, they might not be properly
>> ordered any more.
>>
>
> Okay, now I understand your point, but I think we already change the
> cost of paths in apply_projection_to_path which is done after add_path
> for top level scan/join paths. I think this matters a lot in case of
> Gather because the cost of computing target list can be divided among
> workers. I have changed the patch such that parallel paths for top
> level scan/join node will be generated after pathtarget is ready. I
> had kept the costing of path targets local to
> apply_projection_to_path() as that makes the patch simple.
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.
----------
- 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().
--
Thanks,
-Amit Khandekar
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
