Skip to content

Commit 39ff056

Browse files
nathan-bossartnathan-bossart
committed
Allow resetting unknown custom GUCs with reserved prefixes.
Currently, ALTER DATABASE/ROLE/SYSTEM RESET [ALL] with an unknown custom GUC with a prefix reserved by MarkGUCPrefixReserved() errors (unless a superuser runs a RESET ALL variant). This is problematic for cases such as an extension library upgrade that removes a GUC. To fix, simply make sure the relevant code paths explicitly allow it. Note that we require superuser or privileges on the parameter to reset it. This is perhaps a bit more restrictive than is necessary, but it's not clear whether further relaxing the requirements is safe. Oversight in commit 8810356. The ALTER SYSTEM fix is dependent on commit 2d870b4, which first appeared in v17. Unfortunately, back-patching that commit would introduce ABI breakage, and while that breakage seems unlikely to bother anyone, it doesn't seem worth the risk. Hence, the ALTER SYSTEM part of this commit is omitted on v15 and v16. Reported-by: Mert Alev <mert@futo.org> Reviewed-by: Laurenz Albe <laurenz.albe@cybertec.at> Discussion: https://postgr.es/m/18964-ba09dea8c98fccd6%40postgresql.org Backpatch-through: 15
1 parent 8c29832 commit 39ff056

File tree

5 files changed

+64
-5
lines changed

5 files changed

+64
-5
lines changed

‎contrib/auto_explain/Makefile

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ OBJS = \
66
auto_explain.o
77
PGFILEDESC = "auto_explain - logging facility for execution plans"
88

9+
REGRESS = alter_reset
10+
911
TAP_TESTS = 1
1012

1113
ifdef USE_PGXS

‎contrib/auto_explain/expected/alter_reset.out

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
--
2+
-- This tests resetting unknown custom GUCs with reserved prefixes. There's
3+
-- nothing specific to auto_explain; this is just a convenient place to put
4+
-- this test.
5+
--
6+
SELECT current_database() AS datname \gset
7+
CREATE ROLE regress_ae_role;
8+
ALTER DATABASE :"datname" SET auto_explain.bogus = 1;
9+
ALTER ROLE regress_ae_role SET auto_explain.bogus = 1;
10+
ALTER ROLE regress_ae_role IN DATABASE :"datname" SET auto_explain.bogus = 1;
11+
ALTER SYSTEM SET auto_explain.bogus = 1;
12+
LOAD 'auto_explain';
13+
WARNING: invalid configuration parameter name "auto_explain.bogus", removing it
14+
DETAIL: "auto_explain" is now a reserved prefix.
15+
ALTER DATABASE :"datname" RESET auto_explain.bogus;
16+
ALTER ROLE regress_ae_role RESET auto_explain.bogus;
17+
ALTER ROLE regress_ae_role IN DATABASE :"datname" RESET auto_explain.bogus;
18+
ALTER SYSTEM RESET auto_explain.bogus;
19+
DROP ROLE regress_ae_role;

‎contrib/auto_explain/meson.build

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@ tests += {
2020
'name': 'auto_explain',
2121
'sd': meson.current_source_dir(),
2222
'bd': meson.current_build_dir(),
23+
'regress': {
24+
'sql': [
25+
'alter_reset',
26+
],
27+
},
2328
'tap': {
2429
'tests': [
2530
't/001_auto_explain.pl',

‎contrib/auto_explain/sql/alter_reset.sql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
--
2+
-- This tests resetting unknown custom GUCs with reserved prefixes. There's
3+
-- nothing specific to auto_explain; this is just a convenient place to put
4+
-- this test.
5+
--
6+
7+
SELECT current_database() AS datname \gset
8+
CREATE ROLE regress_ae_role;
9+
10+
ALTER DATABASE :"datname" SET auto_explain.bogus = 1;
11+
ALTER ROLE regress_ae_role SET auto_explain.bogus = 1;
12+
ALTER ROLE regress_ae_role IN DATABASE :"datname" SET auto_explain.bogus = 1;
13+
ALTER SYSTEM SET auto_explain.bogus = 1;
14+
15+
LOAD 'auto_explain';
16+
17+
ALTER DATABASE :"datname" RESET auto_explain.bogus;
18+
ALTER ROLE regress_ae_role RESET auto_explain.bogus;
19+
ALTER ROLE regress_ae_role IN DATABASE :"datname" RESET auto_explain.bogus;
20+
ALTER SYSTEM RESET auto_explain.bogus;
21+
22+
DROP ROLE regress_ae_role;

‎src/backend/utils/misc/guc.c

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4725,8 +4725,13 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
47254725
* the config file cannot cause postmaster start to fail, so we
47264726
* don't have to be too tense about possibly installing a bad
47274727
* value.)
4728+
*
4729+
* As an exception, we skip this check if this is a RESET command
4730+
* for an unknown custom GUC, else there'd be no way for users to
4731+
* remove such settings with reserved prefixes.
47284732
*/
4729-
(void) assignable_custom_variable_name(name, false, ERROR);
4733+
if (value || !valid_custom_variable_name(name))
4734+
(void) assignable_custom_variable_name(name, false, ERROR);
47304735
}
47314736

47324737
/*
@@ -6713,6 +6718,7 @@ validate_option_array_item(const char *name, const char *value,
67136718

67146719
{
67156720
struct config_generic *gconf;
6721+
bool reset_custom;
67166722

67176723
/*
67186724
* There are three cases to consider:
@@ -6731,16 +6737,21 @@ validate_option_array_item(const char *name, const char *value,
67316737
* it's assumed to be fully validated.)
67326738
*
67336739
* name is not known and can't be created as a placeholder. Throw error,
6734-
* unless skipIfNoPermissions is true, in which case return false.
6740+
* unless skipIfNoPermissions or reset_custom is true. If reset_custom is
6741+
* true, this is a RESET or RESET ALL operation for an unknown custom GUC
6742+
* with a reserved prefix, in which case we want to fall through to the
6743+
* placeholder case described in the preceding paragraph (else there'd be
6744+
* no way for users to remove them). Otherwise, return false.
67356745
*/
6736-
gconf = find_option(name, true, skipIfNoPermissions, ERROR);
6737-
if (!gconf)
6746+
reset_custom = (!value && valid_custom_variable_name(name));
6747+
gconf = find_option(name, true, skipIfNoPermissions || reset_custom, ERROR);
6748+
if (!gconf && !reset_custom)
67386749
{
67396750
/* not known, failed to make a placeholder */
67406751
return false;
67416752
}
67426753

6743-
if (gconf->flags & GUC_CUSTOM_PLACEHOLDER)
6754+
if (!gconf || gconf->flags & GUC_CUSTOM_PLACEHOLDER)
67446755
{
67456756
/*
67466757
* We cannot do any meaningful check on the value, so only permissions

0 commit comments

Comments
 (0)