0002-fixup-rework-GUC-options-20220328.patch

From 03e21ff5c7f6a0c4e2d43a0be647f0cd75a04786 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
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;
 }
 
-/************************<DEBUG PART>*************************************/
-bool debug_group_by_reorder_by_pathkeys = true;
-bool debug_group_by_match_order_by = true;
-bool debug_cheapest_group_by = true;
-/************************</DEBUG PART>************************************/
-
 /*
  * 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 OPT GROUP BY>*********************************/
-	{
-		{"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
-	},
-/************************</DEBUG OPT GROUP BY>********************************/
 	/* 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);
-/************************<DEBUG OPT GROUP BY>*********************************/
-extern bool debug_group_by_reorder_by_pathkeys;
-extern bool debug_group_by_match_order_by;
-extern bool debug_cheapest_group_by;
-/************************</DEBUG OPT 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