I wrote:
> Another answer is that maybe the processing of the "role" case
> in particular is just broken.
After digging around a bit more, I think that that is indeed the
right answer. Most of the GUC check functions that have
database-state-dependent behavior are programmed to behave specially
when checking a proposed ALTER USER/DATABASE setting; but check_role
and check_session_authorization did not get that memo. I also
noted that check_temp_buffers would throw an error for no very good
reason. There don't look to be any other troublesome cases.
So I ended up with the attached.
It feels a bit unsatisfactory to have these various check functions
know about this explicitly. However, I'm not sure that there's a
good way to centralize it. Only the check function knows whether
the check it's making is immutable or dependent on DB state --- and
in the former case, not throwing an error wouldn't be an improvement.
Anyway, I'm inclined to think that we should not only apply this
but back-patch it. Two complaints is enough to suggest we have
an issue. Plus, I think there is a dump/reload ordering problem
here: as things stand, "alter user joe set role = bob" would work
or not work depending on the order the roles are created in
and/or the order privileges are granted in.
regards, tom lane
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index c5cf08b423..0c85679420 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -767,6 +767,17 @@ check_session_authorization(char **newval, void **extra, GucSource source)
roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
if (!HeapTupleIsValid(roleTup))
{
+ /*
+ * When source == PGC_S_TEST, we don't throw a hard error for a
+ * nonexistent user name, only a NOTICE. See comments in guc.h.
+ */
+ if (source == PGC_S_TEST)
+ {
+ ereport(NOTICE,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("role \"%s\" does not exist", *newval)));
+ return true;
+ }
GUC_check_errmsg("role \"%s\" does not exist", *newval);
return false;
}
@@ -837,10 +848,23 @@ check_role(char **newval, void **extra, GucSource source)
return false;
}
+ /*
+ * When source == PGC_S_TEST, we don't throw a hard error for a
+ * nonexistent user name or insufficient privileges, only a NOTICE.
+ * See comments in guc.h.
+ */
+
/* Look up the username */
roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
if (!HeapTupleIsValid(roleTup))
{
+ if (source == PGC_S_TEST)
+ {
+ ereport(NOTICE,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("role \"%s\" does not exist", *newval)));
+ return true;
+ }
GUC_check_errmsg("role \"%s\" does not exist", *newval);
return false;
}
@@ -859,6 +883,14 @@ check_role(char **newval, void **extra, GucSource source)
if (!InitializingParallelWorker &&
!is_member_of_role(GetSessionUserId(), roleid))
{
+ if (source == PGC_S_TEST)
+ {
+ ereport(NOTICE,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission will be denied to set role \"%s\"",
+ *newval)));
+ return true;
+ }
GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
GUC_check_errmsg("permission denied to set role \"%s\"",
*newval);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d0a51b507d..2ffefdf943 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11777,8 +11777,9 @@ check_temp_buffers(int *newval, void **extra, GucSource source)
{
/*
* Once local buffers have been initialized, it's too late to change this.
+ * However, if this is only a test call, allow it.
*/
- if (NLocBuffer && NLocBuffer != *newval)
+ if (source != PGC_S_TEST && NLocBuffer && NLocBuffer != *newval)
{
GUC_check_errdetail("\"temp_buffers\" cannot be changed after any temporary tables have been accessed in the session.");
return false;
