documentation fix for SET ROLE

On Wednesday, February 17, 2021, Bossart, Nathan <bossartn@amazon.com>
wrote:

postgres=# ALTER ROLE test1 SET ROLE test2;
ALTER ROLE

I would not have expected this to work - “role” isn’t a
configuration_parameter. Its actually cool that it does, but this doc fix
should address this oversight as well.

David J.

On 2/17/21 2:12 PM, David G. Johnston wrote:

On Wednesday, February 17, 2021, Bossart, Nathan <bossartn@amazon.com
<mailto:bossartn@amazon.com>> wrote:

    postgres=# ALTER ROLE test1 SET ROLE test2;
    ALTER ROLE

I would not have expected this to work - “role” isn’t a
configuration_parameter.  Its actually cool that it does, but this doc fix
should address this oversight as well.

I was surprised this worked too.

But the behavior is consistent with other GUCs. In other words, when you "ALTER
ROLE ... SET ..." you change the default value for the session, and therefore a
RESET just changes to that value.

-- login as postgres
nmx=# show work_mem;
work_mem
----------
200MB
(1 row)

nmx=# set work_mem = '42MB';
SET
nmx=# show work_mem;
work_mem
----------
42MB
(1 row)

nmx=# reset work_mem;
RESET
nmx=# show work_mem;
work_mem
----------
200MB
(1 row)

ALTER ROLE test1 SET work_mem = '42MB';

-- login as test1
nmx=> show work_mem;
work_mem
----------
42MB
(1 row)

nmx=> reset work_mem;
RESET
nmx=> show work_mem;
work_mem
----------
42MB
(1 row)

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 2/17/21, 12:15 PM, "Joe Conway" <mail@joeconway.com> wrote:

On 2/17/21 2:12 PM, David G. Johnston wrote:

On Wednesday, February 17, 2021, Bossart, Nathan <bossartn@amazon.com
<mailto:bossartn@amazon.com>> wrote:

postgres=# ALTER ROLE test1 SET ROLE test2;
ALTER ROLE

I would not have expected this to work - “role” isn’t a
configuration_parameter. Its actually cool that it does, but this doc fix
should address this oversight as well.

I was surprised this worked too.

But the behavior is consistent with other GUCs. In other words, when you "ALTER
ROLE ... SET ..." you change the default value for the session, and therefore a
RESET just changes to that value.

Looking further, I noticed that session_authorization does not work
the same way. AFAICT this is because it's set via SetConfigOption()
in InitializeSessionUserId(). If you initialize role here, it acts
the same as session_authorization.

diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 0f67b99cc5..a201bb3766 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -761,6 +761,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
     }

/* Record username and superuser status as GUC settings too */
+ SetConfigOption("role", rname, PGC_BACKEND, PGC_S_OVERRIDE);
SetConfigOption("session_authorization", rname,
PGC_BACKEND, PGC_S_OVERRIDE);
SetConfigOption("is_superuser",

Nathan

On 2/17/21 2:12 PM, David G. Johnston wrote:

On Wednesday, February 17, 2021, Bossart, Nathan <bossartn@amazon.com
<mailto:bossartn@amazon.com>> wrote:

postgres=# ALTER ROLE test1 SET ROLE test2;
ALTER ROLE

I would not have expected this to work - “role” isn’t a
configuration_parameter. Its actually cool that it does, but this doc fix
should address this oversight as well.

Here's a patch that adds "role" and "session authorization" as
configuration parameters, too.

Nathan

On Thu, Feb 18, 2021 at 6:18 PM Bossart, Nathan <bossartn@amazon.com> wrote:

On 2/17/21 2:12 PM, David G. Johnston wrote:

On Wednesday, February 17, 2021, Bossart, Nathan <bossartn@amazon.com
<mailto:bossartn@amazon.com>> wrote:

postgres=# ALTER ROLE test1 SET ROLE test2;
ALTER ROLE

I would not have expected this to work - “role” isn’t a
configuration_parameter. Its actually cool that it does, but this doc

fix

should address this oversight as well.

Here's a patch that adds "role" and "session authorization" as
configuration parameters, too.

You will want to add this to the commitfest if you haven't already.

I would suggest adding a section titled "Identification" and placing these
under that.

Reading it over it looks good. One point though: SET and SET ROLE are
indeed "at run-time" (not 'run time'). ALTER ROLE and ALTER DATABASE
should be considered "at connection-time" just like the command-line
options.

David J.

On Mon, Mar 8, 2021 at 4:41 PM David G. Johnston <david.g.johnston@gmail.com>
wrote:

On Thu, Feb 18, 2021 at 6:18 PM Bossart, Nathan <bossartn@amazon.com>
wrote:

On 2/17/21 2:12 PM, David G. Johnston wrote:

On Wednesday, February 17, 2021, Bossart, Nathan <bossartn@amazon.com
<mailto:bossartn@amazon.com>> wrote:

postgres=# ALTER ROLE test1 SET ROLE test2;
ALTER ROLE

I would not have expected this to work - “role” isn’t a
configuration_parameter. Its actually cool that it does, but this doc

fix

should address this oversight as well.

Here's a patch that adds "role" and "session authorization" as
configuration parameters, too.

You will want to add this to the commitfest if you haven't already.

I would suggest adding a section titled "Identification" and placing these
under that.

Reading it over it looks good. One point though: SET and SET ROLE are
indeed "at run-time" (not 'run time'). ALTER ROLE and ALTER DATABASE
should be considered "at connection-time" just like the command-line
options.

Also, as a nearby email just reminded me, the determination of which role
name is used to figure out default settings is the presented user name, not
the one that would result from a connection-time role change as described
here - though this should be tested, and then documented.

David J.

Thanks for reviewing.

On 3/8/21 3:49 PM, David G. Johnston wrote:

On Mon, Mar 8, 2021 at 4:41 PM David G. Johnston <david.g.johnston@gmail.com> wrote:

You will want to add this to the commitfest if you haven't already.

Here is the commitfest entry: https://commitfest.postgresql.org/32/2993/

I would suggest adding a section titled "Identification" and placing these under that.

Good idea.

Reading it over it looks good. One point though: SET and SET ROLE are indeed "at run-time" (not 'run time'). ALTER ROLE and ALTER DATABASE should be considered "at connection-time" just like the command-line options.

Makes sense. I've applied these changes.

Also, as a nearby email just reminded me, the determination of which role name is used to figure out default settings is the presented user name, not the one that would result from a connection-time role change as described here - though this should be tested, and then documented.

Yes, this seems to be correct. I've added a note about this behavior
in the patch.

-- setup
CREATE ROLE test1;
CREATE ROLE test2 WITH LOGIN IN ROLE test1;
ALTER ROLE test1 SET client_min_messages = 'error';
ALTER ROLE test2 SET client_min_messages = 'warning';
ALTER ROLE test2 SET ROLE = 'test1';

-- as test2
postgres=> SELECT CURRENT_USER, setting FROM pg_settings WHERE name = 'client_min_messages';
current_user | setting
--------------+---------
test1 | warning
(1 row)

Nathan

I have had a look at the patch, and while I agree that this should
be documented, I am not happy with the patch as it is.

I think we should *not* document that under "server configuration".
This is confusing and will lead people to think that a role is
a configuration parameter. But you cannot add

role = myrole

to "postgresql.conf". A role is not a GUC.

I think that the place to document this is
doc/src/sgml/ref/alter_role.sgml.

The second hunk of the patch is in the right place:

--- a/doc/src/sgml/ref/set_role.sgml
+++ b/doc/src/sgml/ref/set_role.sgml
@@ -53,9 +53,13 @@ RESET ROLE
   </para>

   <para>
-   The <literal>NONE</literal> and <literal>RESET</literal> forms reset the current
-   user identifier to be the current session user identifier.
-   These forms can be executed by any user.
+   The <literal>NONE</literal> form resets the current user identifier to the
+   current session user identifier.  The <literal>RESET</literal> form resets
+   the current user identifier to the default value.  The default value can be
+   the command-line option value, the per-database default setting, or the
+   per-user default setting for the originally authenticated session user, if
+   any such settings exist.  Otherwise, the default value will be the current
+   session user identifier.  These forms can be executed by any user.
   </para>
  </refsect1>

Perhaps this could be reworded in a simpler fashion, like:

<literal>SET ROLE NONE</literal> sets the user identifier to the current
session identifier, as returned by the <function>session_user</function>
function. <literal>RESET ROLE</literal> sets the user identifier to the
value it had after you connected to the database. This can be different
from the session identifier if <literal>ALTER DATABASE</literal> or
<literal>ALTER ROLE</literal> were used to assign a different default role.

(I hope what I wrote is correct.)

Yours,
Laurenz Albe

On Thu, Mar 11, 2021 at 7:58 AM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:

I think we should *not* document that under "server configuration".
This is confusing and will lead people to think that a role is
a configuration parameter. But you cannot add

role = myrole

to "postgresql.conf". A role is not a GUC.

I think that the place to document this is
doc/src/sgml/ref/alter_role.sgml.

Good point. I agree that another syntax specification should be added to
ALTER ROLE/DATABASE cover this instead of shoe-horning it into the "SET
configuration_parameter" syntax specification even though the syntax is
nearly identical. It is indeed a different mechanic that just happens to
share a similar syntax. (On that note, does "FROM CURRENT" work with
"ROLE"?)

I'm a bit indifferent on the wording for RESET ROLE, though it should
probably mirror whatever wording we use for GUCs since this behaves like
one even if it isn't one technically.

David J.

Thanks for reviewing.

On 3/11/21, 6:59 AM, "Laurenz Albe" <laurenz.albe@cybertec.at> wrote:

I have had a look at the patch, and while I agree that this should
be documented, I am not happy with the patch as it is.

I think we should *not* document that under "server configuration".
This is confusing and will lead people to think that a role is
a configuration parameter. But you cannot add

role = myrole

to "postgresql.conf". A role is not a GUC.

I think that the place to document this is
doc/src/sgml/ref/alter_role.sgml.

I don't think I totally agree that "role" and "session_authorization"
aren't GUCs. They are defined in guc.c, and "role" is referred to as
a GUC in both miscinit.c and variable.c. Plus, they are usable as
configuration parameters in many of the same ways that ordinary GUCs
are (e.g., SET, ALTER ROLE, ALTER DATABASE). It is true that "role"
and "session_authorization" cannot be set in postgresql.conf and ALTER
SYSTEM SET, and I think we should add a note to this effect in the
documentation. However, I don't see the value in duplicating a
paragraph about "role" and "session_authorization" in a number of
statements that already accept a configuration_parameter and point to
the chapter on Server Configuration.

I do agree that that adding these parameters to the Server
Configuration section is a bit confusing. At the very least, the
proposed patch would add them to the Client Connection Defaults
section, but it's still not ideal. This is probably why they've been
left out so far.

The second hunk of the patch is in the right place:

--- a/doc/src/sgml/ref/set_role.sgml
+++ b/doc/src/sgml/ref/set_role.sgml
@@ -53,9 +53,13 @@ RESET ROLE
</para>

<para>
-   The <literal>NONE</literal> and <literal>RESET</literal> forms reset the current
-   user identifier to be the current session user identifier.
-   These forms can be executed by any user.
+   The <literal>NONE</literal> form resets the current user identifier to the
+   current session user identifier.  The <literal>RESET</literal> form resets
+   the current user identifier to the default value.  The default value can be
+   the command-line option value, the per-database default setting, or the
+   per-user default setting for the originally authenticated session user, if
+   any such settings exist.  Otherwise, the default value will be the current
+   session user identifier.  These forms can be executed by any user.
</para>
</refsect1>

Perhaps this could be reworded in a simpler fashion, like:

<literal>SET ROLE NONE</literal> sets the user identifier to the current
session identifier, as returned by the <function>session_user</function>
function. <literal>RESET ROLE</literal> sets the user identifier to the
value it had after you connected to the database. This can be different
from the session identifier if <literal>ALTER DATABASE</literal> or
<literal>ALTER ROLE</literal> were used to assign a different default role.

(I hope what I wrote is correct.)

I like the simpler text, but I think it is missing a couple of things.
If no session default was set, RESET ROLE will set the role to the
current session user identifier, which can either be what it was when
you first connected or what you SET SESSION AUTHORIZATION to. The
other thing missing is that "role" can be set via the command-line
options, too. Here's an attempt at including those things:

<literal>SET ROLE NONE</literal> sets the current user identifier to the
current session user identifier, as returned by
<function>session_user</function>. <literal>RESET ROLE</literal> sets the
current user identifier to the connection-time setting specified by the
<link linkend="libpq-connect-options">command-line options</link>,
<link linkend="sql-alterrole"><command>ALTER ROLE</command></link>, or
<link linkend="sql-alterdatabase"><command>ALTER DATABASE</command></link>,
if any such settings exist. Otherwise, <literal>RESET ROLE</literal> sets
the current user identifier to the current session user identifier. These
forms can be executed by any user.

I attached a new version of my proposed patch, but I acknowledge that
much of it is still under discussion.

Nathan

On Thursday, March 11, 2021, Bossart, Nathan <bossartn@amazon.com> wrote:

Thanks for reviewing.

On 3/11/21, 6:59 AM, "Laurenz Albe" <laurenz.albe@cybertec.at> wrote:

I have had a look at the patch, and while I agree that this should
be documented, I am not happy with the patch as it is.

I think we should *not* document that under "server configuration".
This is confusing and will lead people to think that a role is
a configuration parameter. But you cannot add

role = myrole

to "postgresql.conf". A role is not a GUC.

I think that the place to document this is
doc/src/sgml/ref/alter_role.sgml.

I don't think I totally agree that "role" and "session_authorization"
aren't GUCs. They are defined in guc.c, and "role" is referred to as
a GUC in both miscinit.c and variable.c.

Implementation details are not that convincing to me. As a user I wouldn’t
think of these as being “server configuration” or even “client defaults”;
typically they are just representations of me as session state.

The minor bit of documentation pseudo-redundancy doesn’t bother me if I
accept they are there own separate thing. The fact that set role and set
session authorization are entirely distinct top-level commands in our
documentation, as opposed to bundled in with plain set, is a much more
convincing example for treating them uniquely and not just additional GUCs.

David J.

On 3/11/21, 12:11 PM, "David G. Johnston" <david.g.johnston@gmail.com> wrote:

On Thursday, March 11, 2021, Bossart, Nathan <bossartn@amazon.com> wrote:

Thanks for reviewing.

On 3/11/21, 6:59 AM, "Laurenz Albe" <laurenz.albe@cybertec.at> wrote:

I have had a look at the patch, and while I agree that this should
be documented, I am not happy with the patch as it is.

I think we should *not* document that under "server configuration".
This is confusing and will lead people to think that a role is
a configuration parameter. But you cannot add

role = myrole

to "postgresql.conf". A role is not a GUC.

I think that the place to document this is
doc/src/sgml/ref/alter_role.sgml.

I don't think I totally agree that "role" and "session_authorization"
aren't GUCs. They are defined in guc.c, and "role" is referred to as
a GUC in both miscinit.c and variable.c.

Implementation details are not that convincing to me. As a user I wouldn’t think of these as being “server configuration” or even “client defaults”; typically they are just representations of me as session state.

The minor bit of documentation pseudo-redundancy doesn’t bother me if I accept they are there own separate thing. The fact that set role and set session authorization are entirely distinct top-level commands in our documentation, as opposed to bundled in with plain set, is a much more convincing example for treating them uniquely and not just additional GUCs.

I see your point. What do you think about something like the attached
patch?

Nathan

On Thu, 2021-03-11 at 22:30 +0000, Bossart, Nathan wrote:

On 3/11/21, 12:11 PM, "David G. Johnston" <david.g.johnston@gmail.com> wrote:

The minor bit of documentation pseudo-redundancy doesn’t bother me if I accept
they are there own separate thing. The fact that set role and set session
authorization are entirely distinct top-level commands in our documentation,
as opposed to bundled in with plain set, is a much more convincing example
for treating them uniquely and not just additional GUCs.

I see your point. What do you think about something like the attached
patch?

After sleeping on it, I have come to think that it is excessive to write
so much documentation for a feature that is that unimportant.

It takes some effort to come up with a good use case for it.

I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
(although I don't see what sense it could make to set that on the database level),
and briefly explain the difference between RESET ROLE and SET ROLE NONE.

I think adding too much detail will harm - anyone who needs to know the
exact truth can resort to the implementation.

I'll try to come up with a proposal later.

Yours,
Laurenz Albe

On Fri, 2021-03-12 at 10:16 +0100, I wrote:

After sleeping on it, I have come to think that it is excessive to write
so much documentation for a feature that is that unimportant.

It takes some effort to come up with a good use case for it.

I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
(although I don't see what sense it could make to set that on the database level),
and briefly explain the difference between RESET ROLE and SET ROLE NONE.

I think adding too much detail will harm - anyone who needs to know the
exact truth can resort to the implementation.

I'll try to come up with a proposal later.

Attached is my idea of the documentation change.

I think that ALTER DATABASE ... SET ROLE can remain undocumented, because
I cannot imagine that it could be useful.

I am unsure if specifying "role" in a libpq connect string might be
worth documenting. Can you think of a use case?

Yours,
Laurenz Albe

On Fri, Mar 12, 2021 at 7:35 AM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:

On Fri, 2021-03-12 at 10:16 +0100, I wrote:

After sleeping on it, I have come to think that it is excessive to write
so much documentation for a feature that is that unimportant.

It takes some effort to come up with a good use case for it.

I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
(although I don't see what sense it could make to set that on the

database level),

and briefly explain the difference between RESET ROLE and SET ROLE NONE.

I think adding too much detail will harm - anyone who needs to know the
exact truth can resort to the implementation.

I'll try to come up with a proposal later.

Attached is my idea of the documentation change.

I think that ALTER DATABASE ... SET ROLE can remain undocumented, because
I cannot imagine that it could be useful.

I am unsure if specifying "role" in a libpq connect string might be
worth documenting. Can you think of a use case?

Does our imagination really matter here? It works and is just as "useful"
as "ALTER ROLE" and so should be documented if we document ALTER ROLE.

I agree that ALTER DATABASE seems entirely useless and even
counter-productive...but I would still document if only because we document
ALTER ROLE and they should be kept similar.

Haven't formed an opinion on the merits of the two patches.

David J.

On 3/12/21, 6:35 AM, "Laurenz Albe" <laurenz.albe@cybertec.at> wrote:

On Fri, 2021-03-12 at 10:16 +0100, I wrote:

After sleeping on it, I have come to think that it is excessive to write
so much documentation for a feature that is that unimportant.

It takes some effort to come up with a good use case for it.

I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
(although I don't see what sense it could make to set that on the database level),
and briefly explain the difference between RESET ROLE and SET ROLE NONE.

I think adding too much detail will harm - anyone who needs to know the
exact truth can resort to the implementation.

I'll try to come up with a proposal later.

Attached is my idea of the documentation change.

I think that ALTER DATABASE ... SET ROLE can remain undocumented, because
I cannot imagine that it could be useful.

I am unsure if specifying "role" in a libpq connect string might be
worth documenting. Can you think of a use case?

My main goal of this thread is to get the RESET ROLE documentation
fixed. I don't have a terribly strong opinion on documenting these
special uses of "role". I lean in favor of adding it, but I wouldn't
be strongly opposed to simply leaving it out for now. But if we're
going to add it, I think we might as well add it everywhere.

Nathan

On 3/12/21 1:16 PM, Bossart, Nathan wrote:

On 3/12/21, 6:35 AM, "Laurenz Albe" <laurenz.albe@cybertec.at> wrote:

On Fri, 2021-03-12 at 10:16 +0100, I wrote:

After sleeping on it, I have come to think that it is excessive to write
so much documentation for a feature that is that unimportant.

It takes some effort to come up with a good use case for it.

I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
(although I don't see what sense it could make to set that on the database level),
and briefly explain the difference between RESET ROLE and SET ROLE NONE.

I think adding too much detail will harm - anyone who needs to know the
exact truth can resort to the implementation.

I'll try to come up with a proposal later.

Attached is my idea of the documentation change.

I think that ALTER DATABASE ... SET ROLE can remain undocumented, because
I cannot imagine that it could be useful.

I am unsure if specifying "role" in a libpq connect string might be
worth documenting. Can you think of a use case?

My main goal of this thread is to get the RESET ROLE documentation
fixed. I don't have a terribly strong opinion on documenting these
special uses of "role". I lean in favor of adding it, but I wouldn't
be strongly opposed to simply leaving it out for now. But if we're
going to add it, I think we might as well add it everywhere.

Looking back at the commit history it seems to me that this only works
accidentally. Perhaps it would be best to fix RESET ROLE and be done with it.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 3/12/21, 11:14 AM, "Joe Conway" <mail@joeconway.com> wrote:

On 3/12/21 1:16 PM, Bossart, Nathan wrote:

My main goal of this thread is to get the RESET ROLE documentation
fixed. I don't have a terribly strong opinion on documenting these
special uses of "role". I lean in favor of adding it, but I wouldn't
be strongly opposed to simply leaving it out for now. But if we're
going to add it, I think we might as well add it everywhere.

Looking back at the commit history it seems to me that this only works
accidentally. Perhaps it would be best to fix RESET ROLE and be done with it.

That seems reasonable to me.

Nathan

On Fri, 2021-03-12 at 21:41 +0000, Bossart, Nathan wrote:

On 3/12/21, 11:14 AM, "Joe Conway" <mail@joeconway.com> wrote:

Looking back at the commit history it seems to me that this only works
accidentally. Perhaps it would be best to fix RESET ROLE and be done with it.

That seems reasonable to me.

+1 from me too.

Yours,
Laurenz Albe