Thread: is_superuser versus set_config_option's parallelism check
I came across this misbehavior:
regression=# create or replace function foo() returns text as
$$select current_setting('role')$$ language sql
parallel safe set role = postgres;
CREATE FUNCTION
regression=# select foo();
foo
----------
postgres
(1 row)
regression=# set debug_parallel_query to 1;
SET
regression=# select foo();
ERROR: cannot set parameters during a parallel operation
CONTEXT: parallel worker
What is failing is the attempt to update the "is_superuser" GUC
as a side-effect of setting "role". That's coming from here:
/*
* GUC_ACTION_SAVE changes are acceptable during a parallel operation,
* because the current worker will also pop the change. We're probably
* dealing with a function having a proconfig entry. Only the function's
* body should observe the change, and peer workers do not share in the
* execution of a function call started by this worker.
*
* Other changes might need to affect other workers, so forbid them.
*/
if (IsInParallelMode() && changeVal && action != GUC_ACTION_SAVE)
// throw error
Since we're using GUC_ACTION_SET to set "is_superuser", this spits up.
The simplest fix would be to hack this test to allow the action anyway
when context == PGC_INTERNAL, excusing that as "assume the caller
knows what it's doing". That feels pretty grotty though. Perhaps
a cleaner way would be to move this check to some higher code level,
but I'm not sure where would be a good place.
Thoughts?
regards, tom lane
On Fri, Aug 09, 2024 at 02:43:59PM -0400, Tom Lane wrote: > The simplest fix would be to hack this test to allow the action anyway > when context == PGC_INTERNAL, excusing that as "assume the caller > knows what it's doing". That feels pretty grotty though. Perhaps > a cleaner way would be to move this check to some higher code level, > but I'm not sure where would be a good place. From a couple of quick tests, it looks like setting "current_role_is_superuser" directly works. That's still grotty, but at least the grottiness would be localized and not require broad assumptions about callers knowing what they're doing when using PGC_INTERNAL. I wouldn't be surprised if there are other problems with this approach, too. -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Aug 09, 2024 at 02:43:59PM -0400, Tom Lane wrote:
>> The simplest fix would be to hack this test to allow the action anyway
>> when context == PGC_INTERNAL, excusing that as "assume the caller
>> knows what it's doing". That feels pretty grotty though. Perhaps
>> a cleaner way would be to move this check to some higher code level,
>> but I'm not sure where would be a good place.
> From a couple of quick tests, it looks like setting
> "current_role_is_superuser" directly works.
Yeah, I had been thinking along the same lines. Here's a draft
patch. (Still needs some attention to nearby comments, and I can't
avoid the impression that the miscinit.c code in this area could
use refactoring.)
A problem with this is that it couldn't readily be back-patched
further than v14, since we didn't have ReportChangedGUCOptions
before that. Maybe that doesn't matter; given the lack of
previous complaints, maybe we only need to fix this in HEAD.
regards, tom lane
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 6202c5ebe4..7b98b03c10 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -1026,6 +1026,19 @@ show_role(void)
return role_string ? role_string : "none";
}
+const char *
+show_is_superuser(void)
+{
+ /*
+ * Report the real value of current_role_is_superuser, not the
+ * possibly-stale value in the GUC mechanism. The reason we do things
+ * this way is to avoid the overhead and extra failure modes that'd be
+ * involved in using SetConfigOption to set is_superuser every time we
+ * update the session_authorization or role GUCs.
+ */
+ return current_role_is_superuser ? "on" : "off";
+}
+
/*
* PATH VARIABLES
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 537d92c0cf..4052b16ecb 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -822,9 +822,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid, bool bypass_login_chec
/* Record username and superuser status as GUC settings too */
SetConfigOption("session_authorization", rname,
PGC_BACKEND, PGC_S_OVERRIDE);
- SetConfigOption("is_superuser",
- is_superuser ? "on" : "off",
- PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+ current_role_is_superuser = is_superuser;
ReleaseSysCache(roleTup);
}
@@ -854,8 +852,7 @@ InitializeSessionUserIdStandalone(void)
* Since we don't, C code will get NULL, and current_setting() will get an
* empty string.
*/
- SetConfigOption("is_superuser", "on",
- PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+ current_role_is_superuser = true;
}
/*
@@ -909,9 +906,7 @@ SetSessionAuthorization(Oid userid, bool is_superuser)
{
SetSessionUserId(userid, is_superuser);
- SetConfigOption("is_superuser",
- is_superuser ? "on" : "off",
- PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+ current_role_is_superuser = is_superuser;
}
/*
@@ -966,9 +961,7 @@ SetCurrentRoleId(Oid roleid, bool is_superuser)
SetOuterUserId(roleid);
- SetConfigOption("is_superuser",
- is_superuser ? "on" : "off",
- PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+ current_role_is_superuser = is_superuser;
}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0c593b81b4..f99975ed61 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2610,6 +2610,15 @@ ReportChangedGUCOptions(void)
SetConfigOption("in_hot_standby", "false",
PGC_INTERNAL, PGC_S_OVERRIDE);
+ /*
+ * Also, check to see if we need to update the is_superuser GUC. This is
+ * to queue a guc_report_list entry if needed.
+ */
+ if (current_role_is_superuser != current_role_is_superuser_guc)
+ SetConfigOption("is_superuser",
+ current_role_is_superuser ? "on" : "off",
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+
/* Transmit new values of interesting variables */
slist_foreach_modify(iter, &guc_report_list)
{
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index c0a52cdcc3..cf09c59e40 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -516,7 +516,8 @@ bool check_function_bodies = true;
*/
static bool default_with_oids = false;
-bool current_role_is_superuser;
+bool current_role_is_superuser; /* this is the "real" status */
+bool current_role_is_superuser_guc; /* but the GUC table points here */
int log_min_error_statement = ERROR;
int log_min_messages = WARNING;
@@ -1017,9 +1018,9 @@ struct config_bool ConfigureNamesBool[] =
NULL,
GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
},
- ¤t_role_is_superuser,
+ ¤t_role_is_superuser_guc,
false,
- NULL, NULL, NULL
+ NULL, NULL, show_is_superuser
},
{
/*
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 4129ea37ee..efdb4fc815 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -260,6 +260,7 @@ extern PGDLLIMPORT char *event_source;
extern PGDLLIMPORT bool check_function_bodies;
extern PGDLLIMPORT bool current_role_is_superuser;
+extern PGDLLIMPORT bool current_role_is_superuser_guc; /* don't use this */
extern PGDLLIMPORT bool AllowAlterSystem;
extern PGDLLIMPORT bool log_duration;
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 5813dba0a2..d10eb44a2c 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -121,6 +121,7 @@ extern void assign_recovery_target_xid(const char *newval, void *extra);
extern bool check_role(char **newval, void **extra, GucSource source);
extern void assign_role(const char *newval, void *extra);
extern const char *show_role(void);
+extern const char *show_is_superuser(void);
extern bool check_restrict_nonsystem_relation_kind(char **newval, void **extra,
GucSource source);
extern void assign_restrict_nonsystem_relation_kind(const char *newval, void *extra);
On Fri, Aug 09, 2024 at 04:04:15PM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> From a couple of quick tests, it looks like setting >> "current_role_is_superuser" directly works. > > Yeah, I had been thinking along the same lines. Here's a draft > patch. (Still needs some attention to nearby comments, and I can't > avoid the impression that the miscinit.c code in this area could > use refactoring.) Hm. That's a bit more code than I expected. > A problem with this is that it couldn't readily be back-patched > further than v14, since we didn't have ReportChangedGUCOptions > before that. Maybe that doesn't matter; given the lack of > previous complaints, maybe we only need to fix this in HEAD. Another option might be to introduce a new GUC flag or source for anything we want to bypass the check (perhaps with the stipulation that it must also be marked PGC_INTERNAL). I think a new flag would require moving the parallel check down a stanza, but that seems fine. A new source would allow us to limit the damage to specific SetConfigOption() call-sites, but I haven't thought through that idea fully. -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Aug 09, 2024 at 04:04:15PM -0400, Tom Lane wrote:
>> Yeah, I had been thinking along the same lines. Here's a draft
>> patch. (Still needs some attention to nearby comments, and I can't
>> avoid the impression that the miscinit.c code in this area could
>> use refactoring.)
> Hm. That's a bit more code than I expected.
Yeah. I can see a couple of points of attraction in this, but
they're not strong:
* Fewer cycles involved in setting session_authorization or role.
But nobody has really complained that those are slow.
* Gets us out from any other gotchas that may exist or be added
in the SetConfigOption code path, not just this one point.
This is mostly hypothetical, and a regression test case or two
would likely catch any new problems anyway.
> Another option might be to introduce a new GUC flag or source for anything
> we want to bypass the check (perhaps with the stipulation that it must also
> be marked PGC_INTERNAL).
A new GUC flag seems like a promising approach, and better than
giving a blanket exemption to PGC_INTERNAL. At least for
is_superuser, there's no visible value in restricting which
SetConfigOption calls can change it; they'd all need the escape
hatch.
regards, tom lane
I wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> Another option might be to introduce a new GUC flag or source for anything
>> we want to bypass the check (perhaps with the stipulation that it must also
>> be marked PGC_INTERNAL).
> A new GUC flag seems like a promising approach, and better than
> giving a blanket exemption to PGC_INTERNAL. At least for
> is_superuser, there's no visible value in restricting which
> SetConfigOption calls can change it; they'd all need the escape
> hatch.
Here's a draft patch to fix it with a flag, now with regression tests.
Also, now that the error depends on which parameter we're talking
about, I thought it best to include the parameter name in the error
and to re-word it to be more like all the other can't-set-this-now
errors just below it. I'm half tempted to change the errcode and
set_config_option return value to match the others too, ie
ERRCODE_CANT_CHANGE_RUNTIME_PARAM and "return 0" not -1.
I don't think the existing choices here are very well thought
through, and they're certainly inconsistent with a lot of
otherwise-similar-seeming refusals in set_config_option.
regards, tom lane
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0c593b81b4..7dd7b2ec4a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3428,6 +3428,16 @@ set_config_with_handle(const char *name, config_handle *handle,
elevel = ERROR;
}
+ /* if handle is specified, no need to look up option */
+ if (!handle)
+ {
+ record = find_option(name, true, false, elevel);
+ if (record == NULL)
+ return 0;
+ }
+ else
+ record = handle;
+
/*
* GUC_ACTION_SAVE changes are acceptable during a parallel operation,
* because the current worker will also pop the change. We're probably
@@ -3435,26 +3445,20 @@ set_config_with_handle(const char *name, config_handle *handle,
* body should observe the change, and peer workers do not share in the
* execution of a function call started by this worker.
*
+ * Also allow normal setting if the GUC is marked GUC_ALLOW_IN_PARALLEL.
+ *
* Other changes might need to affect other workers, so forbid them.
*/
- if (IsInParallelMode() && changeVal && action != GUC_ACTION_SAVE)
+ if (IsInParallelMode() && changeVal && action != GUC_ACTION_SAVE &&
+ (record->flags & GUC_ALLOW_IN_PARALLEL) == 0)
{
ereport(elevel,
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
- errmsg("cannot set parameters during a parallel operation")));
+ errmsg("parameter \"%s\" cannot be set during a parallel operation",
+ name)));
return -1;
}
- /* if handle is specified, no need to look up option */
- if (!handle)
- {
- record = find_option(name, true, false, elevel);
- if (record == NULL)
- return 0;
- }
- else
- record = handle;
-
/*
* Check if the option can be set at this time. See guc.h for the precise
* rules.
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index c0a52cdcc3..1c4392dd22 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -1015,7 +1015,7 @@ struct config_bool ConfigureNamesBool[] =
{"is_superuser", PGC_INTERNAL, UNGROUPED,
gettext_noop("Shows whether the current user is a superuser."),
NULL,
- GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE |
GUC_ALLOW_IN_PARALLEL
},
¤t_role_is_superuser,
false,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 4129ea37ee..ba6883ae8f 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -223,6 +223,7 @@ typedef enum
#define GUC_DISALLOW_IN_AUTO_FILE \
0x002000 /* can't set in PG_AUTOCONF_FILENAME */
#define GUC_RUNTIME_COMPUTED 0x004000 /* delay processing in 'postgres -C' */
+#define GUC_ALLOW_IN_PARALLEL 0x008000 /* allow setting in parallel mode */
#define GUC_UNIT_KB 0x01000000 /* value is in kilobytes */
#define GUC_UNIT_BLOCKS 0x02000000 /* value is in blocks */
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 68a629619a..a61c138bd0 100644
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -1347,3 +1347,36 @@ select current_setting('session_authorization');
(1 row)
rollback;
+-- test that function option SET ROLE works in parallel workers.
+create role regress_parallel_worker;
+create function set_and_report_role() returns text as
+ $$ select current_setting('role') $$ language sql parallel safe
+ set role = regress_parallel_worker;
+create function set_role_and_error(int) returns int as
+ $$ select 1 / $1 $$ language sql parallel safe
+ set role = regress_parallel_worker;
+set debug_parallel_query = 0;
+select set_and_report_role();
+ set_and_report_role
+-------------------------
+ regress_parallel_worker
+(1 row)
+
+select set_role_and_error(0);
+ERROR: division by zero
+CONTEXT: SQL function "set_role_and_error" statement 1
+set debug_parallel_query = 1;
+select set_and_report_role();
+ set_and_report_role
+-------------------------
+ regress_parallel_worker
+(1 row)
+
+select set_role_and_error(0);
+ERROR: division by zero
+CONTEXT: SQL function "set_role_and_error" statement 1
+parallel worker
+reset debug_parallel_query;
+drop function set_and_report_role();
+drop function set_role_and_error(int);
+drop role regress_parallel_worker;
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 1a0e7f144d..22384b5ad8 100644
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -520,3 +520,26 @@ select current_setting('session_authorization');
set debug_parallel_query = 1;
select current_setting('session_authorization');
rollback;
+
+-- test that function option SET ROLE works in parallel workers.
+create role regress_parallel_worker;
+
+create function set_and_report_role() returns text as
+ $$ select current_setting('role') $$ language sql parallel safe
+ set role = regress_parallel_worker;
+
+create function set_role_and_error(int) returns int as
+ $$ select 1 / $1 $$ language sql parallel safe
+ set role = regress_parallel_worker;
+
+set debug_parallel_query = 0;
+select set_and_report_role();
+select set_role_and_error(0);
+set debug_parallel_query = 1;
+select set_and_report_role();
+select set_role_and_error(0);
+reset debug_parallel_query;
+
+drop function set_and_report_role();
+drop function set_role_and_error(int);
+drop role regress_parallel_worker;
On Fri, Aug 09, 2024 at 06:50:14PM -0400, Tom Lane wrote: > Here's a draft patch to fix it with a flag, now with regression tests. Looks reasonable. > Also, now that the error depends on which parameter we're talking > about, I thought it best to include the parameter name in the error > and to re-word it to be more like all the other can't-set-this-now > errors just below it. I'm half tempted to change the errcode and > set_config_option return value to match the others too, ie > ERRCODE_CANT_CHANGE_RUNTIME_PARAM and "return 0" not -1. > I don't think the existing choices here are very well thought > through, and they're certainly inconsistent with a lot of > otherwise-similar-seeming refusals in set_config_option. This comment for set_config_option() leads me to think we should be returning -1 instead of 0 in many more places in set_config_with_handle(): * Return value: * +1: the value is valid and was successfully applied. * 0: the name or value is invalid (but see below). * -1: the value was not applied because of context, priority, or changeVal. But I haven't thought through it, either. At this point, maybe the comment is wrong... -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Aug 09, 2024 at 06:50:14PM -0400, Tom Lane wrote:
>> Also, now that the error depends on which parameter we're talking
>> about, I thought it best to include the parameter name in the error
>> and to re-word it to be more like all the other can't-set-this-now
>> errors just below it. I'm half tempted to change the errcode and
>> set_config_option return value to match the others too, ie
>> ERRCODE_CANT_CHANGE_RUNTIME_PARAM and "return 0" not -1.
> This comment for set_config_option() leads me to think we should be
> returning -1 instead of 0 in many more places in set_config_with_handle():
> * Return value:
> * +1: the value is valid and was successfully applied.
> * 0: the name or value is invalid (but see below).
> * -1: the value was not applied because of context, priority, or changeVal.
> But I haven't thought through it, either. At this point, maybe the comment
> is wrong...
I poked through all the call sites. The only one that makes a
distinction between 0 and -1 is ProcessConfigFileInternal(),
and what it thinks is:
else if (scres == 0)
{
error = true;
item->errmsg = pstrdup("setting could not be applied");
ConfFileWithError = item->filename;
}
else
{
/* no error, but variable's active value was not changed */
item->applied = true;
}
Now, I don't believe that ProcessConfigFileInternal is ever executed
while IsInParallelMode, so it appears that no caller really cares
about which return code this case would return. However, if you
look through set_config_with_handle the general pattern is that
we "return 0" after any ereport call (either one directly in that
function, or one in a called function). Getting to those of course
implies that elevel is too low to throw an error; but we did think
there was an error condition. We "return -1" in cases where we didn't
ereport anything. So I am still of the opinion that the -1 usage here
is inconsistent, even if it happens to not make a difference today.
Yeah, the header comment could stand to be improved to make this
clearer. I think there are more conditions being checked now than
existed when the comment was written. But the para right below the
bit you quoted is pretty clear that "return 0" is associated with
an ereport.
Maybe
* Return value:
* +1: the value is valid and was successfully applied.
* 0: the name or value is invalid, or it's invalid to try to set
* this GUC now; but elevel was less than ERROR (see below).
* -1: no error detected, but the value was not applied, either
* because changeVal is false or there is some overriding value.
regards, tom lane
On Sat, Aug 10, 2024 at 12:57:36PM -0400, Tom Lane wrote: > Yeah, the header comment could stand to be improved to make this > clearer. I think there are more conditions being checked now than > existed when the comment was written. But the para right below the > bit you quoted is pretty clear that "return 0" is associated with > an ereport. Ah, sorry, ENOCOFFEE. > Maybe > > * Return value: > * +1: the value is valid and was successfully applied. > * 0: the name or value is invalid, or it's invalid to try to set > * this GUC now; but elevel was less than ERROR (see below). > * -1: no error detected, but the value was not applied, either > * because changeVal is false or there is some overriding value. Nevertheless, I think this is an improvement. Regarding returning 0 instead of -1 for the parallel case, I think that follows. While doing some additional research, I noticed this return value was just added in December (commit 059de3c [0]). Before that, it apparently assumed that elevel >= ERROR. With that and your analysis of the call sites, it seems highly unlikely that changing it will cause any problems. For the errcode, I do see that we pretty consistently use ERRCODE_INVALID_TRANSACTION_STATE for "can't do thing during a parallel operation." In fact, it looks like all but one use is for parallel errors. I don't have any particular qualms about changing it to ERRCODE_CANT_CHANGE_RUNTIME_PARAM in set_config_with_handle(), but I thought that was interesting. [0] https://postgr.es/m/2089235.1703617353%40sss.pgh.pa.us -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> Regarding returning 0 instead of -1 for the parallel case, I think that
> follows. While doing some additional research, I noticed this return value
> was just added in December (commit 059de3c [0]). Before that, it
> apparently assumed that elevel >= ERROR. With that and your analysis of
> the call sites, it seems highly unlikely that changing it will cause any
> problems.
Hah ... so the failure to think clearly about which value to use
was mine :-(.
> For the errcode, I do see that we pretty consistently use
> ERRCODE_INVALID_TRANSACTION_STATE for "can't do thing during a parallel
> operation." In fact, it looks like all but one use is for parallel errors.
OK, I'll leave that alone but will change the return code.
regards, tom lane