agora inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/4] warn when setting GUC to a nonextant library
19+ messages / 7 participants
[nested] [flat]
* [PATCH 2/4] warn when setting GUC to a nonextant library
@ 2021-12-13 14:42 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw)
Note that warnings shouldn't be issued during startup (only fatals):
$ ./tmp_install/usr/local/pgsql/bin/postgres -D ./testrun/regress/regress/tmp_check/data -c shared_preload_libraries=ab
2023-07-06 14:47:03.817 CDT postmaster[2608106] FATAL: could not access file "ab": No existe el archivo o el directorio
2023-07-06 14:47:03.817 CDT postmaster[2608106] CONTEXT: while loading shared libraries for setting "shared_preload_libraries"
---
src/backend/commands/variable.c | 95 +++++++++++++++++++
src/backend/utils/misc/guc_tables.c | 6 +-
src/include/utils/guc_hooks.h | 7 ++
.../unsafe_tests/expected/rolenames.out | 19 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 13 +++
.../worker_spi/expected/worker_spi.out | 2 +
.../modules/worker_spi/sql/worker_spi.sql | 2 +
src/test/regress/expected/rules.out | 8 ++
8 files changed, 149 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e076552..9272a0f0e5a 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -40,6 +40,7 @@
#include "utils/timestamp.h"
#include "utils/tzparser.h"
#include "utils/varlena.h"
+#include <unistd.h>
/*
* DATESTYLE
@@ -1210,3 +1211,97 @@ check_ssl(bool *newval, void **extra, GucSource source)
#endif
return true;
}
+
+
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * issue warnings only during an interactive SET, from ALTER
+ * ROLE/DATABASE/SYSTEM.
+ */
+ if (source != PGC_S_TEST)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages
+ * from dlopen() are not portable, so it'd be hard to report any
+ * problem other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0)
+ continue;
+
+ if (source == PGC_S_FILE)
+ /* ALTER SYSTEM */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("The server will currently fail to start with this setting."),
+ errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+ "postgresql.auto.conf"));
+ else
+ /* ALTER ROLE/DATABASE */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("New sessions will currently fail to connect with the new setting."));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
+bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 59ab630ae40..e4a80d87928 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4101,7 +4101,7 @@ struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4112,7 +4112,7 @@ struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4123,7 +4123,7 @@ struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 2ecb9fc0866..de030d750f1 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -160,4 +160,11 @@ extern void assign_xlog_sync_method(int new_sync_method, void *extra);
extern bool check_io_direct(char **newval, void **extra, GucSource source);
extern void assign_io_direct(const char *newval, void *extra);
+extern bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
+
#endif /* GUC_HOOKS_H */
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 61396b2a805..2dcd7cf9ec0 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1083,6 +1083,25 @@ RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+DROP DATABASE regression_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
-- clean up
\c
DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..56fb52d4e08 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION;
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regression_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
-- clean up
\c
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
t
(1 row)
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
$$;
INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 7fd81e6a7d0..299c9cb4d2d 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3453,6 +3453,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "c:/'a"/path"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file ""
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+DETAIL: New sessions will currently fail to connect with the new setting.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--
2.34.1
--Cz2/h24zxIeGl+Za
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0003-errcontext-if-server-fails-to-start-due-to-library-G.patch"
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v5 1/3] warn when setting GUC to a nonextant library
@ 2021-12-13 14:42 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw)
---
src/backend/utils/misc/guc.c | 106 +++++++++++++++++-
.../unsafe_tests/expected/rolenames.out | 19 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 13 +++
.../worker_spi/expected/worker_spi.out | 2 +
.../modules/worker_spi/sql/worker_spi.sql | 2 +
src/test/regress/expected/rules.out | 8 ++
6 files changed, 147 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index af4a1c30689..4e38e73a277 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -176,6 +176,12 @@ static bool call_enum_check_hook(struct config_enum *conf, int *newval,
void **extra, GucSource source, int elevel);
static bool check_log_destination(char **newval, void **extra, GucSource source);
+static bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+static bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+static bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
static void assign_log_destination(const char *newval, void *extra);
static bool check_wal_consistency_checking(char **newval, void **extra,
@@ -4272,7 +4278,7 @@ static struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4283,7 +4289,7 @@ static struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4294,7 +4300,7 @@ static struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
@@ -12506,6 +12512,100 @@ check_max_worker_processes(int *newval, void **extra, GucSource source)
return true;
}
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * issue warnings only during an interactive SET, from ALTER
+ * ROLE/DATABASE/SYSTEM.
+ */
+ if (source != PGC_S_TEST && source != PGC_S_FILE)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages from
+ * dlopen() are not portable, so it'd be hard to report any problem
+ * other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0)
+ continue;
+
+
+ if (source == PGC_S_FILE)
+ /* ALTER SYSTEM */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("The server will currently fail to start with this setting."),
+ errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+ "postgresql.auto.conf"));
+ else
+ /* ALTER ROLE/DATABASE */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("New sessions will currently fail to connect with the new setting."));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+static bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
+static bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+static bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
static bool
check_effective_io_concurrency(int *newval, void **extra, GucSource source)
{
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 88b1ff843be..ac161ce7c33 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1082,6 +1082,25 @@ RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+DROP DATABASE regress_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
-- clean up
\c
DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..cf605b08d30 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION;
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regress_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
-- clean up
\c
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
t
(1 row)
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
$$;
INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 7ec3d2688f0..ea4f9eb39e2 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3392,6 +3392,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "c:/'a"/path"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file ""
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+DETAIL: New sessions will currently fail to connect with the new setting.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--
2.17.1
--3siQDZowHQqNOShm
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v5-0002-errcontext-if-server-fails-to-start-due-to-librar.patch"
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v3 2/2] warn when setting GUC to a nonextant library
@ 2021-12-13 14:42 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw)
XXX: the SPI test is unstable:
-SELECT * FROM schema4.counted;
WARNING: could not load library: $libdir/plugins/worker_spi: cannot open shared object file: No such file or directory
+SELECT * FROM schema4.counted;
---
src/backend/utils/misc/guc.c | 85 ++++++++++++++++++-
.../unsafe_tests/expected/rolenames.out | 1 +
.../worker_spi/expected/worker_spi.out | 2 +
.../modules/worker_spi/sql/worker_spi.sql | 2 +
src/test/regress/expected/rules.out | 5 ++
src/test/regress/sql/rules.sql | 1 +
6 files changed, 93 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 4c94f09c645..5a228a37ee2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -170,6 +170,12 @@ static bool call_enum_check_hook(struct config_enum *conf, int *newval,
void **extra, GucSource source, int elevel);
static bool check_log_destination(char **newval, void **extra, GucSource source);
+static bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+static bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+static bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
static void assign_log_destination(const char *newval, void *extra);
static bool check_wal_consistency_checking(char **newval, void **extra,
@@ -4188,7 +4194,7 @@ static struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4199,7 +4205,7 @@ static struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4210,7 +4216,7 @@ static struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
@@ -12100,6 +12106,79 @@ check_max_worker_processes(int *newval, void **extra, GucSource source)
return true;
}
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages from
+ * dlopen() are not portable, so it'd be hard to report any problem
+ * other than "does not exist".
+ */
+ if (access(filename, R_OK) == -1) // F_OK
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+static bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
+static bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+static bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
static bool
check_effective_io_concurrency(int *newval, void **extra, GucSource source)
{
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index eb608fdc2ea..368b10558d7 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1066,6 +1066,7 @@ GRANT pg_read_all_settings TO regress_role_haspriv;
BEGIN;
-- A GUC using GUC_SUPERUSER_ONLY is useful for negative tests.
SET LOCAL session_preload_libraries TO 'path-to-preload-libraries';
+WARNING: could not access file "$libdir/plugins/path-to-preload-libraries"
SET SESSION AUTHORIZATION regress_role_haspriv;
-- passes with role member of pg_read_all_settings
SHOW session_preload_libraries;
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
t
(1 row)
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
$$;
INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index d652f7b5fb4..ce396814332 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3574,6 +3574,7 @@ drop table hats;
drop table hat_data;
-- test for pg_get_functiondef properly regurgitating SET parameters
-- Note that the function is kept around to stress pg_dump.
+SET check_function_bodies=no;
CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
@@ -3583,6 +3584,10 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+WARNING: could not access file "c:/'a"/path"
+WARNING: could not access file ""
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index b732833e63c..1e42d092862 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1198,6 +1198,7 @@ drop table hat_data;
-- test for pg_get_functiondef properly regurgitating SET parameters
-- Note that the function is kept around to stress pg_dump.
+SET check_function_bodies=no;
CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
--
2.17.1
--9Q79yvCDNTHf0qBd--
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v6 2/4] warn when setting GUC to a nonextant library
@ 2021-12-13 14:42 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw)
TODO: test that warnings aren't issued during startup (only fatals)
---
src/backend/commands/variable.c | 96 +++++++++++++++++++
src/backend/utils/misc/guc.c | 2 +-
src/backend/utils/misc/guc_tables.c | 6 +-
src/include/utils/guc_hooks.h | 7 ++
.../unsafe_tests/expected/rolenames.out | 19 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 13 +++
.../worker_spi/expected/worker_spi.out | 2 +
.../modules/worker_spi/sql/worker_spi.sql | 2 +
src/test/regress/expected/rules.out | 8 ++
9 files changed, 151 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 791bac6715c..85642ab8453 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -40,6 +40,7 @@
#include "utils/timestamp.h"
#include "utils/tzparser.h"
#include "utils/varlena.h"
+#include <unistd.h>
/*
* DATESTYLE
@@ -1210,3 +1211,98 @@ check_ssl(bool *newval, void **extra, GucSource source)
#endif
return true;
}
+
+
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * issue warnings only during an interactive SET, from ALTER
+ * ROLE/DATABASE/SYSTEM.
+ */
+ if (source != PGC_S_TEST)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages from
+ * dlopen() are not portable, so it'd be hard to report any problem
+ * other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0)
+ continue;
+
+
+ if (source == PGC_S_FILE)
+ /* ALTER SYSTEM */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("The server will currently fail to start with this setting."),
+ errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+ "postgresql.auto.conf"));
+ else
+ /* ALTER ROLE/DATABASE */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("New sessions will currently fail to connect with the new setting."));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
+bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ae8810591d6..9c284906d6e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -24,7 +24,7 @@
*/
#include "postgres.h"
-#include <limits.h>
+// #include <limits.h>
#include <sys/stat.h>
#include <unistd.h>
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 05ab087934c..b7c8d8dcdfb 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4015,7 +4015,7 @@ struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4026,7 +4026,7 @@ struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4037,7 +4037,7 @@ struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index f1a9a183b49..3f51436a6f5 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -155,4 +155,11 @@ extern bool check_wal_consistency_checking(char **newval, void **extra,
extern void assign_wal_consistency_checking(const char *newval, void *extra);
extern void assign_xlog_sync_method(int new_sync_method, void *extra);
+extern bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
+
#endif /* GUC_HOOKS_H */
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 88b1ff843be..ac161ce7c33 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1082,6 +1082,25 @@ RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+DROP DATABASE regress_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
-- clean up
\c
DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..cf605b08d30 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION;
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regress_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
-- clean up
\c
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
t
(1 row)
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
$$;
INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 624d0e5aae1..3a36898b8d3 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3398,6 +3398,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "c:/'a"/path"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file ""
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+DETAIL: New sessions will currently fail to connect with the new setting.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--
2.25.1
--vDrIcvfimaPJgjx+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0003-errcontext-if-server-fails-to-start-due-to-librar.patch"
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v4 1/3] warn when setting GUC to a nonextant library
@ 2021-12-13 14:42 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw)
---
src/backend/utils/misc/guc.c | 103 +++++++++++++++++-
.../unsafe_tests/expected/rolenames.out | 1 +
.../worker_spi/expected/worker_spi.out | 2 +
.../modules/worker_spi/sql/worker_spi.sql | 2 +
src/test/regress/expected/rules.out | 9 ++
src/test/regress/sql/rules.sql | 1 +
6 files changed, 115 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f505413a7f9..c44b8ebbfd6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -170,6 +170,12 @@ static bool call_enum_check_hook(struct config_enum *conf, int *newval,
void **extra, GucSource source, int elevel);
static bool check_log_destination(char **newval, void **extra, GucSource source);
+static bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+static bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+static bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
static void assign_log_destination(const char *newval, void *extra);
static bool check_wal_consistency_checking(char **newval, void **extra,
@@ -4198,7 +4204,7 @@ static struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4209,7 +4215,7 @@ static struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4220,7 +4226,7 @@ static struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
@@ -12149,6 +12155,97 @@ check_max_worker_processes(int *newval, void **extra, GucSource source)
return true;
}
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted, bool is_session)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * Do not issue warnings while applying GUCs during startup. That would
+ * issue a FATAL error message, and the warnings would be redundant.
+ */
+ if (!IsUnderPostmaster)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages from
+ * dlopen() are not portable, so it'd be hard to report any problem
+ * other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0) // F_OK
+ continue;
+
+ if (is_session)
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errhint("New sessions will fail with the existing configuration."));
+ else
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errhint("The server will fail to start with the existing configuration."
+ " If it is is shut down, it will be necessary to manually edit the %s file to allow it to start.",
+ "postgresql.auto.conf"));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+static bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true, false);
+}
+
+static bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false, false);
+}
+
+static bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true, true);
+}
+
static bool
check_effective_io_concurrency(int *newval, void **extra, GucSource source)
{
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index eb608fdc2ea..368b10558d7 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1066,6 +1066,7 @@ GRANT pg_read_all_settings TO regress_role_haspriv;
BEGIN;
-- A GUC using GUC_SUPERUSER_ONLY is useful for negative tests.
SET LOCAL session_preload_libraries TO 'path-to-preload-libraries';
+WARNING: could not access file "$libdir/plugins/path-to-preload-libraries"
SET SESSION AUTHORIZATION regress_role_haspriv;
-- passes with role member of pg_read_all_settings
SHOW session_preload_libraries;
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
t
(1 row)
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
$$;
INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 1420288d67b..f3d33dda95e 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3362,6 +3362,7 @@ drop table hats;
drop table hat_data;
-- test for pg_get_functiondef properly regurgitating SET parameters
-- Note that the function is kept around to stress pg_dump.
+SET check_function_bodies=no;
CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
@@ -3371,6 +3372,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
+WARNING: could not access file "c:/'a"/path"
+HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
+WARNING: could not access file ""
+HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 8bdab6dec30..6308e5d27fd 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1198,6 +1198,7 @@ drop table hat_data;
-- test for pg_get_functiondef properly regurgitating SET parameters
-- Note that the function is kept around to stress pg_dump.
+SET check_function_bodies=no;
CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
--
2.17.1
--1Ow488MNN9B9o/ov
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0002-errcontext-if-server-fails-to-start-due-to-librar.patch"
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] Add native windows on arm64 support
@ 2024-02-13 17:52 Andres Freund <[email protected]>
2024-02-13 21:28 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Andres Freund @ 2024-02-13 17:52 UTC (permalink / raw)
To: Dave Cramer <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Michael Paquier <[email protected]>; Anthony Roberts <[email protected]>; Daniel Gustafsson <[email protected]>; Lina Iyer <[email protected]>; Mike Holmes <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
Hi,
On 2024-02-13 12:49:33 -0500, Dave Cramer wrote:
> > I think I might have been on to something - if my human emulation of a
> > preprocessor isn't wrong, we'd end up with
> >
> > #define S_UNLOCK(lock) \
> > do { _ReadWriteBarrier(); (*(lock)) = 0; } while (0)
> >
> > on msvc + arm. And that's entirely insufficient - _ReadWriteBarrier() just
> > limits *compiler* level reordering, not CPU level reordering. I think it's
> > even insufficient on x86[-64], but it's definitely insufficient on arm.
> >
> In fact ReadWriteBarrier has been deprecated _ReadWriteBarrier | Microsoft
> Learn
> <https://learn.microsoft.com/en-us/cpp/intrinsics/readwritebarrier?view=msvc-170;
I'd just ignore that, that's just pushing towards more modern stuff that's
more applicable to C++ than C.
> I did try using atomic_thread_fence as per atomic_thread_fence -
> cppreference.com
> <https://en.cppreference.com/w/c/atomic/atomic_thread_fence;
The semantics of atomic_thread_fence are, uh, very odd. I'd just use
MemoryBarrier().
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] Add native windows on arm64 support
2024-02-13 17:52 Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
@ 2024-02-13 21:28 ` Dave Cramer <[email protected]>
2024-09-24 18:31 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Dave Cramer @ 2024-02-13 21:28 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Michael Paquier <[email protected]>; Anthony Roberts <[email protected]>; Daniel Gustafsson <[email protected]>; Lina Iyer <[email protected]>; Mike Holmes <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Tue, 13 Feb 2024 at 12:52, Andres Freund <[email protected]> wrote:
> Hi,
>
> On 2024-02-13 12:49:33 -0500, Dave Cramer wrote:
> > > I think I might have been on to something - if my human emulation of a
> > > preprocessor isn't wrong, we'd end up with
> > >
> > > #define S_UNLOCK(lock) \
> > > do { _ReadWriteBarrier(); (*(lock)) = 0; } while (0)
> > >
> > > on msvc + arm. And that's entirely insufficient - _ReadWriteBarrier()
> just
> > > limits *compiler* level reordering, not CPU level reordering. I think
> it's
> > > even insufficient on x86[-64], but it's definitely insufficient on arm.
> > >
> > In fact ReadWriteBarrier has been deprecated _ReadWriteBarrier |
> Microsoft
> > Learn
> > <
> https://learn.microsoft.com/en-us/cpp/intrinsics/readwritebarrier?view=msvc-170
> >
>
> I'd just ignore that, that's just pushing towards more modern stuff that's
> more applicable to C++ than C.
>
>
> > I did try using atomic_thread_fence as per atomic_thread_fence -
> > cppreference.com
> > <https://en.cppreference.com/w/c/atomic/atomic_thread_fence;
>
> The semantics of atomic_thread_fence are, uh, very odd. I'd just use
> MemoryBarrier().
>
> #define S_UNLOCK(lock) \
do { MemoryBarrier(); (*(lock)) = 0; } while (0)
#endif
Has no effect.
I have no idea if that is what you meant that I should do ?
Dave
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] Add native windows on arm64 support
2024-02-13 17:52 Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2024-02-13 21:28 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
@ 2024-09-24 18:31 ` Dave Cramer <[email protected]>
2024-09-24 20:01 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
2024-09-28 13:01 ` Re: [PATCH] Add native windows on arm64 support Andrew Dunstan <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Dave Cramer @ 2024-09-24 18:31 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Michael Paquier <[email protected]>; Anthony Roberts <[email protected]>; Daniel Gustafsson <[email protected]>; Lina Iyer <[email protected]>; Mike Holmes <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Tue, 13 Feb 2024 at 16:28, Dave Cramer <[email protected]> wrote:
>
>
>
> On Tue, 13 Feb 2024 at 12:52, Andres Freund <[email protected]> wrote:
>
>> Hi,
>>
>> On 2024-02-13 12:49:33 -0500, Dave Cramer wrote:
>> > > I think I might have been on to something - if my human emulation of a
>> > > preprocessor isn't wrong, we'd end up with
>> > >
>> > > #define S_UNLOCK(lock) \
>> > > do { _ReadWriteBarrier(); (*(lock)) = 0; } while (0)
>> > >
>> > > on msvc + arm. And that's entirely insufficient - _ReadWriteBarrier()
>> just
>> > > limits *compiler* level reordering, not CPU level reordering. I
>> think it's
>> > > even insufficient on x86[-64], but it's definitely insufficient on
>> arm.
>> > >
>> > In fact ReadWriteBarrier has been deprecated _ReadWriteBarrier |
>> Microsoft
>> > Learn
>> > <
>> https://learn.microsoft.com/en-us/cpp/intrinsics/readwritebarrier?view=msvc-170
>> >
>>
>> I'd just ignore that, that's just pushing towards more modern stuff that's
>> more applicable to C++ than C.
>>
>>
>> > I did try using atomic_thread_fence as per atomic_thread_fence -
>> > cppreference.com
>> > <https://en.cppreference.com/w/c/atomic/atomic_thread_fence;
>>
>> The semantics of atomic_thread_fence are, uh, very odd. I'd just use
>> MemoryBarrier().
>>
>> #define S_UNLOCK(lock) \
> do { MemoryBarrier(); (*(lock)) = 0; } while (0)
>
> #endif
>
> Has no effect.
>
> I have no idea if that is what you meant that I should do ?
>
> Dave
>
Revisiting this:
Andrew, can you explain the difference between ninja test (which passes)
and what the build farm does. The buildfarm crashes.
Dave
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] Add native windows on arm64 support
2024-02-13 17:52 Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2024-02-13 21:28 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
2024-09-24 18:31 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
@ 2024-09-24 20:01 ` Dave Cramer <[email protected]>
2024-09-24 20:30 ` Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Dave Cramer @ 2024-09-24 20:01 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Michael Paquier <[email protected]>; Anthony Roberts <[email protected]>; Daniel Gustafsson <[email protected]>; Lina Iyer <[email protected]>; Mike Holmes <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Tue, 24 Sept 2024 at 14:31, Dave Cramer <[email protected]>
wrote:
>
>
> On Tue, 13 Feb 2024 at 16:28, Dave Cramer <[email protected]>
> wrote:
>
>>
>>
>>
>> On Tue, 13 Feb 2024 at 12:52, Andres Freund <[email protected]> wrote:
>>
>>> Hi,
>>>
>>> On 2024-02-13 12:49:33 -0500, Dave Cramer wrote:
>>> > > I think I might have been on to something - if my human emulation of
>>> a
>>> > > preprocessor isn't wrong, we'd end up with
>>> > >
>>> > > #define S_UNLOCK(lock) \
>>> > > do { _ReadWriteBarrier(); (*(lock)) = 0; } while (0)
>>> > >
>>> > > on msvc + arm. And that's entirely insufficient -
>>> _ReadWriteBarrier() just
>>> > > limits *compiler* level reordering, not CPU level reordering. I
>>> think it's
>>> > > even insufficient on x86[-64], but it's definitely insufficient on
>>> arm.
>>> > >
>>> > In fact ReadWriteBarrier has been deprecated _ReadWriteBarrier |
>>> Microsoft
>>> > Learn
>>> > <
>>> https://learn.microsoft.com/en-us/cpp/intrinsics/readwritebarrier?view=msvc-170
>>> >
>>>
>>> I'd just ignore that, that's just pushing towards more modern stuff
>>> that's
>>> more applicable to C++ than C.
>>>
>>>
>>> > I did try using atomic_thread_fence as per atomic_thread_fence -
>>> > cppreference.com
>>> > <https://en.cppreference.com/w/c/atomic/atomic_thread_fence;
>>>
>>> The semantics of atomic_thread_fence are, uh, very odd. I'd just use
>>> MemoryBarrier().
>>>
>>> #define S_UNLOCK(lock) \
>> do { MemoryBarrier(); (*(lock)) = 0; } while (0)
>>
>> #endif
>>
>> Has no effect.
>>
>> I have no idea if that is what you meant that I should do ?
>>
>> Dave
>>
>
>
> Revisiting this:
>
> Andrew, can you explain the difference between ninja test (which passes)
> and what the build farm does. The buildfarm crashes.
>
I have a dmp file with a stack trace if anyone is interested
Dave
>
> Dave
>
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] Add native windows on arm64 support
2024-02-13 17:52 Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2024-02-13 21:28 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
2024-09-24 18:31 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
2024-09-24 20:01 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
@ 2024-09-24 20:30 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Andres Freund @ 2024-09-24 20:30 UTC (permalink / raw)
To: Dave Cramer <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Michael Paquier <[email protected]>; Anthony Roberts <[email protected]>; Daniel Gustafsson <[email protected]>; Lina Iyer <[email protected]>; Mike Holmes <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On 2024-09-24 16:01:30 -0400, Dave Cramer wrote:
> I have a dmp file with a stack trace if anyone is interested
/me raises his hand
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] Add native windows on arm64 support
2024-02-13 17:52 Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2024-02-13 21:28 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
2024-09-24 18:31 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
@ 2024-09-28 13:01 ` Andrew Dunstan <[email protected]>
1 sibling, 0 replies; 19+ messages in thread
From: Andrew Dunstan @ 2024-09-28 13:01 UTC (permalink / raw)
To: Dave Cramer <[email protected]>; Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Anthony Roberts <[email protected]>; Daniel Gustafsson <[email protected]>; Lina Iyer <[email protected]>; Mike Holmes <[email protected]>; Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On 2024-09-24 Tu 2:31 PM, Dave Cramer wrote:
>
>
> On Tue, 13 Feb 2024 at 16:28, Dave Cramer <[email protected]>
> wrote:
>
>
>
>
> On Tue, 13 Feb 2024 at 12:52, Andres Freund <[email protected]>
> wrote:
>
> Hi,
>
> On 2024-02-13 12:49:33 -0500, Dave Cramer wrote:
> > > I think I might have been on to something - if my human
> emulation of a
> > > preprocessor isn't wrong, we'd end up with
> > >
> > > #define S_UNLOCK(lock) \
> > > do { _ReadWriteBarrier(); (*(lock)) = 0; } while (0)
> > >
> > > on msvc + arm. And that's entirely insufficient -
> _ReadWriteBarrier() just
> > > limits *compiler* level reordering, not CPU level
> reordering. I think it's
> > > even insufficient on x86[-64], but it's definitely
> insufficient on arm.
> > >
> > In fact ReadWriteBarrier has been deprecated
> _ReadWriteBarrier | Microsoft
> > Learn
> >
> <https://learn.microsoft.com/en-us/cpp/intrinsics/readwritebarrier?view=msvc-170;
>
> I'd just ignore that, that's just pushing towards more modern
> stuff that's
> more applicable to C++ than C.
>
>
> > I did try using atomic_thread_fence as per atomic_thread_fence -
> > cppreference.com <http://cppreference.com;
> > <https://en.cppreference.com/w/c/atomic/atomic_thread_fence;
>
> The semantics of atomic_thread_fence are, uh, very odd. I'd
> just use
> MemoryBarrier().
>
> #defineS_UNLOCK(lock) \
> do{ MemoryBarrier(); (*(lock)) =0; } while(0)
> #endif
> Has no effect.
>
> I have no idea if that is what you meant that I should do ?
>
> Dave
>
>
>
> Revisiting this:
>
> Andrew, can you explain the difference between ninja test (which
> passes) and what the build farm does. The buildfarm crashes.
The buildfarm client performs these steps:
meson test -C $pgsql --no-rebuild --suite setup
meson test -t $meson_test_timeout $jflag -C $pgsql --logbase checklog --print-errorlogs --no-rebuild --suite regress --test-args=--no-locale
meson test -t $meson_test_timeout $jflag -C $pgsql --print-errorlogs --no-rebuild --logbase checkworld --no-suite setup --no-suite regress
foreach tested locale: meson test -t $meson_test_timeout $jflag -v -C $pgsql --no-rebuild --print-errorlogs --setup running --suite regress-running --logbase regress-installcheck-$locale
$pgsql is the build root, $jflag is setting the number of jobs
IOW, we do test suite setup, run the core regression tests, run all the
remaining non-install tests, then run the install tests for each locale.
We don't call ninja directly, but I don't see why that should make a
difference.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v1 1/1] Use a non-locking initial test in TAS_SPIN on AArch64.
@ 2024-10-22 19:33 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Nathan Bossart @ 2024-10-22 19:33 UTC (permalink / raw)
---
src/include/storage/s_lock.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index e94ed5f48b..07f343baf8 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -270,6 +270,12 @@ tas(volatile slock_t *lock)
*/
#if defined(__aarch64__)
+/*
+ * On ARM64, it's a win to use a non-locking test before the TAS proper. It
+ * may be a win on 32-bit ARM, too, but nobody's tested it yet.
+ */
+#define TAS_SPIN(lock) (*(lock) ? 1 : TAS(lock))
+
#define SPIN_DELAY() spin_delay()
static __inline__ void
--
2.39.5 (Apple Git-154)
--cHDCaEHhDdnHUGIr--
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v1 1/1] Use a non-locking initial test in TAS_SPIN on AArch64.
@ 2024-10-22 19:33 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Nathan Bossart @ 2024-10-22 19:33 UTC (permalink / raw)
---
src/include/storage/s_lock.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index e94ed5f48b..07f343baf8 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -270,6 +270,12 @@ tas(volatile slock_t *lock)
*/
#if defined(__aarch64__)
+/*
+ * On ARM64, it's a win to use a non-locking test before the TAS proper. It
+ * may be a win on 32-bit ARM, too, but nobody's tested it yet.
+ */
+#define TAS_SPIN(lock) (*(lock) ? 1 : TAS(lock))
+
#define SPIN_DELAY() spin_delay()
static __inline__ void
--
2.39.5 (Apple Git-154)
--cHDCaEHhDdnHUGIr--
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Add enable_groupagg GUC parameter to control GroupAggregate usage
@ 2026-07-07 06:47 Richard Guo <[email protected]>
2026-07-07 06:53 ` Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
2026-07-08 11:09 ` RE: Add enable_groupagg GUC parameter to control GroupAggregate usage Tatsuro Yamada <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Richard Guo @ 2026-07-07 06:47 UTC (permalink / raw)
To: Tatsuro Yamada <[email protected]>; +Cc: Tatsuro Yamada <[email protected]>; David Rowley <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Mon, Jun 29, 2026 at 7:02 PM Tatsuro Yamada <[email protected]> wrote:
> Since the enable_groupagg parameter also affects SetOp, Unique, and
> other nodes, I've created documentation patches to clarify this. I'm attaching
> them to this email. They apply on top of your v4 patch.
I've thought it over, and I'm inclined to leave the docs as they are.
My main concern is consistency with the surrounding parameters. All
of the other enable_* GUCs are documented conceptually rather than by
the specific node types they affect, and none of them enumerate the
nodes you'd see in EXPLAIN. Unique and SetOp are really EXPLAIN
labels rather than user-facing concepts, and I'd argue the existing
wording already covers them, since sort-based Unique and sorted SetOp
are both forms of sort-based grouping.
Similarly, enable_hashagg has covered hash-based SetOp for a long time
without us documenting it, which is the same convention at work. I'd
rather not add that now just to match the new parameter.
Attached is a rebased patch to make cfbot work again.
- Richard
Attachments:
[application/octet-stream] v5-0001-Add-an-enable_groupagg-GUC-parameter.patch (33.3K, ../../CAMbWs49RCumHErWYOLm4jDcvLSGqhSm1Asnwsv9Tdn2Yjn0r2A@mail.gmail.com/2-v5-0001-Add-an-enable_groupagg-GUC-parameter.patch)
download | inline diff:
From 4812649b0e7d984378b462eedf22f8ddf3af72a0 Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Wed, 24 Jun 2026 18:09:06 +0900
Subject: [PATCH v5] Add an enable_groupagg GUC parameter
We've long had enable_hashagg to discourage hashed aggregation, but
there was no equivalent for grouping that works by reading presorted
input. That's handy when investigating planner cost misestimates, and
as an escape hatch when a sorted grouping plan is chosen over a much
cheaper hashed one.
enable_groupagg (on by default) covers the GroupAggregate and Group
nodes, the sort-based Unique step used for DISTINCT and semijoin
unique-ification, and the sorted mode of SetOp. It isn't a hard
switch; it only bumps disabled_nodes, so plans with no other choice
are still produced.
Several regression and module tests had been setting enable_sort off,
and one enable_indexscan off, only to force a hashed plan. Use
enable_groupagg there instead, where that was the real intent. In
union.sql this also lets us test the hashed UNION path, which we had
no way to reach before.
Author: Tatsuro Yamada <[email protected]>
Co-authored-by: Richard Guo <[email protected]>
Reviewed-by: Ashutosh Bapat <[email protected]>
Reviewed-by: David Rowley <[email protected]>
Discussion: https://postgr.es/m/CAOKkKFvYHSEsFazkrf9bRH14p-H27XMaqbZfRYjS6EHBruvZMQ@mail.gmail.com
---
contrib/hstore/expected/hstore.out | 4 +-
contrib/hstore/sql/hstore.sql | 4 +-
contrib/ltree/expected/ltree.out | 4 +-
contrib/ltree/sql/ltree.sql | 4 +-
doc/src/sgml/config.sgml | 14 +++++++
src/backend/optimizer/path/costsize.c | 33 ++++++++++++++---
src/backend/optimizer/util/pathnode.c | 20 ++++++++++
src/backend/utils/misc/guc_parameters.dat | 7 ++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/optimizer/cost.h | 1 +
.../injection_points/expected/hashagg.out | 2 +-
.../modules/injection_points/sql/hashagg.sql | 2 +-
src/test/regress/expected/aggregates.out | 10 ++---
src/test/regress/expected/groupingsets.out | 8 ++--
src/test/regress/expected/jsonb.out | 6 +--
src/test/regress/expected/multirangetypes.out | 4 +-
src/test/regress/expected/rangetypes.out | 4 +-
src/test/regress/expected/select_distinct.out | 4 +-
src/test/regress/expected/subselect.out | 2 +-
src/test/regress/expected/sysviews.out | 3 +-
src/test/regress/expected/union.out | 37 ++++++++++++-------
src/test/regress/sql/aggregates.sql | 10 ++---
src/test/regress/sql/groupingsets.sql | 8 ++--
src/test/regress/sql/jsonb.sql | 6 +--
src/test/regress/sql/multirangetypes.sql | 4 +-
src/test/regress/sql/rangetypes.sql | 4 +-
src/test/regress/sql/select_distinct.sql | 4 +-
src/test/regress/sql/subselect.sql | 2 +-
src/test/regress/sql/union.sql | 16 ++++----
29 files changed, 153 insertions(+), 75 deletions(-)
diff --git a/contrib/hstore/expected/hstore.out b/contrib/hstore/expected/hstore.out
index acea8806ba4..9713372b7a4 100644
--- a/contrib/hstore/expected/hstore.out
+++ b/contrib/hstore/expected/hstore.out
@@ -1509,7 +1509,7 @@ select count(*) from (select h from (select * from testhstore union all select *
(1 row)
set enable_hashagg = true;
-set enable_sort = false;
+set enable_groupagg = false;
select count(*) from (select h from (select * from testhstore union all select * from testhstore) hs group by h) hs2;
count
-------
@@ -1522,7 +1522,7 @@ select distinct * from (values (hstore '' || ''),('')) v(h);
(1 row)
-set enable_sort = true;
+set enable_groupagg = true;
-- btree
drop index hidx;
create index hidx on testhstore using btree (h);
diff --git a/contrib/hstore/sql/hstore.sql b/contrib/hstore/sql/hstore.sql
index 8ae95e8a510..ac929e07509 100644
--- a/contrib/hstore/sql/hstore.sql
+++ b/contrib/hstore/sql/hstore.sql
@@ -345,10 +345,10 @@ select count(distinct h) from testhstore;
set enable_hashagg = false;
select count(*) from (select h from (select * from testhstore union all select * from testhstore) hs group by h) hs2;
set enable_hashagg = true;
-set enable_sort = false;
+set enable_groupagg = false;
select count(*) from (select h from (select * from testhstore union all select * from testhstore) hs group by h) hs2;
select distinct * from (values (hstore '' || ''),('')) v(h);
-set enable_sort = true;
+set enable_groupagg = true;
-- btree
drop index hidx;
diff --git a/contrib/ltree/expected/ltree.out b/contrib/ltree/expected/ltree.out
index f1d0eb37b81..0ab623cc204 100644
--- a/contrib/ltree/expected/ltree.out
+++ b/contrib/ltree/expected/ltree.out
@@ -7891,7 +7891,7 @@ reset enable_seqscan;
reset enable_bitmapscan;
-- test hash aggregate
set enable_hashagg=on;
-set enable_sort=off;
+set enable_groupagg=off;
EXPLAIN (COSTS OFF)
SELECT count(*) FROM (
SELECT t FROM (SELECT * FROM ltreetest UNION ALL SELECT * FROM ltreetest) t1 GROUP BY t
@@ -7915,7 +7915,7 @@ SELECT t FROM (SELECT * FROM ltreetest UNION ALL SELECT * FROM ltreetest) t1 GRO
(1 row)
reset enable_hashagg;
-reset enable_sort;
+reset enable_groupagg;
drop index tstidx;
-- test gist index
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
diff --git a/contrib/ltree/sql/ltree.sql b/contrib/ltree/sql/ltree.sql
index 833091dc6bb..5c324160afd 100644
--- a/contrib/ltree/sql/ltree.sql
+++ b/contrib/ltree/sql/ltree.sql
@@ -372,7 +372,7 @@ reset enable_bitmapscan;
-- test hash aggregate
set enable_hashagg=on;
-set enable_sort=off;
+set enable_groupagg=off;
EXPLAIN (COSTS OFF)
SELECT count(*) FROM (
@@ -384,7 +384,7 @@ SELECT t FROM (SELECT * FROM ltreetest UNION ALL SELECT * FROM ltreetest) t1 GRO
) t2;
reset enable_hashagg;
-reset enable_sort;
+reset enable_groupagg;
drop index tstidx;
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 9172a4c5c95..c67130c620e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5863,6 +5863,20 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
</listitem>
</varlistentry>
+ <varlistentry id="guc-enable-groupagg" xreflabel="enable_groupagg">
+ <term><varname>enable_groupagg</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>enable_groupagg</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Enables or disables the query planner's use of sort-based grouping
+ and aggregation plan types. The default is <literal>on</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-enable-hashagg" xreflabel="enable_hashagg">
<term><varname>enable_hashagg</varname> (<type>boolean</type>)
<indexterm>
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 1c575e56ff6..ac523ecf9a8 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -151,6 +151,7 @@ bool enable_tidscan = true;
bool enable_sort = true;
bool enable_incremental_sort = true;
bool enable_hashagg = true;
+bool enable_groupagg = true;
bool enable_nestloop = true;
bool enable_material = true;
bool enable_memoize = true;
@@ -2837,14 +2838,14 @@ cost_agg(Path *path, PlannerInfo *root,
/* we aren't grouping */
total_cost = startup_cost + cpu_tuple_cost;
output_tuples = 1;
+
+ /* AGG_PLAIN neither hashes nor sorts, so neither switch disables it */
}
else if (aggstrategy == AGG_SORTED || aggstrategy == AGG_MIXED)
{
/* Here we are able to deliver output on-the-fly */
startup_cost = input_startup_cost;
total_cost = input_total_cost;
- if (aggstrategy == AGG_MIXED && !enable_hashagg)
- ++disabled_nodes;
/* calcs phrased this way to match HASHED case, see note above */
total_cost += aggcosts->transCost.startup;
total_cost += aggcosts->transCost.per_tuple * input_tuples;
@@ -2853,13 +2854,31 @@ cost_agg(Path *path, PlannerInfo *root,
total_cost += aggcosts->finalCost.per_tuple * numGroups;
total_cost += cpu_tuple_cost * numGroups;
output_tuples = numGroups;
+
+ /*
+ * AGG_MIXED hashes at least one grouping set, so it is disabled when
+ * enable_hashagg is off. Any sorted grouping it also performs is
+ * costed separately, since create_groupingsets_path() calls
+ * cost_agg() once per rollup and the non-hashed rollups come through
+ * as AGG_SORTED.
+ *
+ * AGG_SORTED is disabled when enable_groupagg is off, but only when
+ * there are grouping columns. The empty grouping set arrives with
+ * numGroupCols == 0 and is computed like AGG_PLAIN, with no hashing
+ * or sorting, so it isn't disabled.
+ */
+ if (aggstrategy == AGG_MIXED)
+ {
+ if (!enable_hashagg)
+ ++disabled_nodes;
+ }
+ else if (numGroupCols > 0 && !enable_groupagg) /* AGG_SORTED */
+ ++disabled_nodes;
}
else
{
/* must be AGG_HASHED */
startup_cost = input_total_cost;
- if (!enable_hashagg)
- ++disabled_nodes;
startup_cost += aggcosts->transCost.startup;
startup_cost += aggcosts->transCost.per_tuple * input_tuples;
/* cost of computing hash value */
@@ -2871,6 +2890,10 @@ cost_agg(Path *path, PlannerInfo *root,
/* cost of retrieving from hash table */
total_cost += cpu_tuple_cost * numGroups;
output_tuples = numGroups;
+
+ /* AGG_HASHED is disabled when enable_hashagg is off */
+ if (!enable_hashagg)
+ ++disabled_nodes;
}
/*
@@ -3340,7 +3363,7 @@ cost_group(Path *path, PlannerInfo *root,
}
path->rows = output_tuples;
- path->disabled_nodes = input_disabled_nodes;
+ path->disabled_nodes = input_disabled_nodes + (enable_groupagg ? 0 : 1);
path->startup_cost = startup_cost;
path->total_cost = total_cost;
}
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 73518c8f870..3875e3dd571 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3036,6 +3036,16 @@ create_unique_path(PlannerInfo *root,
cpu_operator_cost * subpath->rows * numCols;
pathnode->path.rows = numGroups;
+ /*
+ * Mark the path as disabled if enable_groupagg is off. While this isn't
+ * a grouping Agg node, it is the sort-based way of removing duplicates
+ * and so is the natural counterpart to the AGG_HASHED path that
+ * enable_hashagg controls; it seems close enough to justify letting that
+ * switch control it.
+ */
+ if (!enable_groupagg)
+ pathnode->path.disabled_nodes++;
+
return pathnode;
}
@@ -3522,6 +3532,16 @@ create_setop_path(PlannerInfo *root,
* qual-checking or projection.
*/
pathnode->path.total_cost += cpu_operator_cost * outputRows;
+
+ /*
+ * Mark the path as disabled if enable_groupagg is off. While this
+ * isn't a grouping Agg node, it is the sort-based implementation and
+ * so is the natural counterpart to the SETOP_HASHED path that
+ * enable_hashagg controls; it seems close enough to justify letting
+ * that switch control it.
+ */
+ if (!enable_groupagg)
+ pathnode->path.disabled_nodes++;
}
else
{
diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.dat
index c9118e71988..dd799a5e70f 100644
--- a/src/backend/utils/misc/guc_parameters.dat
+++ b/src/backend/utils/misc/guc_parameters.dat
@@ -905,6 +905,13 @@
boot_val => 'true',
},
+{ name => 'enable_groupagg', type => 'bool', context => 'PGC_USERSET', group => 'QUERY_TUNING_METHOD',
+ short_desc => 'Enables the planner\'s use of sort-based grouping and aggregation plans.',
+ flags => 'GUC_EXPLAIN',
+ variable => 'enable_groupagg',
+ boot_val => 'true',
+},
+
{ name => 'enable_hashagg', type => 'bool', context => 'PGC_USERSET', group => 'QUERY_TUNING_METHOD',
short_desc => 'Enables the planner\'s use of hashed aggregation plans.',
flags => 'GUC_EXPLAIN',
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index d7942f50a70..47e1221f3c3 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -426,6 +426,7 @@
#enable_async_append = on
#enable_bitmapscan = on
#enable_gathermerge = on
+#enable_groupagg = on
#enable_hashagg = on
#enable_hashjoin = on
#enable_incremental_sort = on
diff --git a/src/include/optimizer/cost.h b/src/include/optimizer/cost.h
index f2fd5d31507..bda3f1690c0 100644
--- a/src/include/optimizer/cost.h
+++ b/src/include/optimizer/cost.h
@@ -57,6 +57,7 @@ extern PGDLLIMPORT bool enable_tidscan;
extern PGDLLIMPORT bool enable_sort;
extern PGDLLIMPORT bool enable_incremental_sort;
extern PGDLLIMPORT bool enable_hashagg;
+extern PGDLLIMPORT bool enable_groupagg;
extern PGDLLIMPORT bool enable_nestloop;
extern PGDLLIMPORT bool enable_material;
extern PGDLLIMPORT bool enable_memoize;
diff --git a/src/test/modules/injection_points/expected/hashagg.out b/src/test/modules/injection_points/expected/hashagg.out
index cc4247af97d..2f75e9be8b7 100644
--- a/src/test/modules/injection_points/expected/hashagg.out
+++ b/src/test/modules/injection_points/expected/hashagg.out
@@ -36,7 +36,7 @@ CREATE TABLE hashagg_ij(x INTEGER);
INSERT INTO hashagg_ij SELECT g FROM generate_series(1,5100) g;
SET max_parallel_workers=0;
SET max_parallel_workers_per_gather=0;
-SET enable_sort=FALSE;
+SET enable_groupagg=FALSE;
SET work_mem='4MB';
SELECT COUNT(*) FROM (SELECT DISTINCT x FROM hashagg_ij) s;
NOTICE: notice triggered for injection point hash-aggregate-spill-1000
diff --git a/src/test/modules/injection_points/sql/hashagg.sql b/src/test/modules/injection_points/sql/hashagg.sql
index 51d814623fc..5d580f8e425 100644
--- a/src/test/modules/injection_points/sql/hashagg.sql
+++ b/src/test/modules/injection_points/sql/hashagg.sql
@@ -17,7 +17,7 @@ INSERT INTO hashagg_ij SELECT g FROM generate_series(1,5100) g;
SET max_parallel_workers=0;
SET max_parallel_workers_per_gather=0;
-SET enable_sort=FALSE;
+SET enable_groupagg=FALSE;
SET work_mem='4MB';
SELECT COUNT(*) FROM (SELECT DISTINCT x FROM hashagg_ij) s;
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 728a3ecd03f..f21f4e225da 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1459,7 +1459,7 @@ drop cascades to table minmaxtest2
drop cascades to table minmaxtest3
-- DISTINCT can also trigger wrong answers with hash aggregation (bug #18465)
begin;
-set local enable_sort = off;
+set local enable_groupagg = off;
explain (costs off)
select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1)
from int4_tbl t0;
@@ -3866,7 +3866,7 @@ select v||'a', case when v||'a' = 'aa' then 1 else 0 end, count(*)
--
-- Hash Aggregation Spill tests
--
-set enable_sort=false;
+set enable_groupagg=false;
set work_mem='64kB';
select unique1, count(*), sum(twothousand) from tenk1
group by unique1
@@ -3925,7 +3925,7 @@ order by sum(twothousand);
(48 rows)
set work_mem to default;
-set enable_sort to default;
+set enable_groupagg to default;
--
-- Compare results between plans using sorting and plans using hash
-- aggregation. Force spilling in both cases by setting work_mem low.
@@ -3974,7 +3974,7 @@ select (g/2)::numeric as c1, array_agg(g::numeric) as c2, count(*) as c3
from agg_data_2k group by g/2;
-- Produce results with hash aggregation
set enable_hashagg = true;
-set enable_sort = false;
+set enable_groupagg = false;
set jit_above_cost = 0;
explain (costs off)
select g%10000 as c1, sum(g::numeric) as c2, count(*) as c3
@@ -4006,7 +4006,7 @@ select (g/2)::numeric as c1, sum(7::int4) as c2, count(*) as c3
create table agg_hash_4 as
select (g/2)::numeric as c1, array_agg(g::numeric) as c2, count(*) as c3
from agg_data_2k group by g/2;
-set enable_sort = true;
+set enable_groupagg = true;
set work_mem to default;
-- Compare group aggregation results to hash aggregation results
(select * from agg_hash_1 except select * from agg_group_1)
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index b08083ec54c..c3f00771f6e 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -2010,7 +2010,7 @@ alter table bug_16784 set (autovacuum_enabled = 'false');
update pg_class set reltuples = 10 where relname='bug_16784';
insert into bug_16784 select g/10, g from generate_series(1,40) g;
set work_mem='64kB';
-set enable_sort = false;
+set enable_groupagg = false;
select * from
(values (1),(2)) v(a),
lateral (select a, i, j, count(*) from
@@ -2205,7 +2205,7 @@ alter table gs_data_1 set (autovacuum_enabled = 'false');
update pg_class set reltuples = 10 where relname='gs_data_1';
set work_mem='64kB';
-- Produce results with sorting.
-set enable_sort = true;
+set enable_groupagg = true;
set enable_hashagg = false;
set jit_above_cost = 0;
explain (costs off)
@@ -2234,7 +2234,7 @@ select g100, g10, sum(g::numeric), count(*), max(g::text)
from gs_data_1 group by cube (g1000, g100,g10);
-- Produce results with hash aggregation.
set enable_hashagg = true;
-set enable_sort = false;
+set enable_groupagg = false;
explain (costs off)
select g100, g10, sum(g::numeric), count(*), max(g::text)
from gs_data_1 group by cube (g1000, g100,g10);
@@ -2255,7 +2255,7 @@ from gs_data_1 group by cube (g1000, g100,g10);
create table gs_hash_1 as
select g100, g10, sum(g::numeric), count(*), max(g::text)
from gs_data_1 group by cube (g1000, g100,g10);
-set enable_sort = true;
+set enable_groupagg = true;
set work_mem to default;
set hash_mem_multiplier to default;
-- Compare results
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index 4e2467852db..b1d2ce5f03a 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -3473,7 +3473,7 @@ SELECT count(*) FROM (SELECT j FROM (SELECT * FROM testjsonb UNION ALL SELECT *
(1 row)
SET enable_hashagg = on;
-SET enable_sort = off;
+SET enable_groupagg = off;
SELECT count(*) FROM (SELECT j FROM (SELECT * FROM testjsonb UNION ALL SELECT * FROM testjsonb) js GROUP BY j) js2;
count
-------
@@ -3486,9 +3486,9 @@ SELECT distinct * FROM (values (jsonb '{}' || ''::text),('{}')) v(j);
{}
(1 row)
-SET enable_sort = on;
+SET enable_groupagg = on;
RESET enable_hashagg;
-RESET enable_sort;
+RESET enable_groupagg;
DROP INDEX jidx;
DROP INDEX jidx_array;
-- btree
diff --git a/src/test/regress/expected/multirangetypes.out b/src/test/regress/expected/multirangetypes.out
index 6006aede31d..d47ce4b6d6a 100644
--- a/src/test/regress/expected/multirangetypes.out
+++ b/src/test/regress/expected/multirangetypes.out
@@ -3442,14 +3442,14 @@ NOTICE: drop cascades to type two_ints_range
--
-- Check behavior when subtype lacks a hash function
--
-set enable_sort = off; -- try to make it pick a hash setop implementation
+set enable_groupagg = off; -- try to make it pick a hash setop implementation
select '{(01,10)}'::varbitmultirange except select '{(10,11)}'::varbitmultirange;
varbitmultirange
------------------
{(01,10)}
(1 row)
-reset enable_sort;
+reset enable_groupagg;
--
-- OUT/INOUT/TABLE functions
--
diff --git a/src/test/regress/expected/rangetypes.out b/src/test/regress/expected/rangetypes.out
index 98ba738fb1d..083e948bc41 100644
--- a/src/test/regress/expected/rangetypes.out
+++ b/src/test/regress/expected/rangetypes.out
@@ -1819,14 +1819,14 @@ NOTICE: drop cascades to type two_ints_range
-- Check behavior when subtype lacks a hash function
--
create type varbitrange as range (subtype = varbit);
-set enable_sort = off; -- try to make it pick a hash setop implementation
+set enable_groupagg = off; -- try to make it pick a hash setop implementation
select '(01,10)'::varbitrange except select '(10,11)'::varbitrange;
varbitrange
-------------
(01,10)
(1 row)
-reset enable_sort;
+reset enable_groupagg;
--
-- OUT/INOUT/TABLE functions
--
diff --git a/src/test/regress/expected/select_distinct.out b/src/test/regress/expected/select_distinct.out
index 379ba0bc9fa..741f7238742 100644
--- a/src/test/regress/expected/select_distinct.out
+++ b/src/test/regress/expected/select_distinct.out
@@ -187,7 +187,7 @@ SELECT DISTINCT hundred, two FROM tenk1;
RESET enable_seqscan;
SET enable_hashagg=TRUE;
-- Produce results with hash aggregation.
-SET enable_sort=FALSE;
+SET enable_groupagg=FALSE;
SET jit_above_cost=0;
EXPLAIN (costs off)
SELECT DISTINCT g%1000 FROM generate_series(0,9999) g;
@@ -203,7 +203,7 @@ SELECT DISTINCT g%1000 FROM generate_series(0,9999) g;
SET jit_above_cost TO DEFAULT;
CREATE TABLE distinct_hash_2 AS
SELECT DISTINCT (g%1000)::text FROM generate_series(0,9999) g;
-SET enable_sort=TRUE;
+SET enable_groupagg=TRUE;
SET work_mem TO DEFAULT;
-- Compare results
(SELECT * FROM distinct_hash_1 EXCEPT SELECT * FROM distinct_group_1)
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index 20140f171af..e7ff7191082 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -1454,7 +1454,7 @@ where o.ten = 0;
-- Test rescan of a hashed SetOp node
--
begin;
-set local enable_sort = off;
+set local enable_groupagg = off;
explain (costs off)
select count(*) from
onek o cross join lateral (
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 132b56a5864..1e327c2afa4 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -161,6 +161,7 @@ select name, setting from pg_settings where name like 'enable%';
enable_eager_aggregate | on
enable_gathermerge | on
enable_group_by_reordering | on
+ enable_groupagg | on
enable_hashagg | on
enable_hashjoin | on
enable_incremental_sort | on
@@ -180,7 +181,7 @@ select name, setting from pg_settings where name like 'enable%';
enable_seqscan | on
enable_sort | on
enable_tidscan | on
-(25 rows)
+(26 rows)
-- There are always wait event descriptions for various types. InjectionPoint
-- may be present or absent, depending on history since last postmaster start.
diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out
index ca9089e9a7d..84abcd6b14f 100644
--- a/src/test/regress/expected/union.out
+++ b/src/test/regress/expected/union.out
@@ -386,14 +386,14 @@ select count(*) from
(1 row)
-- this query will prefer a sorted setop unless we force it.
-set enable_indexscan to off;
+set enable_groupagg to off;
explain (costs off)
select unique1 from tenk1 except select unique2 from tenk1 where unique2 != 10;
- QUERY PLAN
----------------------------------
+ QUERY PLAN
+------------------------------------------------------------
HashSetOp Except
- -> Seq Scan on tenk1
- -> Seq Scan on tenk1 tenk1_1
+ -> Index Only Scan using tenk1_unique1 on tenk1
+ -> Index Only Scan using tenk1_unique2 on tenk1 tenk1_1
Filter: (unique2 <> 10)
(4 rows)
@@ -403,7 +403,7 @@ select unique1 from tenk1 except select unique2 from tenk1 where unique2 != 10;
10
(1 row)
-reset enable_indexscan;
+reset enable_groupagg;
-- the hashed implementation is sensitive to child plans' tuple slot types
explain (costs off)
select * from int8_tbl intersect select q2, q1 from int8_tbl order by 1, 2;
@@ -981,19 +981,30 @@ select except select;
-- check hashed implementation
set enable_hashagg = true;
-set enable_sort = false;
--- We've no way to check hashed UNION as the empty pathkeys in the Append are
--- fine to make use of Unique, which is cheaper than HashAggregate and we've
--- no means to disable Unique.
+set enable_groupagg = false;
+explain (costs off)
+select from generate_series(1,5) union select from generate_series(1,3);
+ QUERY PLAN
+----------------------------------------------------------------
+ HashAggregate
+ -> Append
+ -> Function Scan on generate_series
+ -> Function Scan on generate_series generate_series_1
+(4 rows)
+
explain (costs off)
select from generate_series(1,5) intersect select from generate_series(1,3);
QUERY PLAN
----------------------------------------------------------
- SetOp Intersect
+ HashSetOp Intersect
-> Function Scan on generate_series
-> Function Scan on generate_series generate_series_1
(3 rows)
+select from generate_series(1,5) union select from generate_series(1,3);
+--
+(1 row)
+
select from generate_series(1,5) union all select from generate_series(1,3);
--
(8 rows)
@@ -1016,7 +1027,7 @@ select from generate_series(1,5) except all select from generate_series(1,3);
-- check sorted implementation
set enable_hashagg = false;
-set enable_sort = true;
+set enable_groupagg = true;
explain (costs off)
select from generate_series(1,5) union select from generate_series(1,3);
QUERY PLAN
@@ -1075,7 +1086,7 @@ select from cte union select from cte;
(1 row)
reset enable_hashagg;
-reset enable_sort;
+reset enable_groupagg;
--
-- Check handling of a case with unknown constants. We don't guarantee
-- an undecorated constant will work in all cases, but historically this
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index 342605d5497..5cb9a9dc1be 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -493,7 +493,7 @@ drop table minmaxtest cascade;
-- DISTINCT can also trigger wrong answers with hash aggregation (bug #18465)
begin;
-set local enable_sort = off;
+set local enable_groupagg = off;
explain (costs off)
select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1)
from int4_tbl t0;
@@ -1702,7 +1702,7 @@ select v||'a', case when v||'a' = 'aa' then 1 else 0 end, count(*)
-- Hash Aggregation Spill tests
--
-set enable_sort=false;
+set enable_groupagg=false;
set work_mem='64kB';
select unique1, count(*), sum(twothousand) from tenk1
@@ -1711,7 +1711,7 @@ having sum(fivethous) > 4975
order by sum(twothousand);
set work_mem to default;
-set enable_sort to default;
+set enable_groupagg to default;
--
-- Compare results between plans using sorting and plans using hash
@@ -1766,7 +1766,7 @@ select (g/2)::numeric as c1, array_agg(g::numeric) as c2, count(*) as c3
-- Produce results with hash aggregation
set enable_hashagg = true;
-set enable_sort = false;
+set enable_groupagg = false;
set jit_above_cost = 0;
@@ -1799,7 +1799,7 @@ create table agg_hash_4 as
select (g/2)::numeric as c1, array_agg(g::numeric) as c2, count(*) as c3
from agg_data_2k group by g/2;
-set enable_sort = true;
+set enable_groupagg = true;
set work_mem to default;
-- Compare group aggregation results to hash aggregation results
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index a594449b697..c5d4ed2eb57 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -583,7 +583,7 @@ update pg_class set reltuples = 10 where relname='bug_16784';
insert into bug_16784 select g/10, g from generate_series(1,40) g;
set work_mem='64kB';
-set enable_sort = false;
+set enable_groupagg = false;
select * from
(values (1),(2)) v(a),
@@ -609,7 +609,7 @@ set work_mem='64kB';
-- Produce results with sorting.
-set enable_sort = true;
+set enable_groupagg = true;
set enable_hashagg = false;
set jit_above_cost = 0;
@@ -624,7 +624,7 @@ from gs_data_1 group by cube (g1000, g100,g10);
-- Produce results with hash aggregation.
set enable_hashagg = true;
-set enable_sort = false;
+set enable_groupagg = false;
explain (costs off)
select g100, g10, sum(g::numeric), count(*), max(g::text)
@@ -634,7 +634,7 @@ create table gs_hash_1 as
select g100, g10, sum(g::numeric), count(*), max(g::text)
from gs_data_1 group by cube (g1000, g100,g10);
-set enable_sort = true;
+set enable_groupagg = true;
set work_mem to default;
set hash_mem_multiplier to default;
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index d28ed1c1e85..d4e8d7d8c9e 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -927,13 +927,13 @@ SELECT count(distinct j) FROM testjsonb;
SET enable_hashagg = off;
SELECT count(*) FROM (SELECT j FROM (SELECT * FROM testjsonb UNION ALL SELECT * FROM testjsonb) js GROUP BY j) js2;
SET enable_hashagg = on;
-SET enable_sort = off;
+SET enable_groupagg = off;
SELECT count(*) FROM (SELECT j FROM (SELECT * FROM testjsonb UNION ALL SELECT * FROM testjsonb) js GROUP BY j) js2;
SELECT distinct * FROM (values (jsonb '{}' || ''::text),('{}')) v(j);
-SET enable_sort = on;
+SET enable_groupagg = on;
RESET enable_hashagg;
-RESET enable_sort;
+RESET enable_groupagg;
DROP INDEX jidx;
DROP INDEX jidx_array;
diff --git a/src/test/regress/sql/multirangetypes.sql b/src/test/regress/sql/multirangetypes.sql
index ddff722b28c..cf0fff6fddd 100644
--- a/src/test/regress/sql/multirangetypes.sql
+++ b/src/test/regress/sql/multirangetypes.sql
@@ -862,11 +862,11 @@ drop type two_ints cascade;
-- Check behavior when subtype lacks a hash function
--
-set enable_sort = off; -- try to make it pick a hash setop implementation
+set enable_groupagg = off; -- try to make it pick a hash setop implementation
select '{(01,10)}'::varbitmultirange except select '{(10,11)}'::varbitmultirange;
-reset enable_sort;
+reset enable_groupagg;
--
-- OUT/INOUT/TABLE functions
diff --git a/src/test/regress/sql/rangetypes.sql b/src/test/regress/sql/rangetypes.sql
index 5c4b0337b7a..dbfe0d049da 100644
--- a/src/test/regress/sql/rangetypes.sql
+++ b/src/test/regress/sql/rangetypes.sql
@@ -587,11 +587,11 @@ drop type two_ints cascade;
create type varbitrange as range (subtype = varbit);
-set enable_sort = off; -- try to make it pick a hash setop implementation
+set enable_groupagg = off; -- try to make it pick a hash setop implementation
select '(01,10)'::varbitrange except select '(10,11)'::varbitrange;
-reset enable_sort;
+reset enable_groupagg;
--
-- OUT/INOUT/TABLE functions
diff --git a/src/test/regress/sql/select_distinct.sql b/src/test/regress/sql/select_distinct.sql
index 50ac7dde396..2ed1616b098 100644
--- a/src/test/regress/sql/select_distinct.sql
+++ b/src/test/regress/sql/select_distinct.sql
@@ -81,7 +81,7 @@ SET enable_hashagg=TRUE;
-- Produce results with hash aggregation.
-SET enable_sort=FALSE;
+SET enable_groupagg=FALSE;
SET jit_above_cost=0;
@@ -96,7 +96,7 @@ SET jit_above_cost TO DEFAULT;
CREATE TABLE distinct_hash_2 AS
SELECT DISTINCT (g%1000)::text FROM generate_series(0,9999) g;
-SET enable_sort=TRUE;
+SET enable_groupagg=TRUE;
SET work_mem TO DEFAULT;
diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index 3defbc29177..76bce5cdba5 100644
--- a/src/test/regress/sql/subselect.sql
+++ b/src/test/regress/sql/subselect.sql
@@ -730,7 +730,7 @@ where o.ten = 0;
-- Test rescan of a hashed SetOp node
--
begin;
-set local enable_sort = off;
+set local enable_groupagg = off;
explain (costs off)
select count(*) from
diff --git a/src/test/regress/sql/union.sql b/src/test/regress/sql/union.sql
index 780b704b53b..c8de276c2b5 100644
--- a/src/test/regress/sql/union.sql
+++ b/src/test/regress/sql/union.sql
@@ -135,13 +135,13 @@ select count(*) from
( select unique1 from tenk1 intersect select fivethous from tenk1 ) ss;
-- this query will prefer a sorted setop unless we force it.
-set enable_indexscan to off;
+set enable_groupagg to off;
explain (costs off)
select unique1 from tenk1 except select unique2 from tenk1 where unique2 != 10;
select unique1 from tenk1 except select unique2 from tenk1 where unique2 != 10;
-reset enable_indexscan;
+reset enable_groupagg;
-- the hashed implementation is sensitive to child plans' tuple slot types
explain (costs off)
@@ -321,14 +321,14 @@ select except select;
-- check hashed implementation
set enable_hashagg = true;
-set enable_sort = false;
+set enable_groupagg = false;
--- We've no way to check hashed UNION as the empty pathkeys in the Append are
--- fine to make use of Unique, which is cheaper than HashAggregate and we've
--- no means to disable Unique.
+explain (costs off)
+select from generate_series(1,5) union select from generate_series(1,3);
explain (costs off)
select from generate_series(1,5) intersect select from generate_series(1,3);
+select from generate_series(1,5) union select from generate_series(1,3);
select from generate_series(1,5) union all select from generate_series(1,3);
select from generate_series(1,5) intersect select from generate_series(1,3);
select from generate_series(1,5) intersect all select from generate_series(1,3);
@@ -337,7 +337,7 @@ select from generate_series(1,5) except all select from generate_series(1,3);
-- check sorted implementation
set enable_hashagg = false;
-set enable_sort = true;
+set enable_groupagg = true;
explain (costs off)
select from generate_series(1,5) union select from generate_series(1,3);
@@ -363,7 +363,7 @@ with cte as not materialized (select s from generate_series(1,5) s)
select from cte union select from cte;
reset enable_hashagg;
-reset enable_sort;
+reset enable_groupagg;
--
-- Check handling of a case with unknown constants. We don't guarantee
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Add enable_groupagg GUC parameter to control GroupAggregate usage
2026-07-07 06:47 Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
@ 2026-07-07 06:53 ` Richard Guo <[email protected]>
2026-07-08 11:54 ` RE: Add enable_groupagg GUC parameter to control GroupAggregate usage Tatsuro Yamada <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Richard Guo @ 2026-07-07 06:53 UTC (permalink / raw)
To: Tatsuro Yamada <[email protected]>; +Cc: Tatsuro Yamada <[email protected]>; David Rowley <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Tue, Jul 7, 2026 at 3:47 PM Richard Guo <[email protected]> wrote:
> Attached is a rebased patch to make cfbot work again.
I intend to push this patch soon. That way newly added test cases can
start using enable_groupagg directly. Otherwise we'll have to keep
rebasing it to convert any new tests that use enable_sort to force
hashed grouping. Any thoughts?
- Richard
^ permalink raw reply [nested|flat] 19+ messages in thread
* RE: Add enable_groupagg GUC parameter to control GroupAggregate usage
2026-07-07 06:47 Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
2026-07-07 06:53 ` Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
@ 2026-07-08 11:54 ` Tatsuro Yamada <[email protected]>
2026-07-09 01:01 ` Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Tatsuro Yamada @ 2026-07-08 11:54 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: Tatsuro Yamada <[email protected]>; David Rowley <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
Hi Richard,
>> Attached is a rebased patch to make cfbot work again.
>
>I intend to push this patch soon. That way newly added test cases can
>start using enable_groupagg directly. Otherwise we'll have to keep
>rebasing it to convert any new tests that use enable_sort to force
>hashed grouping. Any thoughts?
I'm in favor of committing this.
Before you do, I'd like to clarify one point regarding the commit message.
Would it make sense to list both of us as authors?
The reason I ask is that you made substantial changes to the patch,
including handling additional plan nodes such as SetOp and adding regression
tests. Given those contributions, I wondered whether it would be more
appropriate for both of us to be listed as authors. (I've seen past commits that
included two Author: lines.)
I'm happy to leave that decision to you.
By the way, I re-ran the test query from the initial email in this thread.
The execution times were:
Default: 25,791.642 ms
SET enable_groupagg TO off;: 1,006.839 ms
This again confirmed an approximately 25x speedup.
Of course, the actual benefit depends on the specific query, but I believe
this parameter will allow users to improve query performance in more situations.
Regards,
Tatsuro Yamada
> -----Original Message-----
> From: Richard Guo <[email protected]>
> Sent: Tuesday, July 7, 2026 3:53 PM
> To: Tatsuro Yamada（山田達朗） <[email protected]>
> Cc: Tatsuro Yamada <[email protected]>; David Rowley
> <[email protected]>; Ashutosh Bapat
> <[email protected]>; [email protected]
> Subject: Re: Add enable_groupagg GUC parameter to control GroupAggregate
> usage
>
> On Tue, Jul 7, 2026 at 3:47 PM Richard Guo <[email protected]>
> wrote:
> > Attached is a rebased patch to make cfbot work again.
>
> I intend to push this patch soon. That way newly added test cases can
> start using enable_groupagg directly. Otherwise we'll have to keep
> rebasing it to convert any new tests that use enable_sort to force
> hashed grouping. Any thoughts?
>
> - Richard
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Add enable_groupagg GUC parameter to control GroupAggregate usage
2026-07-07 06:47 Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
2026-07-07 06:53 ` Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
2026-07-08 11:54 ` RE: Add enable_groupagg GUC parameter to control GroupAggregate usage Tatsuro Yamada <[email protected]>
@ 2026-07-09 01:01 ` Richard Guo <[email protected]>
2026-07-09 10:55 ` RE: Add enable_groupagg GUC parameter to control GroupAggregate usage Tatsuro Yamada <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Richard Guo @ 2026-07-09 01:01 UTC (permalink / raw)
To: Tatsuro Yamada <[email protected]>; +Cc: Tatsuro Yamada <[email protected]>; David Rowley <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
On Wed, Jul 8, 2026 at 8:54 PM Tatsuro Yamada <[email protected]> wrote:
> I'm in favor of committing this.
> Before you do, I'd like to clarify one point regarding the commit message.
> Would it make sense to list both of us as authors?
>
> The reason I ask is that you made substantial changes to the patch,
> including handling additional plan nodes such as SetOp and adding regression
> tests. Given those contributions, I wondered whether it would be more
> appropriate for both of us to be listed as authors. (I've seen past commits that
> included two Author: lines.)
> I'm happy to leave that decision to you.
Pushed.
I still listed myself as Co-authored-by rather than Author, because I
want to give full credit to you, as this is your idea and your patch.
- Richard
^ permalink raw reply [nested|flat] 19+ messages in thread
* RE: Add enable_groupagg GUC parameter to control GroupAggregate usage
2026-07-07 06:47 Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
2026-07-07 06:53 ` Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
2026-07-08 11:54 ` RE: Add enable_groupagg GUC parameter to control GroupAggregate usage Tatsuro Yamada <[email protected]>
2026-07-09 01:01 ` Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
@ 2026-07-09 10:55 ` Tatsuro Yamada <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Tatsuro Yamada @ 2026-07-09 10:55 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: Tatsuro Yamada <[email protected]>; David Rowley <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
Hi Richard,
> Pushed.
>
> I still listed myself as Co-authored-by rather than Author, because I
> want to give full credit to you, as this is your idea and your patch.
Thank you for the commit!
I also appreciate your consideration in giving me full credit for the patch.
Regards,
Tatsuro Yamada
> -----Original Message-----
> From: Richard Guo <[email protected]>
> Sent: Thursday, July 9, 2026 10:02 AM
> To: Tatsuro Yamada（山田達朗） <[email protected]>
> Cc: Tatsuro Yamada <[email protected]>; David Rowley
> <[email protected]>; Ashutosh Bapat
> <[email protected]>; [email protected]
> Subject: Re: Add enable_groupagg GUC parameter to control GroupAggregate
> usage
>
> On Wed, Jul 8, 2026 at 8:54 PM Tatsuro Yamada <[email protected]>
> wrote:
> > I'm in favor of committing this.
> > Before you do, I'd like to clarify one point regarding the commit message.
> > Would it make sense to list both of us as authors?
> >
> > The reason I ask is that you made substantial changes to the patch,
> > including handling additional plan nodes such as SetOp and adding
> regression
> > tests. Given those contributions, I wondered whether it would be more
> > appropriate for both of us to be listed as authors. (I've seen past commits
> that
> > included two Author: lines.)
> > I'm happy to leave that decision to you.
>
> Pushed.
>
> I still listed myself as Co-authored-by rather than Author, because I
> want to give full credit to you, as this is your idea and your patch.
>
> - Richard
^ permalink raw reply [nested|flat] 19+ messages in thread
* RE: Add enable_groupagg GUC parameter to control GroupAggregate usage
2026-07-07 06:47 Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
@ 2026-07-08 11:09 ` Tatsuro Yamada <[email protected]>
1 sibling, 0 replies; 19+ messages in thread
From: Tatsuro Yamada @ 2026-07-08 11:09 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: Tatsuro Yamada <[email protected]>; David Rowley <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>
Hi Richard,
>I've thought it over, and I'm inclined to leave the docs as they are.
>
>My main concern is consistency with the surrounding parameters. All
>of the other enable_* GUCs are documented conceptually rather than by
>the specific node types they affect, and none of them enumerate the
>nodes you'd see in EXPLAIN. Unique and SetOp are really EXPLAIN
>labels rather than user-facing concepts, and I'd argue the existing
>wording already covers them, since sort-based Unique and sorted SetOp
>are both forms of sort-based grouping.
>
>Similarly, enable_hashagg has covered hash-based SetOp for a long time
>without us documenting it, which is the same convention at work. I'd
>rather not add that now just to match the new parameter.
Thanks for your comments and the rebased patch.
I understand and agree with the decision not to modify the documentation.
My initial motivation for proposing the documentation update was to make it
clearer which plan nodes are affected by these GUC parameters. While reviewing
your previous patch, I realized that enable_hashagg and enable_groupagg affect
not only HashAgg and GroupAgg nodes but also other plan nodes.
However, documenting that level of detail only for these parameters would be
inconsistent with the descriptions of the other enable_* GUCs. So I agree that
we should keep the current documentation as it is.
>Attached is a rebased patch to make cfbot work again.
I applied the attached v5 patch to commit 57f93af36, and confirmed that all
regression tests completed successfully.
Regards,
Tatsuro Yamada
> -----Original Message-----
> From: Richard Guo <[email protected]>
> Sent: Tuesday, July 7, 2026 3:47 PM
> To: Tatsuro Yamada（山田達朗） <[email protected]>
> Cc: Tatsuro Yamada <[email protected]>; David Rowley
> <[email protected]>; Ashutosh Bapat
> <[email protected]>; [email protected]
> Subject: Re: Add enable_groupagg GUC parameter to control GroupAggregate
> usage
>
> On Mon, Jun 29, 2026 at 7:02 PM Tatsuro Yamada
> <[email protected]> wrote:
> > Since the enable_groupagg parameter also affects SetOp, Unique, and
> > other nodes, I've created documentation patches to clarify this. I'm
> attaching
> > them to this email. They apply on top of your v4 patch.
>
> I've thought it over, and I'm inclined to leave the docs as they are.
>
> My main concern is consistency with the surrounding parameters. All
> of the other enable_* GUCs are documented conceptually rather than by
> the specific node types they affect, and none of them enumerate the
> nodes you'd see in EXPLAIN. Unique and SetOp are really EXPLAIN
> labels rather than user-facing concepts, and I'd argue the existing
> wording already covers them, since sort-based Unique and sorted SetOp
> are both forms of sort-based grouping.
>
> Similarly, enable_hashagg has covered hash-based SetOp for a long time
> without us documenting it, which is the same convention at work. I'd
> rather not add that now just to match the new parameter.
>
> Attached is a rebased patch to make cfbot work again.
>
> - Richard
^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2026-07-09 10:55 UTC | newest]
Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-12-13 14:42 [PATCH 2/4] warn when setting GUC to a nonextant library Justin Pryzby <[email protected]>
2021-12-13 14:42 [PATCH v5 1/3] warn when setting GUC to a nonextant library Justin Pryzby <[email protected]>
2021-12-13 14:42 [PATCH v3 2/2] warn when setting GUC to a nonextant library Justin Pryzby <[email protected]>
2021-12-13 14:42 [PATCH v6 2/4] warn when setting GUC to a nonextant library Justin Pryzby <[email protected]>
2021-12-13 14:42 [PATCH v4 1/3] warn when setting GUC to a nonextant library Justin Pryzby <[email protected]>
2024-02-13 17:52 Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2024-02-13 21:28 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
2024-09-24 18:31 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
2024-09-24 20:01 ` Re: [PATCH] Add native windows on arm64 support Dave Cramer <[email protected]>
2024-09-24 20:30 ` Re: [PATCH] Add native windows on arm64 support Andres Freund <[email protected]>
2024-09-28 13:01 ` Re: [PATCH] Add native windows on arm64 support Andrew Dunstan <[email protected]>
2024-10-22 19:33 [PATCH v1 1/1] Use a non-locking initial test in TAS_SPIN on AArch64. Nathan Bossart <[email protected]>
2024-10-22 19:33 [PATCH v1 1/1] Use a non-locking initial test in TAS_SPIN on AArch64. Nathan Bossart <[email protected]>
2026-07-07 06:47 Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
2026-07-07 06:53 ` Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
2026-07-08 11:54 ` RE: Add enable_groupagg GUC parameter to control GroupAggregate usage Tatsuro Yamada <[email protected]>
2026-07-09 01:01 ` Re: Add enable_groupagg GUC parameter to control GroupAggregate usage Richard Guo <[email protected]>
2026-07-09 10:55 ` RE: Add enable_groupagg GUC parameter to control GroupAggregate usage Tatsuro Yamada <[email protected]>
2026-07-08 11:09 ` RE: Add enable_groupagg GUC parameter to control GroupAggregate usage Tatsuro Yamada <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox