Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
On 2023-10-16 Mo 20:19, Tom Lane wrote: Currently we have this odd behavior (for a superuser): regression=# ALTER SYSTEM SET foo.bar TO 'baz'; ERROR: unrecognized configuration parameter "foo.bar" regression=# SET foo.bar TO 'baz'; SET regression=# ALTER SYSTEM SET foo.bar TO 'baz'; ALTER SYSTEM That is, you can't ALTER SYSTEM SET a random custom GUC unless there is already a placeholder GUC for it, because the find_option call in AlterSystemSetConfigFile fails. This is surely pretty inconsistent. Either the first ALTER SYSTEM SET ought to succeed, or the second one ought to fail too, because we don't have any more knowledge about the custom GUC than we did before. In the original discussion about this [1], I initially leaned towards "they should both fail", but I reconsidered: there doesn't seem to be any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC name, as long as you're superuser. Hence, attached is a patch for that. Much of it is refactoring to avoid duplicating the code that checks for a reserved GUC name, which I think should still be done here --- otherwise, we're losing a lot of the typo detection that that check was intended to provide. (That is, if you have loaded an extension that defines "foo" as a prefix, we should honor the extension's opinion about whether "foo.bar" is valid.) I also fixed the code for GRANT ON PARAMETER so that it follows the same rules and throws the same errors for invalid cases. There's a chunk of AlterSystemSetConfigFile that now needs indenting one more tab stop, but I didn't do that yet for ease of review. Thoughts? Haven't read the patch but in principle I agree. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
> On 17 Oct 2023, at 05:19, Tom Lane wrote: > > In the original discussion about this [1], I initially leaned towards > "they should both fail", but I reconsidered: there doesn't seem to be > any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC > name, as long as you're superuser. +1 for allowing non-existent custom GUCs. From time to time we have to roll out custom binaries controlled by GUCs that do not exist in normal binaries. Juggling with postgresql.conf would be painful in this case. Best regards, Andrey Borodin.
Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
Thanks for the answer. The code looks good to me. Thanks, Shihao On Thu, Oct 19, 2023 at 12:00 PM Tom Lane wrote: > shihao zhong writes: > > I do like the idea that we should keep the set and the altar system with > > the same behavior. But one thing I am worried about is the typo detected > > here because I usually make that type of mistake myself. I believe we > > should have an extra log to explicitly tell the user this is a `custom > > variable` guc. > > I don't think there's any chance of getting away with that. As noted > upthread, a lot of people use placeholder GUCs as a substitute for a > proper session-variable feature. If we ever get real session variables, > we could start to nudge people away from using placeholders; but right > now too many people would complain about the noise of a warning. > > > Btw, another aspect I want to better understand is if the superuser > session > > called pg_reload_conf with custom variables, does that mean these custom > > variables will override the other active transaction's SET command? > > No, a per-session SET will override a value coming from the config file. > That's independent of whether it's a regular or custom GUC. > > regards, tom lane >
Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
shihao zhong writes: > I do like the idea that we should keep the set and the altar system with > the same behavior. But one thing I am worried about is the typo detected > here because I usually make that type of mistake myself. I believe we > should have an extra log to explicitly tell the user this is a `custom > variable` guc. I don't think there's any chance of getting away with that. As noted upthread, a lot of people use placeholder GUCs as a substitute for a proper session-variable feature. If we ever get real session variables, we could start to nudge people away from using placeholders; but right now too many people would complain about the noise of a warning. > Btw, another aspect I want to better understand is if the superuser session > called pg_reload_conf with custom variables, does that mean these custom > variables will override the other active transaction's SET command? No, a per-session SET will override a value coming from the config file. That's independent of whether it's a regular or custom GUC. regards, tom lane
Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
I do like the idea that we should keep the set and the altar system with the same behavior. But one thing I am worried about is the typo detected here because I usually make that type of mistake myself. I believe we should have an extra log to explicitly tell the user this is a `custom variable` guc. Btw, another aspect I want to better understand is if the superuser session called pg_reload_conf with custom variables, does that mean these custom variables will override the other active transaction's SET command? Thanks, Shihao On Wed, Oct 18, 2023 at 1:59 AM Andrei Lepikhov wrote: > On 18/10/2023 12:15, Tom Lane wrote: > > Andrei Lepikhov writes: > >> "SET foo.bar TO 'smth'" can immediately alter the placeholder's value. > >> But what is the reason that "ALTER SYSTEM SET foo.bar TO 'smth'" doesn't > >> do the same? > > > > Because it's not supposed to take effect until you issue a reload > > command (and maybe not even then, depending on which GUC we're > > talking about). I certainly think it wouldn't make sense for your > > own session to adopt the value ahead of others. > > Thanks for the answer. > Introducing the assignable_custom_variable_name can be helpful. The code > looks good. I think it deserves to be committed - after the indentation > fix, of course. > > -- > regards, > Andrey Lepikhov > Postgres Professional > > > >
Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
On 18/10/2023 12:15, Tom Lane wrote: Andrei Lepikhov writes: "SET foo.bar TO 'smth'" can immediately alter the placeholder's value. But what is the reason that "ALTER SYSTEM SET foo.bar TO 'smth'" doesn't do the same? Because it's not supposed to take effect until you issue a reload command (and maybe not even then, depending on which GUC we're talking about). I certainly think it wouldn't make sense for your own session to adopt the value ahead of others. Thanks for the answer. Introducing the assignable_custom_variable_name can be helpful. The code looks good. I think it deserves to be committed - after the indentation fix, of course. -- regards, Andrey Lepikhov Postgres Professional
Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
Andrei Lepikhov writes: > "SET foo.bar TO 'smth'" can immediately alter the placeholder's value. > But what is the reason that "ALTER SYSTEM SET foo.bar TO 'smth'" doesn't > do the same? Because it's not supposed to take effect until you issue a reload command (and maybe not even then, depending on which GUC we're talking about). I certainly think it wouldn't make sense for your own session to adopt the value ahead of others. regards, tom lane
Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
On 17/10/2023 07:19, Tom Lane wrote: Currently we have this odd behavior (for a superuser): regression=# ALTER SYSTEM SET foo.bar TO 'baz'; ERROR: unrecognized configuration parameter "foo.bar" regression=# SET foo.bar TO 'baz'; SET regression=# ALTER SYSTEM SET foo.bar TO 'baz'; ALTER SYSTEM That is, you can't ALTER SYSTEM SET a random custom GUC unless there is already a placeholder GUC for it, because the find_option call in AlterSystemSetConfigFile fails. This is surely pretty inconsistent. Either the first ALTER SYSTEM SET ought to succeed, or the second one ought to fail too, because we don't have any more knowledge about the custom GUC than we did before. In the original discussion about this [1], I initially leaned towards "they should both fail", but I reconsidered: there doesn't seem to be any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC name, as long as you're superuser. Hence, attached is a patch for that. Much of it is refactoring to avoid duplicating the code that checks for a reserved GUC name, which I think should still be done here --- otherwise, we're losing a lot of the typo detection that that check was intended to provide. (That is, if you have loaded an extension that defines "foo" as a prefix, we should honor the extension's opinion about whether "foo.bar" is valid.) I also fixed the code for GRANT ON PARAMETER so that it follows the same rules and throws the same errors for invalid cases. There's a chunk of AlterSystemSetConfigFile that now needs indenting one more tab stop, but I didn't do that yet for ease of review. Thoughts? I have reviewed this patch. It looks good in general. Now, we can change the placeholder value with the SET command and have one more tool (which may be unusual) to pass some data through the session. Keeping away from the reason why DBMS allows such behaviour, I have one question: "SET foo.bar TO 'smth'" can immediately alter the placeholder's value. But what is the reason that "ALTER SYSTEM SET foo.bar TO 'smth'" doesn't do the same? -- regards, Andrey Lepikhov Postgres Professional
Allow ALTER SYSTEM SET on unrecognized custom GUCs
Currently we have this odd behavior (for a superuser):
regression=# ALTER SYSTEM SET foo.bar TO 'baz';
ERROR: unrecognized configuration parameter "foo.bar"
regression=# SET foo.bar TO 'baz';
SET
regression=# ALTER SYSTEM SET foo.bar TO 'baz';
ALTER SYSTEM
That is, you can't ALTER SYSTEM SET a random custom GUC unless there
is already a placeholder GUC for it, because the find_option call in
AlterSystemSetConfigFile fails. This is surely pretty inconsistent.
Either the first ALTER SYSTEM SET ought to succeed, or the second one
ought to fail too, because we don't have any more knowledge about the
custom GUC than we did before.
In the original discussion about this [1], I initially leaned towards
"they should both fail", but I reconsidered: there doesn't seem to be
any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC
name, as long as you're superuser.
Hence, attached is a patch for that. Much of it is refactoring to
avoid duplicating the code that checks for a reserved GUC name, which
I think should still be done here --- otherwise, we're losing a lot of
the typo detection that that check was intended to provide. (That is,
if you have loaded an extension that defines "foo" as a prefix, we
should honor the extension's opinion about whether "foo.bar" is
valid.) I also fixed the code for GRANT ON PARAMETER so that it
follows the same rules and throws the same errors for invalid cases.
There's a chunk of AlterSystemSetConfigFile that now needs indenting
one more tab stop, but I didn't do that yet for ease of review.
Thoughts?
regards, tom lane
[1]
https://www.postgresql.org/message-id/flat/169746329791.169914.16613647309012285391%40wrigleys.postgresql.org
diff --git a/src/backend/catalog/pg_parameter_acl.c b/src/backend/catalog/pg_parameter_acl.c
index 073392e2c4..f4bc10bafe 100644
--- a/src/backend/catalog/pg_parameter_acl.c
+++ b/src/backend/catalog/pg_parameter_acl.c
@@ -82,11 +82,7 @@ ParameterAclCreate(const char *parameter)
* To prevent cluttering pg_parameter_acl with useless entries, insist
* that the name be valid.
*/
- if (!check_GUC_name_for_parameter_acl(parameter))
- ereport(ERROR,
-(errcode(ERRCODE_INVALID_NAME),
- errmsg("invalid parameter name \"%s\"",
- parameter)));
+ check_GUC_name_for_parameter_acl(parameter);
/* Convert name to the form it should have in pg_parameter_acl. */
parname = convert_GUC_name_for_parameter_acl(parameter);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index c25c697a06..e1ea5561d7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -250,6 +250,8 @@ static void write_auto_conf_file(int fd, const char *filename, ConfigVariable *h
static void replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
const char *name, const char *value);
static bool valid_custom_variable_name(const char *name);
+static bool assignable_custom_variable_name(const char *name, bool skip_errors,
+ int elevel);
static void do_serialize(char **destptr, Size *maxbytes,
const char *fmt,...) pg_attribute_printf(3, 4);
static bool call_bool_check_hook(struct config_bool *conf, bool *newval,
@@ -1063,7 +1065,7 @@ add_guc_variable(struct config_generic *var, int elevel)
*
* It must be two or more identifiers separated by dots, where the rules
* for what is an identifier agree with scan.l. (If you change this rule,
- * adjust the errdetail in find_option().)
+ * adjust the errdetail in assignable_custom_variable_name().)
*/
static bool
valid_custom_variable_name(const char *name)
@@ -1098,6 +1100,71 @@ valid_custom_variable_name(const char *name)
return saw_sep;
}
+/*
+ * Decide whether an unrecognized variable name is allowed to be SET.
+ *
+ * It must pass the syntactic rules of valid_custom_variable_name(),
+ * and it must not be in any namespace already reserved by an extension.
+ * (We make this separate from valid_custom_variable_name() because we don't
+ * apply the reserved-namespace test when reading configuration files.)
+ *
+ * If valid, return true. Otherwise, return false if skip_errors is true,
+ * else throw a suitable error at the specified elevel (and return false
+ * if that's less than ERROR).
+ */
+static bool
+assignable_custom_variable_name(const char *name, bool skip_errors, int elevel)
+{
+ /* If there's no separator, it can't be a custom variable */
+ const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR);
+
+ if (sep != NULL)
+ {
+ size_t classLen = sep - name;
+ ListCell *lc;
+
+ /* The name must be syntactically acceptable ... */
+ if (!valid_custom_variable_name(name))
+ {
+ if (!skip_errors)
+ereport(elevel,
+ (errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid configuration parameter name \"%s\"",
+name),
+ errdetail("Custom parameter names must be two or more simple identifiers separated by dots.")));
+ return false;
+ }
+ /*
