On Thu, Jul 04, 2019 at 09:29:49AM -0400, James Coleman wrote:
On Tue, Jun 25, 2019 at 7:22 PM Tomas Vondra
<tomas.von...@2ndquadrant.com> wrote:
On Tue, Jun 25, 2019 at 04:53:40PM -0400, James Coleman wrote:
>
>Unrelated: if you or someone else you know that's more familiar with
>the parallel code, I'd be interested in their looking at the patch at
>some point, because I have a suspicion it might not be operating in
...
So I've looked into that, and the reason seems fairly simple - when
generating the Gather Merge paths, we only look at paths that are in
partial_pathlist. See generate_gather_paths().
And we only have sequential + index paths in partial_pathlist, not
incremental sort paths.
IMHO we can do two things:
1) modify generate_gather_paths to also consider incremental sort for
each sorted path, similarly to what create_ordered_paths does
2) modify build_index_paths to also generate an incremental sort path
for each index path
IMHO (1) is the right choice here, because it automatically does the
trick for all other types of ordered paths, not just index scans. So,
something like the attached patch, which gives me plans like this:
...
But I'm not going to claim those are total fixes, it's the minimum I
needed to do to make this particular type of plan work.
Thanks for looking into this!
I intended to apply this to my most recent version of the patch (just
sent a few minutes ago), but when I apply it I noticed that the
partition_aggregate regression tests have several of these failures:
ERROR: could not find pathkey item to sort
I haven't had time to look into the cause yet, so I decided to wait
until the next patch revision.
I wanted to investigate this today, but I can't reprodure it. How are
you building and running the regression tests?
Attached is a patch adding the incremental sort below gather merge, and
also tweaking the costing. But that's mostly for and better planning
decisions, I don't get any pathkey errors even with the first patch.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/optimizer/path/allpaths.c
b/src/backend/optimizer/path/allpaths.c
index 3efc807164..d7bf33f64d 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2719,6 +2719,8 @@ generate_gather_paths(PlannerInfo *root, RelOptInfo *rel,
bool override_rows)
{
Path *subpath = (Path *) lfirst(lc);
GatherMergePath *path;
+ bool is_sorted;
+ int presorted_keys;
if (subpath->pathkeys == NIL)
continue;
@@ -2727,6 +2729,26 @@ generate_gather_paths(PlannerInfo *root, RelOptInfo
*rel, bool override_rows)
path = create_gather_merge_path(root, rel, subpath,
rel->reltarget,
subpath->pathkeys, NULL, rowsp);
add_path(rel, &path->path);
+
+ /* consider incremental sort */
+ is_sorted = pathkeys_common_contained_in(root->sort_pathkeys,
+
subpath->pathkeys, &presorted_keys);
+
+ if (!is_sorted && (presorted_keys > 0))
+ {
+ /* Also consider incremental sort. */
+ subpath = (Path *) create_incremental_sort_path(root,
+
rel,
+
subpath,
+
root->sort_pathkeys,
+
presorted_keys,
+
-1);
+
+ path = create_gather_merge_path(root, rel, subpath,
rel->reltarget,
+
subpath->pathkeys, NULL, rowsp);
+
+ add_path(rel, &path->path);
+ }
}
}
diff --git a/src/backend/optimizer/path/costsize.c
b/src/backend/optimizer/path/costsize.c
index 7f820e7351..c6aa17ba67 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1875,16 +1875,8 @@ cost_incremental_sort(Path *path,
limit_tuples);
/* If we have a LIMIT, adjust the number of groups we'll have to
return. */
- if (limit_tuples > 0 && limit_tuples < input_tuples)
- {
- output_tuples = limit_tuples;
- output_groups = floor(output_tuples / group_tuples) + 1;
- }
- else
- {
- output_tuples = input_tuples;
- output_groups = input_groups;
- }
+ output_tuples = input_tuples;
+ output_groups = input_groups;
/*
* Startup cost of incremental sort is the startup cost of its first
group
