final_improvements.patch
text/x-patch
Filename: final_improvements.patch
Type: text/x-patch
Part: 0
Message:
Re: POC: GROUP BY optimization
Patch
Same data as JSON:
GET /api/v1/attachments/:id/patch
the parsed metadata as JSON — format, series position, per-file stats; never the diff bytes.
API reference →
Format: unified
| File | + | − |
|---|---|---|
| src/backend/optimizer/path/pathkeys.c | 22 | 11 |
| src/backend/optimizer/plan/planner.c | 4 | 4 |
| src/include/nodes/pathnodes.h | 13 | 3 |
| src/tools/pgindent/typedefs.list | 1 | 1 |
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index aa80f6486c..9c079270ec 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -461,7 +461,7 @@ group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys,
/*
* pathkeys_are_duplicate
* Check if give pathkeys are already contained the list of
- * PathKeyInfo's.
+ * GroupByOrdering's.
*/
static bool
pathkeys_are_duplicate(List *infos, List *pathkeys)
@@ -470,7 +470,7 @@ pathkeys_are_duplicate(List *infos, List *pathkeys)
foreach(lc, infos)
{
- PathKeyInfo *info = lfirst_node(PathKeyInfo, lc);
+ GroupByOrdering *info = lfirst_node(GroupByOrdering, lc);
if (compare_pathkeys(pathkeys, info->pathkeys) == PATHKEYS_EQUAL)
return true;
@@ -482,7 +482,7 @@ pathkeys_are_duplicate(List *infos, List *pathkeys)
* get_useful_group_keys_orderings
* Determine which orderings of GROUP BY keys are potentially interesting.
*
- * Returns a list of PathKeyInfo items, each representing an interesting
+ * Returns a list of GroupByOrdering items, each representing an interesting
* ordering of GROUP BY keys. Each item stores pathkeys and clauses in the
* matching order.
*
@@ -495,15 +495,15 @@ pathkeys_are_duplicate(List *infos, List *pathkeys)
List *
get_useful_group_keys_orderings(PlannerInfo *root, Path *path)
{
- Query *parse = root->parse;
- List *infos = NIL;
- PathKeyInfo *info;
+ Query *parse = root->parse;
+ List *infos = NIL;
+ GroupByOrdering *info;
List *pathkeys = root->group_pathkeys;
List *clauses = root->processed_groupClause;
/* always return at least the original pathkeys/clauses */
- info = makeNode(PathKeyInfo);
+ info = makeNode(GroupByOrdering);
info->pathkeys = pathkeys;
info->clauses = clauses;
infos = lappend(infos, info);
@@ -539,7 +539,7 @@ get_useful_group_keys_orderings(PlannerInfo *root, Path *path)
(enable_incremental_sort || n == root->num_groupby_pathkeys) &&
!pathkeys_are_duplicate(infos, pathkeys))
{
- info = makeNode(PathKeyInfo);
+ info = makeNode(GroupByOrdering);
info->pathkeys = pathkeys;
info->clauses = clauses;
@@ -564,7 +564,7 @@ get_useful_group_keys_orderings(PlannerInfo *root, Path *path)
(enable_incremental_sort || n == list_length(root->sort_pathkeys)) &&
!pathkeys_are_duplicate(infos, pathkeys))
{
- info = makeNode(PathKeyInfo);
+ info = makeNode(GroupByOrdering);
info->pathkeys = pathkeys;
info->clauses = clauses;
@@ -574,18 +574,29 @@ get_useful_group_keys_orderings(PlannerInfo *root, Path *path)
#ifdef USE_ASSERT_CHECKING
{
- PathKeyInfo *pinfo = linitial_node(PathKeyInfo, infos);
+ GroupByOrdering *pinfo = linitial_node(GroupByOrdering, infos);
ListCell *lc;
/* Test consistency of info structures */
for_each_from(lc, infos, 1)
{
- info = lfirst_node(PathKeyInfo, lc);
+ ListCell *lc1, *lc2;
+
+ info = lfirst_node(GroupByOrdering, lc);
Assert(list_length(info->clauses) == list_length(pinfo->clauses));
Assert(list_length(info->pathkeys) == list_length(pinfo->pathkeys));
Assert(list_difference(info->clauses, pinfo->clauses) == NIL);
Assert(list_difference_ptr(info->pathkeys, pinfo->pathkeys) == NIL);
+
+ forboth(lc1, info->clauses, lc2, info->pathkeys)
+ {
+ SortGroupClause *sgc = lfirst_node(SortGroupClause, lc1);
+ PathKey *pk = lfirst_node(PathKey, lc2);
+
+ if (pk->pk_eclass->ec_sortref != sgc->tleSortGroupRef)
+ elog(ERROR, "Order of group-by clauses doesn't correspond incoming sort order");
+ }
}
}
#endif
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 861656a192..4751a700ab 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -6882,7 +6882,7 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
foreach(lc2, pathkey_orderings)
{
- PathKeyInfo *info = (PathKeyInfo *) lfirst(lc2);
+ GroupByOrdering *info = (GroupByOrdering *) lfirst(lc2);
/* restore the path (we replace it in the loop) */
path = path_save;
@@ -6963,7 +6963,7 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
/* process all potentially interesting grouping reorderings */
foreach(lc2, pathkey_orderings)
{
- PathKeyInfo *info = (PathKeyInfo *) lfirst(lc2);
+ GroupByOrdering *info = (GroupByOrdering *) lfirst(lc2);
/* restore the path (we replace it in the loop) */
path = path_save;
@@ -7214,7 +7214,7 @@ create_partial_grouping_paths(PlannerInfo *root,
/* process all potentially interesting grouping reorderings */
foreach(lc2, pathkey_orderings)
{
- PathKeyInfo *info = (PathKeyInfo *) lfirst(lc2);
+ GroupByOrdering *info = (GroupByOrdering *) lfirst(lc2);
/* restore the path (we replace it in the loop) */
path = path_save;
@@ -7270,7 +7270,7 @@ create_partial_grouping_paths(PlannerInfo *root,
/* process all potentially interesting grouping reorderings */
foreach(lc2, pathkey_orderings)
{
- PathKeyInfo *info = (PathKeyInfo *) lfirst(lc2);
+ GroupByOrdering *info = (GroupByOrdering *) lfirst(lc2);
/* restore the path (we replace it in the loop) */
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 6c71098f2d..b3862500b6 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -427,6 +427,10 @@ struct PlannerInfo
* containing all the query's rows. Hence, if you want to check whether
* GROUP BY was specified, test for nonempty parse->groupClause, not for
* nonempty processed_groupClause.
+ * Optimiser chooses specific order of group-by clauses during the upper
+ * paths generation process, attempting to use different strategies to
+ * minimize number of sorts or engage incremental sort.
+ * See get_useful_group_keys_orderings for details.
*
* Currently, when grouping sets are specified we do not attempt to
* optimize the groupClause, so that processed_groupClause will be
@@ -1468,14 +1472,20 @@ typedef struct PathKey
} PathKey;
/*
- * Combines the information about pathkeys and the associated clauses.
+ * Contains an order of group-by clauses and corresponding list of pathkeys.
+ *
+ * Order of SortGroupClause elements must correspond the order in the head of
+ * PathKey list:
+ * tleSortGroupRef of N-th element in the clauses must be the same as the value
+ * of ec_sortref in N-th pathkey equivalence class.
*/
-typedef struct PathKeyInfo
+typedef struct GroupByOrdering
{
NodeTag type;
+
List *pathkeys;
List *clauses;
-} PathKeyInfo;
+} GroupByOrdering;
/*
* VolatileFunctionStatus -- allows nodes to cache their
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d551ada325..33eff1e63a 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -4113,7 +4113,7 @@ manifest_writer
rfile
ws_options
ws_file_info
-PathKeyInfo
+GroupByOrdering
TidStore
TidStoreIter
TidStoreIterResult