From 03e21ff5c7f6a0c4e2d43a0be647f0cd75a04786 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Mon, 28 Mar 2022 17:14:54 +0200 Subject: [PATCH 2/7] fixup: rework GUC options Replace the three GUC debug options with a single GUC that just disables the optimization and reverts back to the order specified in the query. --- src/backend/optimizer/path/pathkeys.c | 24 ++++++--------- src/backend/utils/misc/guc.c | 42 ++++++-------------------- src/include/optimizer/paths.h | 6 +--- src/test/regress/expected/guc.out | 9 ++---- src/test/regress/expected/sysviews.out | 3 +- 5 files changed, 26 insertions(+), 58 deletions(-) diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c index f18d4c8e4e7..fcc99f0fb1c 100644 --- a/src/backend/optimizer/path/pathkeys.c +++ b/src/backend/optimizer/path/pathkeys.c @@ -31,6 +31,8 @@ #include "utils/lsyscache.h" #include "utils/selfuncs.h" +/* Consider reordering of GROUP BY keys? */ +bool enable_group_by_reordering = true; static bool pathkey_is_redundant(PathKey *new_pathkey, List *pathkeys); static bool matches_boolean_partition_clause(RestrictInfo *rinfo, @@ -337,12 +339,6 @@ pathkeys_contained_in(List *keys1, List *keys2) return false; } -/*************************************************************/ -bool debug_group_by_reorder_by_pathkeys = true; -bool debug_group_by_match_order_by = true; -bool debug_cheapest_group_by = true; -/************************************************************/ - /* * group_keys_reorder_by_pathkeys * Reorder GROUP BY keys to match pathkeys of input path. @@ -361,9 +357,6 @@ group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys, ListCell *lc; int n; - if (debug_group_by_reorder_by_pathkeys == false) - return 0; - if (pathkeys == NIL || *group_pathkeys == NIL) return 0; @@ -598,10 +591,6 @@ get_cheapest_group_keys_order(PlannerInfo *root, double nrows, int nFreeKeys; int nToPermute; - /* If this optimization is disabled, we're done. */ - if (!debug_cheapest_group_by) - return false; - /* If there are less than 2 unsorted pathkeys, we're done. */ if (list_length(*group_pathkeys) - n_preordered < 2) return false; @@ -771,6 +760,13 @@ get_useful_group_keys_orderings(PlannerInfo *root, double nrows, infos = lappend(infos, info); + /* + * If the optimization is disabled, we consider only the pathkey order as + * specified in the query. We don't do any reorderings. + */ + if (!enable_group_by_reordering) + return infos; + /* for grouping sets we can't do any reordering */ if (parse->groupingSets) return infos; @@ -834,7 +830,7 @@ get_useful_group_keys_orderings(PlannerInfo *root, double nrows, * XXX This does nothing if (n_preordered == 0). We shouldn't create the * info in this case. */ - if (root->sort_pathkeys && debug_group_by_match_order_by) + if (root->sort_pathkeys) { n_preordered = group_keys_reorder_by_pathkeys(root->sort_pathkeys, &pathkeys, diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 31f347d469d..9e8ab1420d9 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1182,6 +1182,16 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"enable_group_by_reordering", PGC_USERSET, QUERY_TUNING_METHOD, + gettext_noop("enable reordering of GROUP BY key"), + NULL, + GUC_EXPLAIN + }, + &enable_group_by_reordering, + true, + NULL, NULL, NULL + }, { {"geqo", PGC_USERSET, QUERY_TUNING_GEQO, gettext_noop("Enables genetic query optimization."), @@ -2140,38 +2150,6 @@ static struct config_bool ConfigureNamesBool[] = NULL, NULL, NULL }, -/*********************************************************/ - { - {"debug_group_by_reorder_by_pathkeys", PGC_USERSET, QUERY_TUNING_METHOD, - gettext_noop("enable reorder GROUP BY by pathkeys"), - NULL, - GUC_NOT_IN_SAMPLE - }, - &debug_group_by_reorder_by_pathkeys, - true, - NULL, NULL, NULL - }, - { - {"debug_enable_group_by_match_order_by", PGC_USERSET, QUERY_TUNING_METHOD, - gettext_noop("enable matching GROUP BY by ORDER BY."), - NULL, - GUC_NOT_IN_SAMPLE - }, - &debug_group_by_match_order_by, - true, - NULL, NULL, NULL - }, - { - {"debug_enable_cheapest_group_by", PGC_USERSET, QUERY_TUNING_METHOD, - gettext_noop("find a cheapest order of columns in GROUP BY."), - NULL, - GUC_NOT_IN_SAMPLE - }, - &debug_cheapest_group_by, - true, - NULL, NULL, NULL - }, -/********************************************************/ /* End-of-list marker */ { {NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index 525b594cbd0..2ffa24aa899 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -24,6 +24,7 @@ extern PGDLLIMPORT bool enable_geqo; extern PGDLLIMPORT int geqo_threshold; extern PGDLLIMPORT int min_parallel_table_scan_size; extern PGDLLIMPORT int min_parallel_index_scan_size; +extern PGDLLIMPORT bool enable_group_by_reordering; /* Hook for plugins to get control in set_rel_pathlist() */ typedef void (*set_rel_pathlist_hook_type) (PlannerInfo *root, @@ -206,11 +207,6 @@ extern bool pathkeys_count_contained_in(List *keys1, List *keys2, int *n_common) extern int group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys, List **group_clauses); -/*********************************************************/ -extern bool debug_group_by_reorder_by_pathkeys; -extern bool debug_group_by_match_order_by; -extern bool debug_cheapest_group_by; -/********************************************************/ extern List *get_useful_group_keys_orderings(PlannerInfo *root, double nrows, List *path_pathkeys, List *pathkeys, List *clauses); diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out index 98a16c118f2..3de6404ba5b 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -858,13 +858,10 @@ SELECT name FROM tab_settings_flags SELECT name FROM tab_settings_flags WHERE category ~ '^Query Tuning' AND NOT explain ORDER BY 1; - name --------------------------------------- - debug_enable_cheapest_group_by - debug_enable_group_by_match_order_by - debug_group_by_reorder_by_pathkeys + name +--------------------------- default_statistics_target -(4 rows) +(1 row) -- Runtime-computed GUCs should be part of the preset category. SELECT name FROM tab_settings_flags diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out index 442eeb1e3fe..72891690399 100644 --- a/src/test/regress/expected/sysviews.out +++ b/src/test/regress/expected/sysviews.out @@ -105,6 +105,7 @@ select name, setting from pg_settings where name like 'enable%'; enable_async_append | on enable_bitmapscan | on enable_gathermerge | on + enable_group_by_reordering | on enable_hashagg | on enable_hashjoin | on enable_incremental_sort | on @@ -122,7 +123,7 @@ select name, setting from pg_settings where name like 'enable%'; enable_seqscan | on enable_sort | on enable_tidscan | on -(20 rows) +(21 rows) -- Test that the pg_timezone_names and pg_timezone_abbrevs views are -- more-or-less working. We can't test their contents in any great detail -- 2.34.1