Обсуждение: psql: Add role's membership options to the \du+ command
When you include one role in another, you can specify three options: ADMIN, INHERIT (added in e3ce2de0) and SET (3d14e171). For example. CREATE ROLE alice LOGIN; GRANT pg_read_all_settings TO alice WITH ADMIN TRUE, INHERIT TRUE, SET TRUE; GRANT pg_stat_scan_tables TO alice WITH ADMIN FALSE, INHERIT FALSE, SET FALSE; GRANT pg_read_all_stats TO alice WITH ADMIN FALSE, INHERIT TRUE, SET FALSE; For information about the options, you need to look in the pg_auth_members: SELECT roleid::regrole, admin_option, inherit_option, set_option FROM pg_auth_members WHERE member = 'alice'::regrole; roleid | admin_option | inherit_option | set_option ----------------------+--------------+----------------+------------ pg_read_all_settings | t | t | t pg_stat_scan_tables | f | f | f pg_read_all_stats | f | t | f (3 rows) I think it would be useful to be able to get this information with a psql command like \du (and \dg). With proposed patch the \du command still only lists the roles of which alice is a member: \du alice List of roles Role name | Attributes | Member of -----------+------------+-------------------------------------------------------------- alice | | {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables} But the \du+ command adds information about the selected ADMIN, INHERIT and SET options: \du+ alice List of roles Role name | Attributes | Member of | Description -----------+------------+-----------------------------------------------+------------- alice | | pg_read_all_settings WITH ADMIN, INHERIT, SET+| | | pg_read_all_stats WITH INHERIT +| | | pg_stat_scan_tables | One more change. The roles in the "Member of" column are sorted for both \du+ and \du for consistent output. Any comments are welcome. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Вложения
Added the patch to the open commitfest: https://commitfest.postgresql.org/42/4116/ Feel free to reject if it's not interesting.
-- Pavel Luzanov
When you include one role in another, you can specify three options:
ADMIN, INHERIT (added in e3ce2de0) and SET (3d14e171).
For example.
CREATE ROLE alice LOGIN;
GRANT pg_read_all_settings TO alice WITH ADMIN TRUE, INHERIT TRUE, SET TRUE;
GRANT pg_stat_scan_tables TO alice WITH ADMIN FALSE, INHERIT FALSE, SET
FALSE;
GRANT pg_read_all_stats TO alice WITH ADMIN FALSE, INHERIT TRUE, SET FALSE;
For information about the options, you need to look in the pg_auth_members:
SELECT roleid::regrole, admin_option, inherit_option, set_option
FROM pg_auth_members
WHERE member = 'alice'::regrole;
roleid | admin_option | inherit_option | set_option
----------------------+--------------+----------------+------------
pg_read_all_settings | t | t | t
pg_stat_scan_tables | f | f | f
pg_read_all_stats | f | t | f
(3 rows)
I think it would be useful to be able to get this information with a
psql command
like \du (and \dg). With proposed patch the \du command still only lists
the roles of which alice is a member:
\du alice
List of roles
Role name | Attributes | Member of
-----------+------------+--------------------------------------------------------------
alice | |
{pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}
But the \du+ command adds information about the selected ADMIN, INHERIT
and SET options:
\du+ alice
List of roles
Role name | Attributes | Member of
| Description
-----------+------------+-----------------------------------------------+-------------
alice | | pg_read_all_settings WITH ADMIN, INHERIT, SET+|
| | pg_read_all_stats WITH INHERIT +|
| | pg_stat_scan_tables |
One more change. The roles in the "Member of" column are sorted for both
\du+ and \du for consistent output.
Any comments are welcome.
List of roles
Role name | Attributes | Member of | Members | Description
----------------------+--------------+-----------+-------------
pg_read_all_settings | Cannot login | {} | { pg_monitor } |
Yeah, I noticed the lack too, then went a bit too far afield with trying to compose a graph of the roles. I'm still working on that but at this point it probably won't be something I try to get committed to psql. Something more limited like this does need to be included.
Glad to hear that you're working on it.
I'm not too keen on the idea of converting the existing array into a newline separated string. I would try hard to make the modification here purely additional.
I agree with all of your arguments. A couple of months I tried to find an acceptable variant in the background.
But apparently tried not very hard ))
In the end, the variant proposed in the patch seemed to me worthy to show and
start a discussion. But I'm not sure that this is the best choice.
Another thing I did with the graph was have both "member" and "memberof" columns in the output. In short, every grant row in pg_auth_members appears twice, once in each column, so the role being granted membership and the role into which membership is granted both have visibility when you filter on them. For the role graph I took this idea and extended out to an entire chain of roles (and also broke out user and group separately) but I think doing the direct-grant only here would still be a big improvement.
It will be interesting to see the result.
-- Pavel Luzanov
Thanks a lot for the improvement, and it will definitely help provide more very useful information. I noticed the document psql-ref.sgml has been updated for both `du+` and `dg+`, but only `du` and `\du+` are covered in regression test. Is that because `dg+` is treated exactly the same as `du+` from testing point of view? The reason I am asking this question is that I notice that `pg_monitor` also has the detailed information, so not sure if more test cases required. postgres=# \duS+ List of roles Role name | Attributes | Member of | Description -----------------------------+------------------------------------------------------------+-----------------------------------------------+------------- alice | | pg_read_all_settings WITH ADMIN, INHERIT, SET | pg_checkpoint | Cannot login | | pg_database_owner | Cannot login | | pg_execute_server_program | Cannot login | | pg_maintain | Cannot login | | pg_monitor | Cannot login | pg_read_all_settings WITH INHERIT, SET +| | | pg_read_all_stats WITH INHERIT, SET +| | | pg_stat_scan_tables WITH INHERIT, SET | Best regards, David On 2023-01-09 8:09 a.m., Pavel Luzanov wrote: > When you include one role in another, you can specify three options: > ADMIN, INHERIT (added in e3ce2de0) and SET (3d14e171). > > For example. > > CREATE ROLE alice LOGIN; > > GRANT pg_read_all_settings TO alice WITH ADMIN TRUE, INHERIT TRUE, SET > TRUE; > GRANT pg_stat_scan_tables TO alice WITH ADMIN FALSE, INHERIT FALSE, > SET FALSE; > GRANT pg_read_all_stats TO alice WITH ADMIN FALSE, INHERIT TRUE, SET > FALSE; > > For information about the options, you need to look in the > pg_auth_members: > > SELECT roleid::regrole, admin_option, inherit_option, set_option > FROM pg_auth_members > WHERE member = 'alice'::regrole; > roleid | admin_option | inherit_option | set_option > ----------------------+--------------+----------------+------------ > pg_read_all_settings | t | t | t > pg_stat_scan_tables | f | f | f > pg_read_all_stats | f | t | f > (3 rows) > > I think it would be useful to be able to get this information with a > psql command > like \du (and \dg). With proposed patch the \du command still only lists > the roles of which alice is a member: > > \du alice > List of roles > Role name | Attributes | Member of > -----------+------------+-------------------------------------------------------------- > > alice | | > {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables} > > But the \du+ command adds information about the selected ADMIN, INHERIT > and SET options: > > \du+ alice > List of roles > Role name | Attributes | Member > of | Description > -----------+------------+-----------------------------------------------+------------- > > alice | | pg_read_all_settings WITH ADMIN, INHERIT, SET+| > | | pg_read_all_stats WITH INHERIT +| > | | pg_stat_scan_tables | > > One more change. The roles in the "Member of" column are sorted for both > \du+ and \du for consistent output. > > Any comments are welcome. >
I noticed the document psql-ref.sgml has been updated for both `du+` and
`dg+`, but only `du` and `\du+` are covered in regression test. Is that
because `dg+` is treated exactly the same as `du+` from testing point of
view?
The reason I am asking this question is that I notice that `pg_monitor`
also has the detailed information, so not sure if more test cases required.
On Fri, Feb 10, 2023 at 2:08 PM David Zhang <david.zhang@highgo.ca> wrote:
I noticed the document psql-ref.sgml has been updated for both `du+` and
`dg+`, but only `du` and `\du+` are covered in regression test. Is that
because `dg+` is treated exactly the same as `du+` from testing point of
view?Yes.
The reason I am asking this question is that I notice that `pg_monitor`
also has the detailed information, so not sure if more test cases required.Of course it does. Why does that bother you? And what does that have to do with the previous paragraph?
There is a default built-in role `pg_monitor` and the behavior changed after the patch. If `\dg+` and `\du+` is treated as the same, and `make check` all pass, then I assume there is no test case to verify the output of `duS+`. My point is should we consider add a test case?
Before patch the output for `pg_monitor`,
postgres=# \duS+
List of roles
Role name | Attributes | Member of | Description
-----------------------------+------------------------------------------------------------+--------------------------------------------------------------+-------------
alice | | {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables} |
pg_checkpoint | Cannot login | {} |
pg_database_owner | Cannot login | {} |
pg_execute_server_program | Cannot login | {} |
pg_maintain | Cannot login | {} |
pg_monitor | Cannot login | {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables} |
pg_read_all_data | Cannot login | {} |
pg_read_all_settings | Cannot login | {} |
pg_read_all_stats | Cannot login | {} |
pg_read_server_files | Cannot login | {} |
pg_signal_backend | Cannot login | {} |
pg_stat_scan_tables | Cannot login | {} |
pg_use_reserved_connections | Cannot login | {} |
pg_write_all_data | Cannot login | {} |
pg_write_server_files | Cannot login | {} |
ubuntu | Superuser, Create role, Create DB, Replication, Bypass RLS | {} |
After patch the output for `pg_monitor`,
postgres=# \duS+
List of roles
Role name | Attributes | Member of | Description
-----------------------------+------------------------------------------------------------+-----------------------------------------------+-------------
alice | | pg_read_all_settings WITH ADMIN, INHERIT, SET+|
| | pg_read_all_stats WITH INHERIT +|
| | pg_stat_scan_tables |
pg_checkpoint | Cannot login | |
pg_database_owner | Cannot login | |
pg_execute_server_program | Cannot login | |
pg_maintain | Cannot login | |
pg_monitor | Cannot login | pg_read_all_settings WITH INHERIT, SET +|
| | pg_read_all_stats WITH INHERIT, SET +|
| | pg_stat_scan_tables WITH INHERIT, SET |
pg_read_all_data | Cannot login | |
pg_read_all_settings | Cannot login | |
pg_read_all_stats | Cannot login | |
pg_read_server_files | Cannot login | |
pg_signal_backend | Cannot login | |
pg_stat_scan_tables | Cannot login | |
pg_use_reserved_connections | Cannot login | |
pg_write_all_data | Cannot login | |
pg_write_server_files | Cannot login | |
ubuntu | Superuser, Create role, Create DB, Replication, Bypass RLS | |
Best regards,
David
David J.
There is a default built-in role `pg_monitor` and the behavior changed after the patch. If `\dg+` and `\du+` is treated as the same, and `make check` all pass, then I assume there is no test case to verify the output of `duS+`. My point is should we consider add a test case?
On 2023-02-15 1:37 p.m., David G. Johnston wrote:
Good improvement, +1.On Wed, Feb 15, 2023 at 2:31 PM David Zhang <david.zhang@highgo.ca> wrote:There is a default built-in role `pg_monitor` and the behavior changed after the patch. If `\dg+` and `\du+` is treated as the same, and `make check` all pass, then I assume there is no test case to verify the output of `duS+`. My point is should we consider add a test case?I mean, either you accept the change in how this meta-command presents its information or you don't. I don't see how a test case is particularly beneficial. Or, at least the pg_monitor role is not special in this regard. Alice changed too and you don't seem to be including it in your complaint.
I mean, either you accept the change in how this meta-command presents its information or you don't.
Yes, that's the main issue of this patch.
On the one hand, it would be nice to see the membership options with the psql command.
On the other hand, I don't have an exact understanding of how best to do it. That's why I proposed a variant for discussion. It is quite possible that if there is no consensus, it would be better to leave it as is, and get information by queries to the system catalog.
----- Pavel Luzanov
On the one hand, it would be nice to see the membership options with the psql command.
After playing with cf5eb37c and e5b8a4c0 I think something must be made with \du command.postgres@demo(16.0)=# CREATE ROLE admin LOGIN CREATEROLE;
CREATE ROLE
postgres@demo(16.0)=# \c - admin
You are now connected to database "demo" as user "admin".
admin@demo(16.0)=> SET createrole_self_grant = 'SET, INHERIT';
SET
admin@demo(16.0)=> CREATE ROLE bob LOGIN;
CREATE ROLE
admin@demo(16.0)=> \du
List of roles
Role name | Attributes | Member of
-----------+------------------------------------------------------------+-----------
admin | Create role | {bob,bob}
bob | | {}
postgres | Superuser, Create role, Create DB, Replication, Bypass RLS | {}
We see two bob roles in the 'Member of' column. Strange? But this is correct.
admin@demo(16.0)=> select roleid::regrole, member::regrole, * from pg_auth_members where roleid = 'bob'::regrole;
roleid | member | oid | roleid | member | grantor | admin_option | inherit_option | set_option
--------+--------+-------+--------+--------+---------+--------------+----------------+------------
bob | admin | 16713 | 16712 | 16711 | 10 | t | f | f
bob | admin | 16714 | 16712 | 16711 | 16711 | f | t | t
(2 rows)
First 'grant bob to admin' command issued immediately after creating role bob by superuser(grantor=10). Second command issues by admin role and set membership options SET and INHERIT.If we don't ready to display membership options with \du+ may be at least we must group records in 'Member of' column for \du command?
----- Pavel Luzanov
List of roles
Role name | Attributes | Member of
-----------+------------------------------------------------------------+-----------
admin | Create role | {bob,bob}
bob | | {}
postgres | Superuser, Create role, Create DB, Replication, Bypass RLS | {}First 'grant bob to admin' command issued immediately after creating role bob by superuser(grantor=10). Second command issues by admin role and set membership options SET and INHERIT.If we don't ready to display membership options with \du+ may be at least we must group records in 'Member of' column for \du command?
On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:List of roles
Role name | Attributes | Member of
-----------+------------------------------------------------------------+-----------
admin | Create role | {bob,bob}
bob | | {}
postgres | Superuser, Create role, Create DB, Replication, Bypass RLS | {}First 'grant bob to admin' command issued immediately after creating role bob by superuser(grantor=10). Second command issues by admin role and set membership options SET and INHERIT.If we don't ready to display membership options with \du+ may be at least we must group records in 'Member of' column for \du command?
I agree that these views should GROUP BY roleid and use bool_or(*_option) to produce their result.
Ok, I'll try in the next few days. But what presentation format to use?
1. bob(admin_option=t inherit_option=t set_option=f) -- it seems very long
2. bob(ai) -- short, but will it be clear?
3. something else?
Their purpose is to communicate the current effective state to the user, not facilitate full inspection of the configuration, possibly to aid in issuing GRANT and REVOKE commands.
This can help in issuing GRANT command, but not REVOKE. Revoking a role's membership is now very similar to revoking privileges. Only the role that granted membership can revoke that membership. So for REVOKE you need to know who granted membership, but this information will not be available after grouping.
One thing I found, and I plan to bring this up independently once I've collected my thoughts, is that pg_has_role() uses the terminology "USAGE" and "MEMBER" for "INHERIT" and "SET" respectively.It's annoying that "member" has been overloaded here. And the choice of USAGE just seems arbitrary (though I haven't researched it) given the related syntax.
I didn't even know this function existed. But I see that it was changed in 3d14e171 with updated documentation:
https://www.postgresql.org/docs/devel/functions-info.html#FUNCTIONS-INFO-ACCESS
Maybe that's enough.
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
On 17.02.2023 19:53, David G. Johnston wrote:On Fri, Feb 17, 2023 at 4:02 AM Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:List of roles
Role name | Attributes | Member of
-----------+------------------------------------------------------------+-----------
admin | Create role | {bob,bob}
bob | | {}
postgres | Superuser, Create role, Create DB, Replication, Bypass RLS | {}First 'grant bob to admin' command issued immediately after creating role bob by superuser(grantor=10). Second command issues by admin role and set membership options SET and INHERIT.If we don't ready to display membership options with \du+ may be at least we must group records in 'Member of' column for \du command?
I agree that these views should GROUP BY roleid and use bool_or(*_option) to produce their result.
Ok, I'll try in the next few days. But what presentation format to use?
I didn't even know this function existed. But I see that it was changed in 3d14e171 with updated documentation:
https://www.postgresql.org/docs/devel/functions-info.html#FUNCTIONS-INFO-ACCESS
Maybe that's enough.
This is the format I've gone for (more-or-less) in my RoleGraph view (I'll be sharing it publicly in the near future).bob from grantor (a, s, i) \nadam from postgres (a, s, i) \nemily from postgres (empty)
I think this is a good compromise.
Based upon prior comments going for something like the following is undesirable: bob=asi/grantor
Agree. Membership options are not the ACL (although they have similarities). Therefore, showing them as a ACL-like column will be confusing.
So, please find attached the second version of the patch. It implements suggested display format and small refactoring of existing code for \du command.
As a non-native writer, I have doubts about the documentation part.
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
Вложения
Next version (v3) addresses complains from cfbot. Changed only tests. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Вложения
I didn't even know this function existed. But I see that it was changed in 3d14e171 with updated documentation:I think that should probably have ADMIN as one of the options as well. Also curious what it reports for an empty membership.
I've been experimenting for a few days and I want to admit that this is a very difficult and not obvious topic.
I'll try to summarize what I think.
1.
About ADMIN value for pg_has_role.
Implementation of ADMIN value will be different from USAGE and SET.
To be True, USAGE value requires the full chain of memberships to have INHERIT option.
Similar with SET: the full chain of memberships must have SET option.
But for ADMIN, only last member in the chain must have ADMIN option and all previous members
must have INHERIT (to administer directly) or SET option (to switch to role, last in the chain).
Therefore, it is not obvious to me that the function needs the ADMIN value.
2.
pg_has_role function description starts with: Does user have privilege for role?
- This is not exact: function works not only with users, but with NOLOGIN roles too.
- Term "privilege": this term used for ACL columns, such usage may be confusing,
especially after adding INHERIT and SET in addition to ADMIN option.
3.
It is possible to grant membership with all three options turned off:
grant a to b with admin false, inherit false, set false;
But such membership is completely useless (if i didn't miss something).
May be such grants must be prohibited. At least this may be documented in the GRANT command.
4.
Since v16 it is possible to grant membership from one role to another several times with different grantors.
And only grantor can revoke membership.
- This is not documented anywhere.
- Current behavior of \du command with duplicated roles in "Member of" column strongly confusing.
This is one of the goals of the discussion patch.
I think to write about this to pgsql-docs additionally to this topic.
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
Hello,On 22.02.2023 00:34, David G. Johnston wrote:I didn't even know this function existed. But I see that it was changed in 3d14e171 with updated documentation:I think that should probably have ADMIN as one of the options as well. Also curious what it reports for an empty membership.
I've been experimenting for a few days and I want to admit that this is a very difficult and not obvious topic.
I'll try to summarize what I think.
1.
About ADMIN value for pg_has_role.
Implementation of ADMIN value will be different from USAGE and SET.
To be True, USAGE value requires the full chain of memberships to have INHERIT option.
Similar with SET: the full chain of memberships must have SET option.
But for ADMIN, only last member in the chain must have ADMIN option and all previous members
must have INHERIT (to administer directly) or SET option (to switch to role, last in the chain).
Therefore, it is not obvious to me that the function needs the ADMIN value.
2.
pg_has_role function description starts with: Does user have privilege for role?
- This is not exact: function works not only with users, but with NOLOGIN roles too.
- Term "privilege": this term used for ACL columns, such usage may be confusing,
especially after adding INHERIT and SET in addition to ADMIN option.
3.
It is possible to grant membership with all three options turned off:
grant a to b with admin false, inherit false, set false;
But such membership is completely useless (if i didn't miss something).
May be such grants must be prohibited. At least this may be documented in the GRANT command.
4.
Since v16 it is possible to grant membership from one role to another several times with different grantors.
And only grantor can revoke membership.
- This is not documented anywhere.
- Current behavior of \du command with duplicated roles in "Member of" column strongly confusing.
This is one of the goals of the discussion patch.
I think to write about this to pgsql-docs additionally to this topic.
I'd be fine with "pg_can_admin_role" being a newly created function that provides this true/false answer but it seems indisputable that today there is no core-provided means to answer the question "can one role get ADMIN rights on another role". Modifying \du to show this seems out-of-scope but the pg_has_role function already provides that question for INHERIT and SET so it is at least plausible to extend it to include ADMIN, even if the phrase "has role" seems a bit of a misnomer. I do cover this aspect with the Role Graph pseudo-extension but given the presence and ease-of-use of a boolean-returning function this seems like a natural addition. We've also survived quite long without it - this isn't a new concept in v16, just a bit refined.
I must admit that I am slowly coming to the same conclusions that you have already outlined in previous messages.
Indeed, adding ADMIN to pg_has_role looks logical. The function will show whether one role can manage another directly or indirectly (via SET ROLE).
Adding ADMIN will lead to the question of naming other values. It is more reasonable to have INHERIT instead of USAGE.
And it is not very clear whether (except for backward compatibility) a separate MEMBER value is needed at all.
I wouldn't bother starting yet another thread in this area right now, this one can absorb some related changes as well as the subject line item.
I agree.
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
Indeed, adding ADMIN to pg_has_role looks logical. The function will show whether one role can manage another directly or indirectly (via SET ROLE).
Adding ADMIN will lead to the question of naming other values. It is more reasonable to have INHERIT instead of USAGE.
And it is not very clear whether (except for backward compatibility) a separate MEMBER value is needed at all.
I'll be looking over your v3 patch sometime this week, if not today.
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1727,15 +1727,18 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
<literal>S</literal> modifier to include system roles.
If <replaceable class="parameter">pattern</replaceable> is specified,
only those roles whose names match the pattern are listed.
- For each membership in the role, the membership options and
- the role that granted the membership are displayed.
- Оne-letter abbreviations are used for membership options:
- <literal>a</literal> — admin option, <literal>i</literal> — inherit option,
- <literal>s</literal> — set option and <literal>empty</literal> if no one is set.
- See <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
- If the form <literal>\dg+</literal> is used, additional information
- is shown about each role; currently this adds the comment for each
- role.
+ </para>
+ <para>
+ Shown within each row, in newline-separated format, are the memberships granted to
+ the role. The presentation includes both the name of the grantor
+ as well as the membership permissions (in an abbreviated format:
+ <literal>a</literal> for admin option, <literal>i</literal> for inherit option,
+ <literal>s</literal> for set option.) The word <literal>empty</literal> is printed in
+ the case that none of those permissions are granted.
+ See the <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
+ </para>
+ <para>
+ If the form <literal>\dg+</literal> is used the comment attached to the role is shown.
</para>
</listitem>
</varlistentry>
Moving the goal posts for this meta-command to >= 9.5 seems like it should be done as a separate patch and thread. The documentation presently states we are targeting 9.2 and newer.
I missed the comment at the beginning of the file about version 9.2. I will return the version check for rolbypassrls.
My suggestion for the docs is below.
+ <para>
+ Shown within each row, in newline-separated format, are the memberships granted to
+ the role. The presentation includes both the name of the grantor
+ as well as the membership permissions (in an abbreviated format:
+ <literal>a</literal> for admin option, <literal>i</literal> for inherit option,
+ <literal>s</literal> for set option.) The word <literal>empty</literal> is printed in
+ the case that none of those permissions are granted.
+ See the <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
+ </para>
+ <para>
+ If the form <literal>\dg+</literal> is used the comment attached to the role is shown.
</para>
Thanks. I will replace the description with this one.
I would suggest tweaking the test output to include regress_du_admin and also to make regress_du_admin a CREATEROLE role with LOGIN.
Ok.
Thank you for review. I will definitely work on the new version, but unfortunately and with a high probability it will happen after March 20.
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
FWIW I've finally gotten to publishing my beta version of the Role Graph for PostgreSQL pseudo-extension I'd been talking about:
Great. So far I've looked very briefly, but it's interesting.
I handle EMPTY explicitly in the Role Graph but as I noted somewhere in my comments, it really shouldn't be possible to leave the database in that state. Do we need to bug Robert on this directly or do you plan to have a go at it?
I don't plan to do that. Right now I don't have enough time and experience. This requires an experienced developer.
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
I missed the comment at the beginning of the file about version 9.2. I will return the version check for rolbypassrls.
+ <para>
+ Shown within each row, in newline-separated format, are the memberships granted to
+ the role. The presentation includes both the name of the grantor
+ as well as the membership permissions (in an abbreviated format:
+ <literal>a</literal> for admin option, <literal>i</literal> for inherit option,
+ <literal>s</literal> for set option.) The word <literal>empty</literal> is printed in
+ the case that none of those permissions are granted.
+ See the <link linkend="sql-grant"><command>GRANT</command></link> command for their meaning.
+ </para>
+ <para>
+ If the form <literal>\dg+</literal> is used the comment attached to the role is shown.
</para>
Thanks. I will replace the description with this one.
I would suggest tweaking the test output to include regress_du_admin and also to make regress_du_admin a CREATEROLE role with LOGIN.
Ok.
Please review the attached version 4 with the changes discussed.
----- Pavel Luzanov
Вложения
I would suggest tweaking the test output to include regress_du_admin ...
I found (with a help of cfbot) difficulty with this. The problem is the bootstrap superuser name (oid=10). This name depends on the OS username. In my case it's pal, but in most cases it's postgres or something else. And the output of \du regress_du_admin can't be predicted: \du regress_du_admin List of roles Role name | Attributes | Member of ------------------+-------------+------------------------------------- regress_du_admin | Create role | regress_du_role0 from pal (a, i, s)+ | | regress_du_role1 from pal (a, i, s)+ | | regress_du_role2 from pal (a, i, s) So, I decided not to include regress_du_admin in the test output. Please, see version 5 attached. Only tests changed. ----- Pavel Luzanov
Вложения
In the previous version, I didn't notice (unlike cfbot) the compiler warning. Fixed in version 6. ----- Pavel Luzanov
Вложения
In the previous version, I didn't notice (unlike cfbot) the compiler
warning. Fixed in version 6.
"David G. Johnston" <david.g.johnston@gmail.com> writes: > I've marked this Ready for Committer. Hmm ... not sure I like the proposed output. The 'a', 'i', 's' annotations are short but they don't have much else to recommend them. On the other hand, there's nearby precedent for single-letter abbreviations in ACL displays. Nobody particularly likes those, though. Also, if we're modeling this on ACLs then the display could be further shortened to "(ais)" or the like. Also, the patch is ignoring i18n issues. I suppose if we stick with said single-letter abbreviations we'd not translate them, but the construction "rolename from rolename" ought to be translatable IMO. Perhaps it'd be enough to allow replacement of "from", but I wonder if the phrase order would need to be different in some languages? regards, tom lane
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> I've marked this Ready for Committer.
Hmm ... not sure I like the proposed output. The 'a', 'i', 's'
annotations are short but they don't have much else to recommend them.
On the other hand, there's nearby precedent for single-letter
abbreviations in ACL displays. Nobody particularly likes those,
though. Also, if we're modeling this on ACLs then the display
could be further shortened to "(ais)" or the like.
Also, the patch is ignoring i18n issues.
I suppose if we stick with
said single-letter abbreviations we'd not translate them,
but the
construction "rolename from rolename" ought to be translatable IMO.
Perhaps it'd be enough to allow replacement of "from", but I wonder
if the phrase order would need to be different in some languages?
On Tue, Apr 4, 2023 at 12:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Hmm ... not sure I like the proposed output. The 'a', 'i', 's' > annotations are short but they don't have much else to recommend them. Yeah, I don't like that, either. I'm not sure what the right thing to do is here. It's a problem to have new information in the catalogs that you can't view via \d<whatever>. But displaying that information as a string of characters that will be gibberish to anyone but an expert doesn't necessarily seem like it really solves the problem. However, if we spell out the words, then it gets bulky. But maybe bulky is better than incomprehensible. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > I'm not sure what the right thing to do is here. It's a problem to > have new information in the catalogs that you can't view via > \d<whatever>. But displaying that information as a string of > characters that will be gibberish to anyone but an expert doesn't > necessarily seem like it really solves the problem. However, if we > spell out the words, then it gets bulky. But maybe bulky is better > than incomprehensible. The existing precedent in \du definitely leans towards "bulky": regression=# \du List of roles Role name | Attributes | Member of -----------+------------------------------------------------------------+----------- alice | Cannot login | {bob} bob | Cannot login | {} postgres | Superuser, Create role, Create DB, Replication, Bypass RLS | {} It seems pretty inconsistent to me to treat the role attributes this way and then economize in the adjacent column. Another advantage to spelling out the SQL keywords is that it removes the question of whether we should translate them. I wonder if, while we're here, we should apply the idea of joining-with-newlines-not-commas to the attributes column too. That's another source of inconsistency in the proposed display. regards, tom lane
On Tue, Apr 4, 2023 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wonder if, while we're here, we should apply the idea of > joining-with-newlines-not-commas to the attributes column too. > That's another source of inconsistency in the proposed display. That would make the column narrower, which might be good, because it seems to me that listing the memberships could require quite a lot of space, both vertical and horizontal. There can be any number of memberships, and each of those memberships has a grantor and three flag bits (INHERIT, SET, ADMIN). If some user with a long username has been granted membership with all three of those flags by a grantor who also has a long username, and if we show all that information, we're going to use up a lot of horizontal space. And if there are many such grants, also a lot of vertical space. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, Apr 4, 2023 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I wonder if, while we're here, we should apply the idea of >> joining-with-newlines-not-commas to the attributes column too. > That would make the column narrower, which might be good, because it > seems to me that listing the memberships could require quite a lot of > space, both vertical and horizontal. Right, that's what I was thinking. > There can be any number of memberships, and each of those memberships > has a grantor and three flag bits (INHERIT, SET, ADMIN). If some user > with a long username has been granted membership with all three of > those flags by a grantor who also has a long username, and if we show > all that information, we're going to use up a lot of horizontal space. > And if there are many such grants, also a lot of vertical space. Yup --- and if you were incautious enough to not exclude the bootstrap superuser, then you'll also have a very wide Attributes column. We can buy back some of that by joining the attributes with newlines. At some point people are going to have to resort to \x mode for this display, but we should do what we can to put that off as long as we're not sacrificing readability. regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 4, 2023 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wonder if, while we're here, we should apply the idea of
>> joining-with-newlines-not-commas to the attributes column too.
> That would make the column narrower, which might be good, because it
> seems to me that listing the memberships could require quite a lot of
> space, both vertical and horizontal.
Right, that's what I was thinking.
On Tue, Apr 4, 2023 at 3:02 PM David G. Johnston <david.g.johnston@gmail.com> wrote: > So, by way of example: > > regress_du_role1 | cannot login | regress_du_role0 granted by regress_du_admin with admin, inherit, set | Description forregress_du_role1 > > ~140 character width with description That seems wider than necessary. Why not have the third column be something like regress_du_role0 by regress_du_admin (admin, inherit, set)? -- Robert Haas EDB: http://www.enterprisedb.com
On 04.04.2023 23:00, Robert Haas wrote: > On Tue, Apr 4, 2023 at 3:02 PM David G. Johnston > <david.g.johnston@gmail.com> wrote: >> So, by way of example: >> >> regress_du_role1 | cannot login | regress_du_role0 granted by regress_du_admin with admin, inherit, set | Descriptionfor regress_du_role1 >> >> >> That seems wider than necessary. Why not have the third column be >> something like regress_du_role0 by regress_du_admin (admin, inherit, >> set)? 'granted by' can be left without translation, but just 'by' required translation, as I think. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
On Tue, Apr 4, 2023 at 10:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 4, 2023 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wonder if, while we're here, we should apply the idea of
>> joining-with-newlines-not-commas to the attributes column too.
> That would make the column narrower, which might be good, because it
> seems to me that listing the memberships could require quite a lot of
> space, both vertical and horizontal.
Right, that's what I was thinking.So, by way of example:regress_du_role1 | cannot login | regress_du_role0 granted by regress_du_admin with admin, inherit, set | Description for regress_du_role1
Perhaps more closely to syntax?
regress_du_role0 with admin, inherit, set granted by regress_du_admin
instead of
regress_du_role0 granted by regress_du_admin with admin, inherit, set
No translations, all words are either identical to syntax or identifiers.I'm on board with newlines in the attributes field.
+1
The specific member of column changes are:from -> granted by( ) -> "with"ais -> admin, inherit, setI'm good with any or all of those selections, either as-is or in the more verbose form.
From yesterday's discussion, I think two things are important:
- it is advisable to avoid translation,
- there is no sense in the abbreviation (a,i,s), if there are full names in the 'attributes' column.
So I agree with such changes and plan to implement them.
And one more question. (I think it's better to have it explicitly rejected than to keep silent.)
What if this long output will be available only for \du+, and for \du just show distinct (without duplicates)
roles in the current array format? For those, who don't care about these new membership options, nothing will change.
Those, who need details will use the + modifier.
?
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
Pavel Luzanov <p.luzanov@postgrespro.ru> writes: > What if this long output will be available only for \du+, and for \du > just show distinct (without duplicates) > roles in the current array format? For those, who don't care about these > new membership options, nothing will change. > Those, who need details will use the + modifier. > ? I kind of like that. Would we change to newlines in the Attributes field in both \du and \du+? (I'm +1 for that, but maybe others aren't.) regards, tom lane
Pavel Luzanov <p.luzanov@postgrespro.ru> writes:
> What if this long output will be available only for \du+, and for \du
> just show distinct (without duplicates)
> roles in the current array format? For those, who don't care about these
> new membership options, nothing will change.
> Those, who need details will use the + modifier.
> ?
I kind of like that. Would we change to newlines in the Attributes
field in both \du and \du+? (I'm +1 for that, but maybe others aren't.)
All attributes are translatable. Also, two of nine attributes shows in new line separated format (connection limit and password valid until).
$ LANGUAGE=fr psql -c "ALTER ROLE postgres CONNECTION LIMIT 3 VALID UNTIL 'infinity'" -c '\du'
ALTER ROLE
Liste des rôles
Nom du rôle | Attributs | Membre de
-------------+---------------------------------------------------------------------------------+-----------
postgres | Superutilisateur, Créer un rôle, Créer une base, Réplication, Contournement RLS+| {}
| 3 connexions +|
| Mot de passe valide jusqu'à infinity |
So I decided to keep the format suggested by David, but without abbreviations and only for extended mode.
$ psql -c '\duS+'
List of roles
Role name | Attributes | Member of | Description
-----------------------------+-------------------------------+---------------------------------------------------+-------------
pg_checkpoint | Cannot login | |
pg_create_subscription | Cannot login | |
pg_database_owner | Cannot login | |
pg_execute_server_program | Cannot login | |
pg_maintain | Cannot login | |
pg_monitor | Cannot login | pg_read_all_settings from postgres (inherit, set)+|
| | pg_read_all_stats from postgres (inherit, set) +|
| | pg_stat_scan_tables from postgres (inherit, set) |
pg_read_all_data | Cannot login | |
pg_read_all_settings | Cannot login | |
pg_read_all_stats | Cannot login | |
pg_read_server_files | Cannot login | |
pg_signal_backend | Cannot login | |
pg_stat_scan_tables | Cannot login | |
pg_use_reserved_connections | Cannot login | |
pg_write_all_data | Cannot login | |
pg_write_server_files | Cannot login | |
postgres | Superuser +| |
| Create role +| |
| Create DB +| |
| Replication +| |
| Bypass RLS +| |
| 3 connections +| |
| Password valid until infinity | |
Please look at new version. I understand that this is a compromise choice.
I am ready to change it if a better solution is offered.
P.S. If no objections I plan to add this patch to Open Items for v16
https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items
On Wed, Apr 5, 2023 at 6:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:Pavel Luzanov <p.luzanov@postgrespro.ru> writes:
> What if this long output will be available only for \du+, and for \du
> just show distinct (without duplicates)
> roles in the current array format? For those, who don't care about these
> new membership options, nothing will change.
> Those, who need details will use the + modifier.
> ?
I kind of like that. Would we change to newlines in the Attributes
field in both \du and \du+? (I'm +1 for that, but maybe others aren't.)If we don't change the \du "Member of" column display (aside from removing duplicates) I'm disinclined to change the Attributes column.I too am partial to only exposing this detail on the extended (+) display.David J.
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
Вложения
Sorry for joining in late.. At Thu, 13 Apr 2023 15:44:20 +0300, Pavel Luzanov <p.luzanov@postgrespro.ru> wrote in > After playing with the \du command, I found that we can't avoid > translation. > All attributes are translatable. Also, two of nine attributes shows in > new line separated format (connection limit and password valid until). Going a bit off-topic here, but I'd like the "infinity" to be translatable... As David-G appears to express concern in upthread, I don't think a direct Japanese translation from "rolename from rolename" works well, as the "from" needs accompanying verb. I, as a Japanese speaker, I prefer a more non-sentence-like notation, similar to David's suggestion but with slight differences: "pg_read_all_stats (grantor: postgres, inherit, set)" This is easily translated into Japanese. "pg_read_all_stats (付与者: postgres、継承、設定)" Come to think of this, I recalled a past discussion in which we concluded that translating punctuation marks appearing between a variable number of items within list expressions should be avoided... Thus, I'd like to propose to use an ACL-like notation, which doesn't need translation. ..| Mamber of | ..| pg_read_server_files=ais/horiguti,pg_execute_server_program=/postgres | If we'd like, but not likely, we might want to provide a parallel function to aclexplode for this notation. =# select memberofexplode('pg_read_server_files=ais/horiguti,pg_execute_server_program=/postgres'); privilege | grantor | admin | inherit | set ---------------------------+----------+-------+---------+------- pg_read_server_files | horiguti | true | true | true pg_execute_server_programs | postgres | false | false | false regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 14.04.2023 10:28, Kyotaro Horiguchi wrote: > As David-G appears to express concern in upthread, I don't think a > direct Japanese translation from "rolename from rolename" works well, > as the "from" needs accompanying verb. I, as a Japanese speaker, I > prefer a more non-sentence-like notation, similar to David's > suggestion but with slight differences: > > "pg_read_all_stats (grantor: postgres, inherit, set)" In this form, it confuses me that 'postgres' and 'inherit, set' look like a common list. > Come to think of this, I recalled a past discussion in which we > concluded that translating punctuation marks appearing between a > variable number of items within list expressions should be avoided... > > Thus, I'd like to propose to use an ACL-like notation, which doesn't > need translation. > > ..| Mamber of | > ..| pg_read_server_files=ais/horiguti,pg_execute_server_program=/postgres | It's very tempting to do so. But I don't like this approach. Showing membership options as an ACL-like column will be confusing. Even in your example, my first reaction is that pg_execute_server_program is available to public. (As for the general patterns, we can also consider combining three options into one column, like pg_class.reloptions.) So, yet another way to discuss: pg_read_all_stats(inherit,set/horiguti) pg_execute_server_program(empty/postgres) One more point. Grants without any option probably should be prohibited as useless. But this is for a new thread. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
On 4/13/23 8:44 AM, Pavel Luzanov wrote: > P.S. If no objections I plan to add this patch to Open Items for v16 > https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items [RMT hat] I don't see why this is an open item as this feature was not committed for v16. Open items typically revolve around committed features. Unless someone makes a convincing argument otherwise, I'll remove this from the Open Items list[1] tomorrow. Thanks, Jonathan [1] https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items
Вложения
On 4/13/23 8:44 AM, Pavel Luzanov wrote:
> P.S. If no objections I plan to add this patch to Open Items for v16
> https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items
[RMT hat]
I don't see why this is an open item as this feature was not committed
for v16. Open items typically revolve around committed features.
Unless someone makes a convincing argument otherwise, I'll remove this
from the Open Items list[1] tomorrow.
Thanks,
Jonathan
[1] https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items
"David G. Johnston" <david.g.johnston@gmail.com> writes: > On Wed, May 3, 2023 at 9:00 AM Jonathan S. Katz <jkatz@postgresql.org> > wrote: >> I don't see why this is an open item as this feature was not committed >> for v16. Open items typically revolve around committed features. > The argument is that updating the psql \d views to show the newly added > options is something that the patch to add those options should have done > before being committed. Yeah, if there is not any convenient way to see that info in psql then that seems like a missing part of the feature. regards, tom lane
On 5/3/23 12:25 PM, Tom Lane wrote: > "David G. Johnston" <david.g.johnston@gmail.com> writes: >> On Wed, May 3, 2023 at 9:00 AM Jonathan S. Katz <jkatz@postgresql.org> >> wrote: >>> I don't see why this is an open item as this feature was not committed >>> for v16. Open items typically revolve around committed features. > >> The argument is that updating the psql \d views to show the newly added >> options is something that the patch to add those options should have done >> before being committed. > > Yeah, if there is not any convenient way to see that info in psql > then that seems like a missing part of the feature. [RMT hat] OK -- I was rereading the thread again to see if I could glean that insight. There was a comment buried in the thread with David's opinion on that front, but no one had +1'd that. However, if this is for feature completeness, I'll withdraw the closing of the open item, but would strongly suggest we complete it in time for Beta 1. [Personal hat] Looking at Pavel's latest patch, I do find the output easy to understand, though do we need to explicitly list "empty" if there are no membership permissions? Thanks, Jonathan
Вложения
[Personal hat]
Looking at Pavel's latest patch, I do find the output easy to
understand, though do we need to explicitly list "empty" if there are no
membership permissions?
But if it is really a blocker then maybe we should produce 3 separate newline separated columns, one for the member of role, one for the list of attributes, and one with the grantor. The column headers can be translated more easily as single nouns. The readability quite probably would end up being equivalent (maybe even better) in tabular form instead of sentence form.
Just to visualize this approach. Below are the output for the tabular form and the sentence form from last patch version (sql script attached):
Tabular form rolname | memberof | options | grantor ------------------+------------------+---------------------+------------------ postgres | | | regress_du_admin | regress_du_role0+| admin, inherit, set+| postgres + | regress_du_role1+| admin, inherit, set+| postgres + | regress_du_role2 | admin, inherit, set | postgres regress_du_role0 | | | regress_du_role1 | regress_du_role0+| admin, inherit, set+| regress_du_admin+ | regress_du_role0+| inherit +| regress_du_role1+ | regress_du_role0 | set | regress_du_role2 regress_du_role2 | regress_du_role0+| admin +| regress_du_admin+ | regress_du_role0+| inherit, set +| regress_du_role1+ | regress_du_role0+| empty +| regress_du_role2+ | regress_du_role1 | admin, set | regress_du_admin (5 rows) Sentence form from patch v7 rolname | memberof ------------------+-------------------------------------------------------------- postgres | regress_du_admin | regress_du_role0 from postgres (admin, inherit, set) + | regress_du_role1 from postgres (admin, inherit, set) + | regress_du_role2 from postgres (admin, inherit, set) regress_du_role0 | regress_du_role1 | regress_du_role0 from regress_du_admin (admin, inherit, set)+ | regress_du_role0 from regress_du_role1 (inherit) + | regress_du_role0 from regress_du_role2 (set) regress_du_role2 | regress_du_role0 from regress_du_admin (admin) + | regress_du_role0 from regress_du_role1 (inherit, set) + | regress_du_role0 from regress_du_role2 (empty) + | regress_du_role1 from regress_du_admin (admin, set) (5 rows)
The tabular form solves the latest patch translation problems mentioned by Kyotaro. But it requires mapping elements between 3 array-like columns. To move forward, needs more opinions?
----- Pavel Luzanov
Вложения
On 5/7/23 3:14 PM, Pavel Luzanov wrote: > On 05.05.2023 19:51, David G. Johnston wrote: >> But if it is really a blocker then maybe we should produce 3 separate >> newline separated columns, one for the member of role, one for the >> list of attributes, and one with the grantor. The column headers can >> be translated more easily as single nouns. The readability quite >> probably would end up being equivalent (maybe even better) in tabular >> form instead of sentence form. > > Just to visualize this approach. Below are the output for the tabular > form and the sentence form from last patch version (sql script attached): > > Tabular form rolname | memberof | options > | grantor > ------------------+------------------+---------------------+------------------ postgres | | | regress_du_admin | regress_du_role0+| admin, inherit, set+| postgres + |regress_du_role1+| admin, inherit, set+| postgres + | regress_du_role2 | admin, inherit, set | postgres regress_du_role0| | | regress_du_role1 | regress_du_role0+| admin, inherit,set+| regress_du_admin+ | regress_du_role0+| inherit +| regress_du_role1+ | regress_du_role0 | set | regress_du_role2 regress_du_role2 | regress_du_role0+|admin +| regress_du_admin+ | regress_du_role0+| inherit, set +| regress_du_role1+ | regress_du_role0+| empty +| regress_du_role2+ | regress_du_role1| admin, set | regress_du_admin(5 rows)Sentence form from patch v7 rolname | memberof ------------------+-------------------------------------------------------------- postgres | regress_du_admin | regress_du_role0from postgres (admin, inherit, set) + | regress_du_role1 from postgres (admin, inherit,set) + | regress_du_role2 from postgres (admin, inherit, set) regress_du_role0 | regress_du_role1| regress_du_role0 from regress_du_admin (admin, inherit, set)+ | regress_du_role0 fromregress_du_role1 (inherit) + | regress_du_role0 from regress_du_role2 (set) regress_du_role2| regress_du_role0 from regress_du_admin (admin) + | regress_du_role0from regress_du_role1 (inherit, set) + | regress_du_role0 from regress_du_role2 (empty) + | regress_du_role1 from regress_du_admin (admin, set)(5 rows) > > The tabular form solves the latest patch translation problems mentioned by Kyotaro. > But it requires mapping elements between 3 array-like columns. > > To move forward, needs more opinions? [RMT Hat] Nudging this along, as it's an open item. It'd be good to get this resolved before Beta 1, but that may be tough at this point. [Personal hat] I'm probably not the target user for this feature, so I'm not sure how much you should weigh my opinion (e.g. I still don't agree with explicitly showing "empty", but as mentioned, I'm not the target user). That said, from a readability standpoint, it was easier for me to follow the tabular form vs. the sentence form. Thanks, Jonathan
Вложения
On 18.05.2023 05:42, Jonathan S. Katz wrote:
That said, from a readability standpoint, it was easier for me to follow the tabular form vs. the sentence form.
May be possible to reach a agreement on the sentence form. Similar descriptions used for referential constraints in the \d command:
create table t1 (id int primary key); create table t2 (id int references t1(id)); \d t2 Table "public.t2" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- id | integer | | | Foreign-key constraints: "t2_id_fkey" FOREIGN KEY (id) REFERENCES t1(id) As for tabular form it looks more natural to have a separate psql command for pg_auth_members system catalog. Something based on this query: SELECT r.rolname role, m.rolname member, admin_option admin, inherit_option inherit, set_option set, g.rolname grantor FROM pg_catalog.pg_auth_members pam JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid) JOIN pg_catalog.pg_roles m ON (pam.member = m.oid) JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid) WHERE r.rolname !~ '^pg_' ORDER BY role, member, grantor; role | member | admin | inherit | set | grantor ------------------+------------------+-------+---------+-----+------------------ regress_du_role0 | regress_du_admin | t | t | t | postgres regress_du_role0 | regress_du_role1 | t | t | t | regress_du_admin regress_du_role0 | regress_du_role1 | f | t | f | regress_du_role1 regress_du_role0 | regress_du_role1 | f | f | t | regress_du_role2 regress_du_role0 | regress_du_role2 | t | f | f | regress_du_admin regress_du_role0 | regress_du_role2 | f | t | t | regress_du_role1 regress_du_role0 | regress_du_role2 | f | f | f | regress_du_role2 regress_du_role1 | regress_du_admin | t | t | t | postgres regress_du_role1 | regress_du_role2 | t | f | t | regress_du_admin regress_du_role2 | regress_du_admin | t | t | t | postgres (10 rows) But is it worth inventing a new psql command for this?
----- Pavel Luzanov
rolname | memberof | options | grantor ------------------+------------------+---------------------+------------------ postgres | | | regress_du_admin | regress_du_role0+| admin, inherit, set+| postgres + | regress_du_role1+| admin, inherit, set+| postgres + | regress_du_role2 | admin, inherit, set | postgres regress_du_role0 | | | regress_du_role1 | regress_du_role0+| admin, inherit, set+| regress_du_admin+ | regress_du_role0+| inherit +| regress_du_role1+ | regress_du_role0 | set | regress_du_role2 regress_du_role2 | regress_du_role0+| admin +| regress_du_admin+ | regress_du_role0+| inherit, set +| regress_du_role1+ | regress_du_role0+| empty +| regress_du_role2+ | regress_du_role1 | admin, set | regress_du_admin (5 rows)
On 18.05.2023 05:42, Jonathan S. Katz wrote:That said, from a readability standpoint, it was easier for me to follow the tabular form vs. the sentence form.May be possible to reach a agreement on the sentence form. Similar descriptions used for referential constraints in the \d command:
As for tabular form it looks more natural to have a separate psql command for pg_auth_members system catalog. Something based on this query: But is it worth inventing a new psql command for this?
On 6/15/23 2:47 PM, David G. Johnston wrote: > Robert - can you please comment on what you are willing to commit in > order to close out your open item here. My take is that the design for > this, the tabular form a couple of emails ago (copied here), is > ready-to-commit, just needing the actual (trivial) code changes to be > made to accomplish it. > > Tabular form > > rolname | memberof | options | > grantor > ------------------+------------------+---------------------+------------------ postgres | | | regress_du_admin | regress_du_role0+| admin, inherit, set+| postgres + |regress_du_role1+| admin, inherit, set+| postgres + | regress_du_role2 | admin, inherit, set | postgres regress_du_role0| | | regress_du_role1 | regress_du_role0+| admin, inherit,set+| regress_du_admin+ | regress_du_role0+| inherit +| regress_du_role1+ | regress_du_role0 | set | regress_du_role2 regress_du_role2 | regress_du_role0+|admin +| regress_du_admin+ | regress_du_role0+| inherit, set +| regress_du_role1+ | regress_du_role0+| empty +| regress_du_role2+ | regress_du_role1| admin, set | regress_du_admin(5 rows) > [RMT hat] Can we resolve this before Beta 2?[1] The RMT originally advised to try to resolve before Beta 1[2], and this seems to be lingering. Thanks, Jonathan [1] https://www.postgresql.org/message-id/460ae02a-3123-16a3-f2d7-ccd79778819b%40postgresql.org [2] https://www.postgresql.org/message-id/d61db38b-29d9-81cc-55b3-8a5c704bb969%40postgresql.org
Вложения
"Jonathan S. Katz" <jkatz@postgresql.org> writes: > On 6/15/23 2:47 PM, David G. Johnston wrote: >> Robert - can you please comment on what you are willing to commit in >> order to close out your open item here. My take is that the design for >> this, the tabular form a couple of emails ago (copied here), is >> ready-to-commit, just needing the actual (trivial) code changes to be >> made to accomplish it. > Can we resolve this before Beta 2?[1] The RMT originally advised to try > to resolve before Beta 1[2], and this seems to be lingering. At this point I kinda doubt that we can get this done before beta2 either, but I'll put in my two cents anyway: * I agree that the "tabular" format looks nicer and has fewer i18n issues than the other proposals. * Personally I could do without the "empty" business, but that seems unnecessary in the tabular format; an empty column will serve fine. * I also agree with Pavel's comment that we'd be better off taking this out of \du altogether and inventing a separate \d command. Maybe "\drg" for "display role grants"? * Parenthetically, the "Attributes" column of \du is a complete disaster, lacking not only conceptual but even notational consistency. (Who decided that some items belonged on their own line and others not?) I suppose it's way too late to redesign that for v16. But I think we'd have more of a free hand to clean that up if we weren't trying to shoehorn role grants into the same display. regards, tom lane
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> On 6/15/23 2:47 PM, David G. Johnston wrote:
>> Robert - can you please comment on what you are willing to commit in
>> order to close out your open item here. My take is that the design for
>> this, the tabular form a couple of emails ago (copied here), is
>> ready-to-commit, just needing the actual (trivial) code changes to be
>> made to accomplish it.
> Can we resolve this before Beta 2?[1] The RMT originally advised to try
> to resolve before Beta 1[2], and this seems to be lingering.
At this point I kinda doubt that we can get this done before beta2
either, but I'll put in my two cents anyway:
* I agree that the "tabular" format looks nicer and has fewer i18n
issues than the other proposals.
* Personally I could do without the "empty" business, but that seems
unnecessary in the tabular format; an empty column will serve fine.
* I also agree with Pavel's comment that we'd be better off taking
this out of \du altogether and inventing a separate \d command.
Maybe "\drg" for "display role grants"?
* Parenthetically, the "Attributes" column of \du is a complete
disaster
"David G. Johnston" <david.g.johnston@gmail.com> writes: > On Thu, Jun 22, 2023 at 5:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> * I agree that the "tabular" format looks nicer and has fewer i18n >> issues than the other proposals. > As you are on board with a separate command please clarify whether you mean > the tabular format but still with newlines, one row per grantee, or the > table with one row per grantor-grantee pair. I'd lean towards a straight table with a row per grantee/grantor. I tend to think that faking table layout with some newlines is a poor idea. I'm not dead set on that approach though. regards, tom lane
On 6/23/23 11:52 AM, David G. Johnston wrote: > On Thu, Jun 22, 2023 at 5:08 PM Tom Lane <tgl@sss.pgh.pa.us > <mailto:tgl@sss.pgh.pa.us>> wrote: > > "Jonathan S. Katz" <jkatz@postgresql.org > <mailto:jkatz@postgresql.org>> writes: > > On 6/15/23 2:47 PM, David G. Johnston wrote: > >> Robert - can you please comment on what you are willing to > commit in > >> order to close out your open item here. My take is that the > design for > >> this, the tabular form a couple of emails ago (copied here), is > >> ready-to-commit, just needing the actual (trivial) code changes > to be > >> made to accomplish it. > > > Can we resolve this before Beta 2?[1] The RMT originally advised > to try > > to resolve before Beta 1[2], and this seems to be lingering. > > At this point I kinda doubt that we can get this done before beta2 > either, but I'll put in my two cents anyway: [RMT Hat] Well, the probability of completing this before the beta 2 freeze is effectively zero now. This is a bit disappointing as there was ample time since the first RMT nudge on the issue. But let's move forward and resolve it before Beta 3. > * I agree that the "tabular" format looks nicer and has fewer i18n > issues than the other proposals. > > As you are on board with a separate command please clarify whether you > mean the tabular format but still with newlines, one row per grantee, or > the table with one row per grantor-grantee pair. > > I still like using newlines here even in the separate meta-command. (I'll save for the downthread comment). > > * Personally I could do without the "empty" business, but that seems > unnecessary in the tabular format; an empty column will serve fine. > > > I disagree, but not strongly. > > I kinda expected you to be on the side of "why are we discussing a > situation that should just be prohibited" though. [Personal hat] I'm still not a fan of "empty" but perhaps the formatting around the "separate command" will help drive a conclusion on this. > > * I also agree with Pavel's comment that we'd be better off taking > this out of \du altogether and inventing a separate \d command. > Maybe "\drg" for "display role grants"? > > Just to be clear, the open item fix proposal is to remove the presently > broken (due to it showing duplicates without any context) "member of" > array in \du and make a simple table report output in \drg instead. > > I'm good with \drg as a new meta-command. [Personal hat] +1 for a new command. The proposal above seems reasonable. Thanks, Jonathan
Вложения
On 6/23/23 12:16 PM, Tom Lane wrote: > "David G. Johnston" <david.g.johnston@gmail.com> writes: >> On Thu, Jun 22, 2023 at 5:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> * I agree that the "tabular" format looks nicer and has fewer i18n >>> issues than the other proposals. > >> As you are on board with a separate command please clarify whether you mean >> the tabular format but still with newlines, one row per grantee, or the >> table with one row per grantor-grantee pair. > > I'd lean towards a straight table with a row per grantee/grantor. > I tend to think that faking table layout with some newlines is > a poor idea. I'm not dead set on that approach though. [Personal hat] Generally, I find the tabular view w/o newlines is easier to read, and makes it simpler to join to other data (though that may not be applicable here). Again, I'm not the target user of this feature (until I need to use it), so my opinion comes with a few grains of salt. Thanks, Jonathan
Вложения
"David G. Johnston" <david.g.johnston@gmail.com> writes: > On Thu, Jun 22, 2023 at 5:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> * Personally I could do without the "empty" business, but that seems >> unnecessary in the tabular format; an empty column will serve fine. > I disagree, but not strongly. > I kinda expected you to be on the side of "why are we discussing a > situation that should just be prohibited" though. I haven't formed an opinion yet on whether it should be prohibited. But even if we do that going forward, won't psql need to deal with such cases when examining old servers? regards, tom lane
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Thu, Jun 22, 2023 at 5:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * Personally I could do without the "empty" business, but that seems
>> unnecessary in the tabular format; an empty column will serve fine.
> I disagree, but not strongly.
> I kinda expected you to be on the side of "why are we discussing a
> situation that should just be prohibited" though.
I haven't formed an opinion yet on whether it should be prohibited.
But even if we do that going forward, won't psql need to deal with
such cases when examining old servers?
Thank you for all valuable comments. I can now continue working on the patch. Here's what I plan to do in the next version. Changes for \du & \dg commands * showing distinct roles in the "Member of" column * explicit order for list of roles * no changes for extended mode (\du+) New meta-command \drg * showing info from pg_auth_members based on a query: SELECT r.rolname role, m.rolname member, pg_catalog.concat_ws(', ', CASE WHEN pam.admin_option THEN 'ADMIN' END, CASE WHEN pam.inherit_option THEN 'INHERIT' END, CASE WHEN pam.set_option THEN 'SET' END ) AS options, g.rolname grantor FROM pg_catalog.pg_auth_members pam JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid) JOIN pg_catalog.pg_roles m ON (pam.member = m.oid) JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid) WHERE r.rolname !~ '^pg_' ORDER BY role, member, grantor; role | member | options | grantor ------------------+------------------+---------------------+------------------ regress_du_role0 | regress_du_admin | ADMIN, INHERIT, SET | postgres regress_du_role0 | regress_du_role1 | ADMIN, INHERIT, SET | regress_du_admin regress_du_role0 | regress_du_role1 | INHERIT | regress_du_role1 regress_du_role0 | regress_du_role1 | SET | regress_du_role2 regress_du_role0 | regress_du_role2 | ADMIN | regress_du_admin regress_du_role0 | regress_du_role2 | INHERIT, SET | regress_du_role1 regress_du_role0 | regress_du_role2 | | regress_du_role2 regress_du_role1 | regress_du_admin | ADMIN, INHERIT, SET | postgres regress_du_role1 | regress_du_role2 | ADMIN, SET | regress_du_admin regress_du_role2 | regress_du_admin | ADMIN, INHERIT, SET | postgres (10 rows) Notes * The name of the new command. It's a good name, if not for the history. There are two commands showing the same information about roles: \du and \dr. The addition of \drg may be misinterpreted: if there is \drg, then there is also \dug. Maybe it's time to think about deprecating of the \du command and leave only \dg in the next versions? * 'empty'. I suggest thinking about forbidding the situation with empty options. If we prohibit them, the issue will be resolved automatically. * The new meta-command will also make sense for versions <16. The ADMIN OPTION is available in all supported versions. * The new meta-command will not show all roles. It will only show the roles included in other roles. To show all roles you need to add an outer join between pg_roles and pg_auth_members. But all columns except "role" will be left blank. Is it worth doing this? -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Notes
* The name of the new command. It's a good name, if not for the history.
There are two commands showing the same information about roles: \du and
\dr.
The addition of \drg may be misinterpreted: if there is \drg, then there
is also \dug.
Maybe it's time to think about deprecating of the \du command and leave
only \dg in the next versions?
* The new meta-command will also make sense for versions <16.
The ADMIN OPTION is available in all supported versions.
* The new meta-command will not show all roles. It will only show the
roles included in other roles.
To show all roles you need to add an outer join between pg_roles and
pg_auth_members.
But all columns except "role" will be left blank. Is it worth doing this?
On Sat, Jun 24, 2023 at 8:11 AM Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:There are two commands showing the same information about roles: \du and
\dr.I would add \dr as the new official command to complement adding \drg and deprecate both \du and \dg. Though actual removal and de-documenting doesn't seem like a good idea. But if we ever did assign something non-role to \dr it would be very confusing.
It's my mistake and inattention. I was thinking about '\du' and '\dg', and wrote about '\du' and '\dr'.
I agree that \dr and \drg the best names.
So, now concentrating on implementing \drg.
* The new meta-command will also make sense for versions <16.
The ADMIN OPTION is available in all supported versions.Doesn't every role pre-16 gain SET permission? We can also deduce whether the grant provides INHERIT based upon the attribute of the role in question.
Indeed! I will do so.
* The new meta-command will not show all roles. It will only show the
roles included in other roles.
To show all roles you need to add an outer join between pg_roles and
pg_auth_members.
But all columns except "role" will be left blank. Is it worth doing this?I'm inclined to want this. I would be good when specifying a role to filter upon that all rows that do exist matching that filter end up in the output regardless if they are standalone or not.
Ok
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
Please find attached new patch version. It implements \drg command and hides duplicates in \du & \dg commands. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Вложения
Pavel Luzanov <p.luzanov@postgrespro.ru> writes: > Please find attached new patch version. > It implements \drg command and hides duplicates in \du & \dg commands. I took a quick look through this, and have some minor suggestions: 1. I was thinking in terms of dropping the "Member of" column entirely in \du and \dg. It doesn't tell you enough, and the output of those commands is often too wide already. 2. You have describeRoleGrants() set up to localize "ADMIN", "INHERIT", and "SET". Since those are SQL keywords, our usual practice is to not localize them; this'd simplify the code. 3. Not sure about use of LEFT JOIN in the query. That will mean we get a row out even for roles that have no grants, which seems like clutter. The LEFT JOINs to r and g are fine, but I suggest changing the first join to a plain join. Beyond those nits, I think this is a good approach and we should adopt it (including in v16). regards, tom lane
1. I was thinking in terms of dropping the "Member of" column entirely in \du and \dg. It doesn't tell you enough, and the output of those commands is often too wide already.
I understood it that way that the dropping "Member of" column will be done as part of another work for v17. [1]
But I'm ready to do it now.
2. You have describeRoleGrants() set up to localize "ADMIN", "INHERIT", and "SET". Since those are SQL keywords, our usual practice is to not localize them; this'd simplify the code.
The reason is that \du has translatable all attributes of the role, including NOINHERIT.
I decided to make a new command the same way.
But I'm ready to leave them untranslatable, if no objections.
3. Not sure about use of LEFT JOIN in the query. That will mean we get a row out even for roles that have no grants, which seems like clutter. The LEFT JOINs to r and g are fine, but I suggest changing the first join to a plain join.
It was David's suggestion:
On Sat, Jun 24, 2023 at 8:11 AM Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:* The new meta-command will not show all roles. It will only show the
roles included in other roles.
To show all roles you need to add an outer join between pg_roles and
pg_auth_members.
But all columns except "role" will be left blank. Is it worth doing this?I'm inclined to want this. I would be good when specifying a role to filter upon that all rows that do exist matching that filter end up in the output regardless if they are standalone or not.
Personally, I tend to think that left join is not needed. No memberships - nothing shown.
So, I accepted all three suggestions. I will wait for other opinions and
plan to implement discussed changes within a few days.
1. https://www.postgresql.org/message-id/4133242.1687481416%40sss.pgh.pa.us
-- Pavel Luzanov Postgres Professional: https://postgrespro.com
On 09.07.2023 13:56, Pavel Luzanov wrote: > On 08.07.2023 20:07, Tom Lane wrote: >> 1. I was thinking in terms of dropping the "Member of" column entirely >> in \du and \dg. It doesn't tell you enough, and the output of those >> commands is often too wide already. > >> 2. You have describeRoleGrants() set up to localize "ADMIN", "INHERIT", >> and "SET". Since those are SQL keywords, our usual practice is to not >> localize them; this'd simplify the code. > > >> 3. Not sure about use of LEFT JOIN in the query. That will mean we >> get a row out even for roles that have no grants, which seems like >> clutter. The LEFT JOINs to r and g are fine, but I suggest changing >> the first join to a plain join. > > So, I accepted all three suggestions. I will wait for other opinions and > plan to implement discussed changes within a few days. Please review the updated version with suggested changes. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Вложения
On 08.07.2023 20:07, Tom Lane wrote > 3. Not sure about use of LEFT JOIN in the query. That will mean we > get a row out even for roles that have no grants, which seems like > clutter. The LEFT JOINs to r and g are fine, but I suggest changing > the first join to a plain join. Hm. Can you explain why LEFT JOIN to r and g are fine after removing LEFT JOIN to pam? Why not to change all three left joins to plain join? The query for v16+ now looks like: SELECT m.rolname AS "Role name", r.rolname AS "Member of", pg_catalog.concat_ws(', ', CASE WHEN pam.admin_option THEN 'ADMIN' END, CASE WHEN pam.inherit_option THEN 'INHERIT' END, CASE WHEN pam.set_option THEN 'SET' END ) AS "Options", g.rolname AS "Grantor" FROM pg_catalog.pg_roles m JOIN pg_catalog.pg_auth_members pam ON (pam.member = m.oid) LEFT JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid) LEFT JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid) WHERE m.rolname !~ '^pg_' ORDER BY 1, 2, 4; And for versions <16 I forget to simplify expression for 'Options' column after removing LEFT JOIN on pam: SELECT m.rolname AS "Role name", r.rolname AS "Member of", pg_catalog.concat_ws(', ', CASE WHEN pam.admin_option THEN 'ADMIN' END, CASE WHEN pam.roleid IS NOT NULL AND m.rolinherit THEN 'INHERIT' END, CASE WHEN pam.roleid IS NOT NULL THEN 'SET' END ) AS "Options", g.rolname AS "Grantor" FROM pg_catalog.pg_roles m JOIN pg_catalog.pg_auth_members pam ON (pam.member = m.oid) LEFT JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid) LEFT JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid) WHERE m.rolname !~ '^pg_' ORDER BY 1, 2, 4; I plan to replace it to: pg_catalog.concat_ws(', ', CASE WHEN pam.admin_option THEN 'ADMIN' END, CASE WHEN m.rolinherit THEN 'INHERIT' END, 'SET' ) AS "Options", -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Pavel Luzanov <p.luzanov@postgrespro.ru> writes: > On 08.07.2023 20:07, Tom Lane wrote >> 3. Not sure about use of LEFT JOIN in the query. That will mean we >> get a row out even for roles that have no grants, which seems like >> clutter. The LEFT JOINs to r and g are fine, but I suggest changing >> the first join to a plain join. > Can you explain why LEFT JOIN to r and g are fine after removing LEFT > JOIN to pam? The idea with that, IMO, is to do something at least minimally sane if there's a bogus role OID in pg_auth_members. With plain joins, the output row would disappear and you'd have no clue that anything is wrong. With left joins, you get a row with a null column and there's reason to investigate why. Since such a case should not happen in normal use, I don't think it counts for discussions about compactness of output. However, this is also an argument for using a plain not left join between pg_roles and pg_auth_members: if we do it as per the earlier patch, then nulls in the output are common and wouldn't draw your attention. (Indeed, I think broken and not-broken pg_auth_members contents would be indistinguishable.) > I plan to replace it to: > pg_catalog.concat_ws(', ', > CASE WHEN pam.admin_option THEN 'ADMIN' END, > CASE WHEN m.rolinherit THEN 'INHERIT' END, > 'SET' > ) AS "Options", That does not seem right. Is it impossible for pam.set_option to be false? Even if it is, should this code assume that? regards, tom lane
> I plan to replace it to:
> pg_catalog.concat_ws(', ',
> CASE WHEN pam.admin_option THEN 'ADMIN' END,
> CASE WHEN m.rolinherit THEN 'INHERIT' END,
> 'SET'
> ) AS "Options",
That does not seem right. Is it impossible for pam.set_option
to be false? Even if it is, should this code assume that?
On 13.07.2023 18:01, Tom Lane wrote: > The idea with that, IMO, is to do something at least minimally sane > if there's a bogus role OID in pg_auth_members. With plain joins, > the output row would disappear and you'd have no clue that anything > is wrong. With left joins, you get a row with a null column and > there's reason to investigate why. > > Since such a case should not happen in normal use, I don't think it > counts for discussions about compactness of output. However, this > is also an argument for using a plain not left join between pg_roles > and pg_auth_members: if we do it as per the earlier patch, then > nulls in the output are common and wouldn't draw your attention. > (Indeed, I think broken and not-broken pg_auth_members contents > would be indistinguishable.) It reminds me about defensive programming practices. That's great, thanks for explanation. > That does not seem right. Is it impossible for pam.set_option > to be false? Even if it is, should this code assume that? For versions before 16, including one role to another automatically gives possibility to issue SET ROLE. IMO, the only question is whether it is correct to show IMPLICIT and SET options in versions where they are not actually present in pg_auth_members, but can be determined. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Pavel Luzanov <p.luzanov@postgrespro.ru> writes: > On 13.07.2023 18:01, Tom Lane wrote: >> That does not seem right. Is it impossible for pam.set_option >> to be false? Even if it is, should this code assume that? > For versions before 16, including one role to another automatically > gives possibility to issue SET ROLE. Right, -ENOCAFFEINE. > IMO, the only question is whether it is correct to show IMPLICIT and > SET options in versions where they are not actually present > in pg_auth_members, but can be determined. Hmm, that's definitely a judgment call. You could argue that it's useful and consistent, but also that it's confusing to somebody who's not familiar with the new terminology. On balance I'd lean to showing them, but I won't fight hard for that viewpoint. regards, tom lane
On 13.07.2023 11:26, Pavel Luzanov wrote: > And for versions <16 I forget to simplify expression for 'Options' > column after removing LEFT JOIN on pam: > > SELECT m.rolname AS "Role name", r.rolname AS "Member of", > pg_catalog.concat_ws(', ', > CASE WHEN pam.admin_option THEN 'ADMIN' END, > CASE WHEN pam.roleid IS NOT NULL AND m.rolinherit THEN 'INHERIT' END, > CASE WHEN pam.roleid IS NOT NULL THEN 'SET' END > ) AS "Options", > g.rolname AS "Grantor" > FROM pg_catalog.pg_roles m > JOIN pg_catalog.pg_auth_members pam ON (pam.member = m.oid) > LEFT JOIN pg_catalog.pg_roles r ON (pam.roleid = r.oid) > LEFT JOIN pg_catalog.pg_roles g ON (pam.grantor = g.oid) > WHERE m.rolname !~ '^pg_' > ORDER BY 1, 2, 4; > > I plan to replace it to: > > pg_catalog.concat_ws(', ', > CASE WHEN pam.admin_option THEN 'ADMIN' END, > CASE WHEN m.rolinherit THEN 'INHERIT' END, > 'SET' > ) AS "Options", > The new version contains only this change. -- ----- Pavel Luzanov
Вложения
I tried this out. It looks good to me, and I like it. Not translating the labels seems correct to me. +1 for backpatching to 16, given that it's a psql-only change that pertains to a backend change that was done in the 16 timeframe. Regarding the controversy of showing SET for previous versions, I think it's clearer if it's shown, because ultimately what the user really wants to know is if the role can be SET to; they don't want to have to learn from memory in which version they can SET because the column is empty and in which version they have to look for the label. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Small aircraft do not crash frequently ... usually only once!" (ponder, http://thedailywtf.com/)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > I tried this out. It looks good to me, and I like it. Not translating > the labels seems correct to me. > +1 for backpatching to 16, given that it's a psql-only change that > pertains to a backend change that was done in the 16 timeframe. Agreed. In the interests of moving things along, I'll take point on getting this committed. > Regarding the controversy of showing SET for previous versions, I think > it's clearer if it's shown, because ultimately what the user really > wants to know is if the role can be SET to; they don't want to have to > learn from memory in which version they can SET because the column is > empty and in which version they have to look for the label. Seems reasonable. I'll go with that interpretation unless there's pretty quick pushback. regards, tom lane
I wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> +1 for backpatching to 16, given that it's a psql-only change that >> pertains to a backend change that was done in the 16 timeframe. > Agreed. In the interests of moving things along, I'll take point > on getting this committed. And done, with some minor editorialization. I'll go mark the open item as closed. regards, tom lane
I wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> +1 for backpatching to 16, given that it's a psql-only change that
>> pertains to a backend change that was done in the 16 timeframe.
> Agreed. In the interests of moving things along, I'll take point
> on getting this committed.
And done, with some minor editorialization. I'll go mark the
open item as closed.
On 19.07.2023 19:47, Tom Lane wrote: > And done, with some minor editorialization. Thanks to everyone who participated in the work. Special thanks to David for moving forward this patch for a long time, and to Tom for taking commit responsibilities. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
On 7/19/23 1:44 PM, Pavel Luzanov wrote: > On 19.07.2023 19:47, Tom Lane wrote: >> And done, with some minor editorialization. > > Thanks to everyone who participated in the work. > Special thanks to David for moving forward this patch for a long time, > and to Tom for taking commit responsibilities. [RMT] +1; thanks to everyone for seeing this through! Jonathan