From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: mert@futo.org
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist
Date: Fri, 20 Jun 2025 17:48:37 -0500
Message-ID: <aFXlRTxMxVyMjfjq@nathan> (raw)
In-Reply-To: <aFXQmZ2sveFAwdjY@nathan>
References: <18964-ba09dea8c98fccd6@postgresql.org>
<aFW8R6Qlgtbp7TOk@nathan>
<2474668.1750451081@sss.pgh.pa.us>
<aFXQmZ2sveFAwdjY@nathan>
Here is a fragile proof-of-concept patch that demonstrates roughly what I
had in mind.
--
nathan
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex 667df448732..ba1bf49e83c 100644--- a/src/backend/utils/misc/guc.c+++ b/src/backend/utils/misc/guc.c@@ -250,7 +250,7 @@ static void reapply_stacked_values(struct config_generic *variable,
GucContext curscontext, GucSource cursource,
Oid cursrole);
static bool validate_option_array_item(const char *name, const char *value,
- bool skipIfNoPermissions);+ bool skipIfNoPermissions, bool isReset);
static void write_auto_conf_file(int fd, const char *filename, ConfigVariable *head);
static void replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
const char *name, const char *value);
@@ -6497,7 +6497,7 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value)
Assert(value);
/* test if the option is valid and we're allowed to set it */
- (void) validate_option_array_item(name, value, false);+ (void) validate_option_array_item(name, value, false, false);
/* normalize name (converts obsolete GUC names to modern spellings) */
record = find_option(name, false, true, WARNING);
@@ -6574,7 +6574,7 @@ GUCArrayDelete(ArrayType *array, const char *name)
Assert(name);
/* test if the option is valid and we're allowed to set it */
- (void) validate_option_array_item(name, NULL, false);+ (void) validate_option_array_item(name, NULL, false, true);
/* normalize name (converts obsolete GUC names to modern spellings) */
record = find_option(name, false, true, WARNING);
@@ -6672,7 +6672,7 @@ GUCArrayReset(ArrayType *array)
*eqsgn = '\0';
/* skip if we have permission to delete it */
- if (validate_option_array_item(val, NULL, true))+ if (validate_option_array_item(val, NULL, true, true))
continue;
/* else add it to the output array */
@@ -6707,10 +6707,18 @@ GUCArrayReset(ArrayType *array)
*/
static bool
validate_option_array_item(const char *name, const char *value,
- bool skipIfNoPermissions)+ bool skipIfNoPermissions, bool isReset)
{
struct config_generic *gconf;
+ bool skip_errors;++ /*+ * For RESET and RESET ALL, we treat unknown custom GUCs with reserved+ * prefixes the same as if a placeholder is created. That requires we+ * skip errors in find_option().+ */+ skip_errors = (isReset && valid_custom_variable_name(name));
/*
* There are three cases to consider:
@@ -6731,14 +6739,19 @@ validate_option_array_item(const char *name, const char *value,
* name is not known and can't be created as a placeholder. Throw error,
* unless skipIfNoPermissions is true, in which case return false.
*/
- gconf = find_option(name, true, skipIfNoPermissions, ERROR);- if (!gconf)+ gconf = find_option(name, true, skipIfNoPermissions || skip_errors, ERROR);+ if (!gconf && !skip_errors)
{
/* not known, failed to make a placeholder */
return false;
}
- if (gconf->flags & GUC_CUSTOM_PLACEHOLDER)+ /*+ * If we reach this point and gconf is NULL, we know we're dealing with a+ * RESET or RESET ALL command for an unknown custom GUC with a reserved+ * prefix. We treat that the same as if a placeholder was created.+ */+ if (!gconf || gconf->flags & GUC_CUSTOM_PLACEHOLDER)
{
/*
* We cannot do any meaningful check on the value, so only permissions
Attachments:
[text/plain] fix_alter_role_reset_v1.patch (3.3K, ../aFXlRTxMxVyMjfjq@nathan/2-fix_alter_role_reset_v1.patch)
download | inline diff:diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex 667df448732..ba1bf49e83c 100644--- a/src/backend/utils/misc/guc.c+++ b/src/backend/utils/misc/guc.c@@ -250,7 +250,7 @@ static void reapply_stacked_values(struct config_generic *variable,
GucContext curscontext, GucSource cursource,
Oid cursrole);
static bool validate_option_array_item(const char *name, const char *value,
- bool skipIfNoPermissions);+ bool skipIfNoPermissions, bool isReset);
static void write_auto_conf_file(int fd, const char *filename, ConfigVariable *head);
static void replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
const char *name, const char *value);
@@ -6497,7 +6497,7 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value)
Assert(value);
/* test if the option is valid and we're allowed to set it */
- (void) validate_option_array_item(name, value, false);+ (void) validate_option_array_item(name, value, false, false);
/* normalize name (converts obsolete GUC names to modern spellings) */
record = find_option(name, false, true, WARNING);
@@ -6574,7 +6574,7 @@ GUCArrayDelete(ArrayType *array, const char *name)
Assert(name);
/* test if the option is valid and we're allowed to set it */
- (void) validate_option_array_item(name, NULL, false);+ (void) validate_option_array_item(name, NULL, false, true);
/* normalize name (converts obsolete GUC names to modern spellings) */
record = find_option(name, false, true, WARNING);
@@ -6672,7 +6672,7 @@ GUCArrayReset(ArrayType *array)
*eqsgn = '\0';
/* skip if we have permission to delete it */
- if (validate_option_array_item(val, NULL, true))+ if (validate_option_array_item(val, NULL, true, true))
continue;
/* else add it to the output array */
@@ -6707,10 +6707,18 @@ GUCArrayReset(ArrayType *array)
*/
static bool
validate_option_array_item(const char *name, const char *value,
- bool skipIfNoPermissions)+ bool skipIfNoPermissions, bool isReset)
{
struct config_generic *gconf;
+ bool skip_errors;++ /*+ * For RESET and RESET ALL, we treat unknown custom GUCs with reserved+ * prefixes the same as if a placeholder is created. That requires we+ * skip errors in find_option().+ */+ skip_errors = (isReset && valid_custom_variable_name(name));
/*
* There are three cases to consider:
@@ -6731,14 +6739,19 @@ validate_option_array_item(const char *name, const char *value,
* name is not known and can't be created as a placeholder. Throw error,
* unless skipIfNoPermissions is true, in which case return false.
*/
- gconf = find_option(name, true, skipIfNoPermissions, ERROR);- if (!gconf)+ gconf = find_option(name, true, skipIfNoPermissions || skip_errors, ERROR);+ if (!gconf && !skip_errors)
{
/* not known, failed to make a placeholder */
return false;
}
- if (gconf->flags & GUC_CUSTOM_PLACEHOLDER)+ /*+ * If we reach this point and gconf is NULL, we know we're dealing with a+ * RESET or RESET ALL command for an unknown custom GUC with a reserved+ * prefix. We treat that the same as if a placeholder was created.+ */+ if (!gconf || gconf->flags & GUC_CUSTOM_PLACEHOLDER)
{
/*
* We cannot do any meaningful check on the value, so only permissions
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, mert@futo.org, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist
In-Reply-To: <aFXlRTxMxVyMjfjq@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox