Re: [HACKERS] alter user/role CURRENT_USER
Hello, At Thu, 30 Apr 2015 17:12:25 -0300, Alvaro Herrera alvhe...@2ndquadrant.com wrote in 20150430201225.gv4...@alvh.no-ip.org Kyotaro HORIGUCHI wrote: Thank you for completing this and very sorry not to respond these days. I understood that it is committed after I noticed that rebasing my code failed.. You'd do well to check your email, I guess :-) Yeah, I agree with you since I noticed that before I read the mail mentioning that. I should be more carefull:( Sorry to bother you and thank you for your kindness. | =# alter role current_user rename to PubLic; | ERROR: CURRENT_USER cannot be used as a role name | LINE 1: alter role current_user rename to PubLic; |^ The error message sounds somewhat different from the intention. I think the following message would be clearer. | ERROR: CURRENT_USER cannot be used as a role name here Okay, changed. The document sql-altergroup.html says | ALTER GROUP role_specification ADD USER user_name [, ... ] But current_user is also usable in user_name list. So the doc should be as following, but it would not be necessary to be fixed because it is an obsolete commnand.. | ALTER GROUP role_specification ADD USER role_specification [, ... ] Yeah, EDONTCARE. ALTER GROUP role_spec ADD/DROP USER role_spec is naturally denied so I think no additional description is needed. +1 sql-alterpolicy.html ALTER POLICY name ON table_name TO also accepts current_user and so as the role to which the policy applies. Changed. # As a different topic, the syntax ALTER POLICY pname ON # tname TO user looks a bit wired, it might be better be to # be ON tname APPLY TO user but I shouldn't try to fix it # since it is a long standing syntax.. Yeah, it's a bit strange. Not a strong opinion. Maybe you should raise it as a separate thread. sql-createtablespace.html sql-drop-owned.html, sql-reassign-owned.html Changed. Thank you applying the changes above. == sql-grant.html, sql-revoke.html, GRANT roles TO roles and REVOKE roles FROM roles are the modern equivalents of the deprecated syntaxes ALTER roles ADD USER roles and ALTER roles DROP USER roles respectively. But the current parser infrastructure doesn't allow coexistence of the two following syntaxes but I couldn't find the way to their coexistence. I decided to leave this out. I think we should consider it as a new patch for 9.6; these changes aren't as clear-cut as the rest of your patch. I didn't want to have to research the ecpg changes. Ok, it sounds fair enough. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Kyotaro HORIGUCHI wrote: Thank you for completing this and very sorry not to respond these days. I understood that it is committed after I noticed that rebasing my code failed.. You'd do well to check your email, I guess :-) Although after committed, I found some issues as I looked on it. Please forgive me to comment it now after all this time. | =# alter role current_user rename to PubLic; | ERROR: CURRENT_USER cannot be used as a role name | LINE 1: alter role current_user rename to PubLic; |^ The error message sounds somewhat different from the intention. I think the following message would be clearer. | ERROR: CURRENT_USER cannot be used as a role name here Okay, changed. The document sql-altergroup.html says | ALTER GROUP role_specification ADD USER user_name [, ... ] But current_user is also usable in user_name list. So the doc should be as following, but it would not be necessary to be fixed because it is an obsolete commnand.. | ALTER GROUP role_specification ADD USER role_specification [, ... ] Yeah, EDONTCARE. ALTER GROUP role_spec ADD/DROP USER role_spec is naturally denied so I think no additional description is needed. +1 sql-alterpolicy.html ALTER POLICY name ON table_name TO also accepts current_user and so as the role to which the policy applies. Changed. # As a different topic, the syntax ALTER POLICY pname ON # tname TO user looks a bit wired, it might be better be to # be ON tname APPLY TO user but I shouldn't try to fix it # since it is a long standing syntax.. Yeah, it's a bit strange. Not a strong opinion. Maybe you should raise it as a separate thread. sql-createtablespace.html sql-drop-owned.html, sql-reassign-owned.html Changed. == sql-grant.html, sql-revoke.html, GRANT roles TO roles and REVOKE roles FROM roles are the modern equivalents of the deprecated syntaxes ALTER roles ADD USER roles and ALTER roles DROP USER roles respectively. But the current parser infrastructure doesn't allow coexistence of the two following syntaxes but I couldn't find the way to their coexistence. I decided to leave this out. I think we should consider it as a new patch for 9.6; these changes aren't as clear-cut as the rest of your patch. I didn't want to have to research the ecpg changes. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Thank you for completing this and very sorry not to respond these days. I understood that it is committed after I noticed that rebasing my code failed.. Although after committed, I found some issues as I looked on it. Please forgive me to comment it now after all this time. Several changes in docs according to the changed syntax and one change in code itself to allow CURRENT_USER in GRANT roleid TO roleid syntax. At Mon, 9 Mar 2015 15:50:32 -0300, Alvaro Herrera alvhe...@2ndquadrant.com wrote in 20150309185032.gq3...@alvh.no-ip.org Alvaro Herrera wrote: With this patch applied, doing \h ALTER ROLE in psql looks quite odd: note how wide it has become. Maybe we should be doing this differently? (Hmm, why don't we accept ALL in the first SET line? Maybe that's just a mistake and the four lines should be all identical in the first half ...) I have fixed the remaining issues, completed the doc changes, and pushed. Given the lack of feedback I had to follow my gut on the best way to change the docs. I added the regression test you submitted with some additional changes, mainly to make sure they don't fail immediately when other databases exist; maybe some more concurrency or platform issues will show up there, but let's see what the buildfarm says. Thanks Horiguchi-san for the patch and everyone for the reviews. (It's probably worthwhile giving things an extra look.) | =# alter role current_user rename to PubLic; | ERROR: CURRENT_USER cannot be used as a role name | LINE 1: alter role current_user rename to PubLic; |^ The error message sounds somewhat different from the intention. I think the following message would be clearer. | ERROR: CURRENT_USER cannot be used as a role name here The document sql-altergroup.html says | ALTER GROUP role_specification ADD USER user_name [, ... ] But current_user is also usable in user_name list. So the doc should be as following, but it would not be necessary to be fixed because it is an obsolete commnand.. | ALTER GROUP role_specification ADD USER role_specification [, ... ] ALTER GROUP role_spec ADD/DROP USER role_spec is naturally denied so I think no additional description is needed. sql-alterpolicy.html ALTER POLICY name ON table_name TO also accepts current_user and so as the role to which the policy applies. # As a different topic, the syntax ALTER POLICY pname ON # tname TO user looks a bit wired, it might be better be to # be ON tname APPLY TO user but I shouldn't try to fix it # since it is a long standing syntax.. sql-createtablespace.html OWNER username should be OWNER user_name | (CURRENT|SESSION)_USER sql-drop-owned.html, sql-reassign-owned.html name should be {name | (CURRENT|SESSION)_USER} For REASSIGN OWNED, TO clause also needs the same fix. == sql-grant.html, sql-revoke.html, GRANT roles TO roles and REVOKE roles FROM roles are the modern equivalents of the deprecated syntaxes ALTER roles ADD USER roles and ALTER roles DROP USER roles respectively. But the current parser infrastructure doesn't allow coexistence of the two following syntaxes but I couldn't find the way to their coexistence. # more precisely, I guess the GRANT followed by both # privelege_list and role_list will steps out of the realm of # LALR(1), which I don't know well of.. GRANT privs ON ... GRANT roles TO ... After some struggle, I decided to add special rules (CURRENT|SESSION)_USER to the non-terminal privilege and make a room to store RoleSpec in AccessPriv. This is quite ugly but there seems no way other than that to accomplish it.. (AccessPriv already conveys other than the information different from what its name represents:p) After this fix, the commands like following are processed properly. public and none are simply handled as nonexistent names. GRANT test1 TO CURRENT_USER; GRANT priv ON table TO role properly rejects CURRENT_USER as priv. But the change in gram.y cuases changes in preproc.y as following, privilege: SELECT opt_column_list { ... | ColId opt_column_list { $$ = cat_str(2,$1,$2); } + | CURRENT_USER + { + $$ = mm_strdup(current_user); + } + | SESSION_USER + { + $$ = mm_strdup(session_user); + } ; I don't understand what this change causes... = I haven't looked on the docs for syntaxes related to AlterOwnerStatement. But perhaps they don't be wrong. regards, -- Kyotaro Horiguchi NTT Open Source Software Center From 26afc656576c8778921ff44519e3de86866ab138 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp Date: Thu, 12 Mar 2015 20:56:14 +0900 Subject: [PATCH] Some additional changes for ALTER ROLE/USER CURRENT_USER. Some documents are not edit according to new specs. Addition to it, GRANT roles TO roles and REVOKE roles FROM roles syntaxes, which are the modern replacement of ALTER GROUP ADD/DROP USER syntax, are not accepted by the previous patch. This patch fixes some docs and changes
Re: [HACKERS] alter user/role CURRENT_USER
Alvaro Herrera wrote: With this patch applied, doing \h ALTER ROLE in psql looks quite odd: note how wide it has become. Maybe we should be doing this differently? (Hmm, why don't we accept ALL in the first SET line? Maybe that's just a mistake and the four lines should be all identical in the first half ...) I have fixed the remaining issues, completed the doc changes, and pushed. Given the lack of feedback I had to follow my gut on the best way to change the docs. I added the regression test you submitted with some additional changes, mainly to make sure they don't fail immediately when other databases exist; maybe some more concurrency or platform issues will show up there, but let's see what the buildfarm says. Thanks Horiguchi-san for the patch and everyone for the reviews. (It's probably worthwhile giving things an extra look.) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
There is something odd here going on: alvherre=# alter group test add user current_user; ERROR: role test is a member of role (null) Surely (null) is not the right name to be reporting there ... Attached is a rebased patch, which also has some incomplete doc changes. With this patch applied, doing \h ALTER ROLE in psql looks quite odd: note how wide it has become. Maybe we should be doing this differently? (Hmm, why don't we accept ALL in the first SET line? Maybe that's just a mistake and the four lines should be all identical in the first half ...) alvherre=# \h alter role Command: ALTER ROLE Description: change a database role Syntax: ALTER ROLE { name | CURRENT_USER | SESSION_USER } [ WITH ] option [ ... ] where option can be: SUPERUSER | NOSUPERUSER | CREATEDB | NOCREATEDB | CREATEROLE | NOCREATEROLE | CREATEUSER | NOCREATEUSER | INHERIT | NOINHERIT | LOGIN | NOLOGIN | REPLICATION | NOREPLICATION | BYPASSRLS | NOBYPASSRLS | CONNECTION LIMIT connlimit | [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password' | VALID UNTIL 'timestamp' ALTER ROLE name RENAME TO new_name ALTER ROLE { name | CURRENT_USER | SESSION_USER } [ IN DATABASE database_name ] SET configuration_parameter { TO | = } { value | DEFAULT } ALTER ROLE { name | CURRENT_USER | SESSION_USER | ALL } [ IN DATABASE database_name ] SET configuration_parameter FROM CURRENT ALTER ROLE { name | CURRENT_USER | SESSION_USER | ALL } [ IN DATABASE database_name ] RESET configuration_parameter ALTER ROLE { name | CURRENT_USER | SESSION_USER | ALL } [ IN DATABASE database_name ] RESET ALL -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services diff --git a/doc/src/sgml/ref/alter_group.sgml b/doc/src/sgml/ref/alter_group.sgml index 1432242..5f0d8b4 100644 --- a/doc/src/sgml/ref/alter_group.sgml +++ b/doc/src/sgml/ref/alter_group.sgml @@ -21,10 +21,10 @@ PostgreSQL documentation refsynopsisdiv synopsis -ALTER GROUP replaceable class=PARAMETERgroup_name/replaceable ADD USER replaceable class=PARAMETERuser_name/replaceable [, ... ] -ALTER GROUP replaceable class=PARAMETERgroup_name/replaceable DROP USER replaceable class=PARAMETERuser_name/replaceable [, ... ] +ALTER GROUP { replaceable class=PARAMETERgroup_name/replaceable | CURRENT_USER | SESSION_USER } ADD USER replaceable class=PARAMETERuser_name/replaceable [, ... ] +ALTER GROUP { replaceable class=PARAMETERgroup_name/replaceable | CURRENT_USER | SESSION_USER } DROP USER replaceable class=PARAMETERuser_name/replaceable [, ... ] -ALTER GROUP replaceable class=PARAMETERgroup_name/replaceable RENAME TO replaceablenew_name/replaceable +ALTER GROUP { replaceable class=PARAMETERgroup_name/replaceable | CURRENT_USER | SESSION_USER } RENAME TO replaceablenew_name/replaceable /synopsis /refsynopsisdiv diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml index 0471daa..59f6499 100644 --- a/doc/src/sgml/ref/alter_role.sgml +++ b/doc/src/sgml/ref/alter_role.sgml @@ -21,7 +21,7 @@ PostgreSQL documentation refsynopsisdiv synopsis -ALTER ROLE replaceable class=PARAMETERname/replaceable [ [ WITH ] replaceable class=PARAMETERoption/replaceable [ ... ] ] +ALTER ROLE { replaceable class=PARAMETERname/replaceable | CURRENT_USER | SESSION_USER } [ WITH ] replaceable class=PARAMETERoption/replaceable [ ... ] phrasewhere replaceable class=PARAMETERoption/replaceable can be:/phrase @@ -39,10 +39,10 @@ ALTER ROLE replaceable class=PARAMETERname/replaceable [ [ WITH ] replace ALTER ROLE replaceable class=PARAMETERname/replaceable RENAME TO replaceablenew_name/replaceable -ALTER ROLE replaceable class=PARAMETERname/replaceable [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT } -ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable FROM CURRENT -ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET replaceableconfiguration_parameter/replaceable -ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET ALL +ALTER ROLE { replaceable class=PARAMETERname/replaceable | CURRENT_USER | SESSION_USER } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT } +ALTER ROLE { replaceable class=PARAMETERname/replaceable | CURRENT_USER | SESSION_USER | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable FROM CURRENT +ALTER ROLE { replaceable
Re: [HACKERS] alter user/role CURRENT_USER
Kyotaro HORIGUCHI wrote: Hello, thank you for the comment. Looking at this a bit, I'm not sure completely replacing RoleId with a node is the best idea; some of the users of that production in the grammar are okay with accepting only normal strings as names, and don't need all the CURRENT_USER etc stuff. You're right. the productions don't need RoleSpec already uses char* for the role member in its *Stmt structs. I might have done a bit too much. Adding new nonterminal RoleId and using it makes a bit duplicate of check code for public/none and others but removes other redundant check for != CSTRING from some production, CREATE ROLE/USER/GROUP, ALTER ROLE/USER/GROUP RENAME. Thanks for doing the fiddly work here. Attached is a new version of this patch. I simplified some things, including removing those rules you added to RoleId. It seems to me that this problem: RoleId in the patch still has rule components for CURRENT_USER, SESSION_USER, and CURRENT_ROLE. Without them, the parser prints an error ununderstandable to users. | =# alter role current_user rename to PuBlic; | ERROR: syntax error at or near rename | LINE 1: alter role current_user rename to PuBlic; | ^ can be fixed without complicating the rest of the stuff simply by using RoleSpec instead of RoleId and doing the error checks at the RenameStmt production. Altering the current user and session user is disallowed downstream, so there's no reason we can't just have gram.y throw the same error when special node types are used; so we end up passing the role name only (just as currently) and the error message remains clear. I couldn't find any further problems with this version of the code, though I also noticed that a lot of things are not being tested in the regression tests, such as create user public or alter user none. It would be good to have tests for such cases, to avoid breaking them accidentally. If you can spare some time to submit test cases for such commands, I would be thankful. I'm pretty sure, thought I haven't tried yet, that we can now remove the PrivGrantee node completely. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 1e3888e..56cafa8 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -422,21 +422,24 @@ ExecuteGrantStmt(GrantStmt *stmt) /* * Convert the PrivGrantee list into an Oid list. Note that at this point - * we insert an ACL_ID_PUBLIC into the list if an empty role name is - * detected (which is what the grammar uses if PUBLIC is found), so - * downstream there shouldn't be any additional work needed to support - * this case. + * we insert an ACL_ID_PUBLIC into the list if appropriate, so downstream + * there shouldn't be any additional work needed to support this case. */ foreach(cell, stmt-grantees) { PrivGrantee *grantee = (PrivGrantee *) lfirst(cell); + Oid grantee_uid = ACL_ID_PUBLIC; - if (grantee-rolname == NULL) - istmt.grantees = lappend_oid(istmt.grantees, ACL_ID_PUBLIC); - else - istmt.grantees = -lappend_oid(istmt.grantees, - get_role_oid(grantee-rolname, false)); + switch (grantee-role-roltype) + { + case ROLESPEC_PUBLIC: +grantee_uid = ACL_ID_PUBLIC; +break; + default: +grantee_uid = get_rolespec_oid(grantee-role, false); +break; + } + istmt.grantees = lappend_oid(istmt.grantees, grantee_uid); } /* @@ -905,21 +908,24 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt) /* * Convert the PrivGrantee list into an Oid list. Note that at this point - * we insert an ACL_ID_PUBLIC into the list if an empty role name is - * detected (which is what the grammar uses if PUBLIC is found), so - * downstream there shouldn't be any additional work needed to support - * this case. + * we insert an ACL_ID_PUBLIC into the list if appropriate, so downstream + * there shouldn't be any additional work needed to support this case. */ foreach(cell, action-grantees) { PrivGrantee *grantee = (PrivGrantee *) lfirst(cell); + Oid grantee_uid = ACL_ID_PUBLIC; - if (grantee-rolname == NULL) - iacls.grantees = lappend_oid(iacls.grantees, ACL_ID_PUBLIC); - else - iacls.grantees = -lappend_oid(iacls.grantees, - get_role_oid(grantee-rolname, false)); + switch (grantee-role-roltype) + { + case ROLESPEC_PUBLIC: +grantee_uid = ACL_ID_PUBLIC; +break; + default: +grantee_uid = get_rolespec_oid(grantee-role, false); +break; + } + iacls.grantees = lappend_oid(iacls.grantees, grantee_uid); } /* diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index 78b54b4..1d8799b 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -679,7 +679,7 @@ AlterObjectNamespace_internal(Relation rel, Oid
Re: [HACKERS] alter user/role CURRENT_USER
Hello, thank you for the comment. Looking at this a bit, I'm not sure completely replacing RoleId with a node is the best idea; some of the users of that production in the grammar are okay with accepting only normal strings as names, and don't need all the CURRENT_USER etc stuff. You're right. the productions don't need RoleSpec already uses char* for the role member in its *Stmt structs. I might have done a bit too much. Adding new nonterminal RoleId and using it makes a bit duplicate of check code for public/none and others but removes other redundant check for != CSTRING from some production, CREATE ROLE/USER/GROUP, ALTER ROLE/USER/GROUP RENAME. RoleId in the patch still has rule components for CURRENT_USER, SESSION_USER, and CURRENT_ROLE. Without them, the parser prints an error ununderstandable to users. | =# alter role current_user rename to PuBlic; | ERROR: syntax error at or near rename | LINE 1: alter role current_user rename to PuBlic; | ^ With the components, the error message becomes like this. | =# alter role current_role rename to PuBlic; | ERROR: reserved word cannot be used | LINE 1: alter role current_role rename to PuBlic; |^ Maybe we need a new production, say RoleSpec, and we modify the few productions that need the extra flexibility? For instance we could have ALTER USER RoleSpec instead of ALTER USER RoleId. But we would keep CREATE USER RoleId, because it doesn't make any sense to accept CREATE USER CURRENT_USER. I think that would lead to a less invasive patch also. Am I making sense? I think I did what you expected. It was good as the code got simpler but the invasive-ness dosn't seem to be reduced.. What do you think about this? regards, -- Kyotaro Horiguchi NTT Open Source Software Center From d90b7e09968f32a5b543242469fb65e304df3318 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp Date: Fri, 14 Nov 2014 17:37:22 +0900 Subject: [PATCH] ALTER USER CURRENT_USER v4 Added RoleId for the use in where CURRENT_USER-like sutff is not used. CREATE ROLE/USER/GROUP, ALTER ROLE/USER/GROUP RENAME are changed to use RoleId. --- src/backend/catalog/aclchk.c | 30 +++-- src/backend/commands/alter.c | 2 +- src/backend/commands/extension.c | 2 +- src/backend/commands/foreigncmds.c | 57 - src/backend/commands/schemacmds.c | 30 - src/backend/commands/tablecmds.c | 4 +- src/backend/commands/tablespace.c | 2 +- src/backend/commands/user.c| 86 +++--- src/backend/nodes/copyfuncs.c | 37 -- src/backend/nodes/equalfuncs.c | 35 -- src/backend/parser/gram.y | 229 + src/backend/parser/parse_utilcmd.c | 4 +- src/backend/utils/adt/acl.c| 34 ++ src/include/commands/user.h| 2 +- src/include/nodes/nodes.h | 1 + src/include/nodes/parsenodes.h | 48 ++-- src/include/utils/acl.h| 2 +- 17 files changed, 400 insertions(+), 205 deletions(-) diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 1e3888e..1c90626 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -430,13 +430,16 @@ ExecuteGrantStmt(GrantStmt *stmt) foreach(cell, stmt-grantees) { PrivGrantee *grantee = (PrivGrantee *) lfirst(cell); + Oid grantee_uid = ACL_ID_PUBLIC; - if (grantee-rolname == NULL) - istmt.grantees = lappend_oid(istmt.grantees, ACL_ID_PUBLIC); - else - istmt.grantees = -lappend_oid(istmt.grantees, - get_role_oid(grantee-rolname, false)); + /* public is mapped to ACL_ID_PUBLIC */ + if (grantee-role-roltype != ROLESPEC_PUBLIC) + { + grantee_uid = get_rolespec_oid(grantee-role, false); + if (!OidIsValid(grantee_uid)) +grantee_uid = ACL_ID_PUBLIC; + } + istmt.grantees = lappend_oid(istmt.grantees, grantee_uid); } /* @@ -913,13 +916,16 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt) foreach(cell, action-grantees) { PrivGrantee *grantee = (PrivGrantee *) lfirst(cell); + Oid grantee_uid = ACL_ID_PUBLIC; - if (grantee-rolname == NULL) - iacls.grantees = lappend_oid(iacls.grantees, ACL_ID_PUBLIC); - else - iacls.grantees = -lappend_oid(iacls.grantees, - get_role_oid(grantee-rolname, false)); + /* public is mapped to ACL_ID_PUBLIC */ + if (grantee-role-roltype != ROLESPEC_PUBLIC) + { + grantee_uid = get_rolespec_oid(grantee-role, false); + if (!OidIsValid(grantee_uid)) +grantee_uid = ACL_ID_PUBLIC; + } + iacls.grantees = lappend_oid(iacls.grantees, grantee_uid); } /* diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index 78b54b4..1d8799b 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -679,7 +679,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid) Oid
Re: [HACKERS] alter user/role CURRENT_USER
Looking at this a bit, I'm not sure completely replacing RoleId with a node is the best idea; some of the users of that production in the grammar are okay with accepting only normal strings as names, and don't need all the CURRENT_USER etc stuff. Maybe we need a new production, say RoleSpec, and we modify the few productions that need the extra flexibility? For instance we could have ALTER USER RoleSpec instead of ALTER USER RoleId. But we would keep CREATE USER RoleId, because it doesn't make any sense to accept CREATE USER CURRENT_USER. I think that would lead to a less invasive patch also. Am I making sense? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Thank you. A new patch has been sent here but no review occurred, hence moving this item to CF 2014-12. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
On Fri, Nov 14, 2014 at 5:39 PM, Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp wrote: Hi, this is revised version. Kyotaro HORIGUCHI wrote: - Storage for new information The new struct NameId stores an identifier which telling what it logically is using the new enum NameIdTypes. I think NameId is a bad name for this. My point is that NameId, as it stands, might be a name for anything, not just a role; and the object it identifies is not an Id either. Maybe RoleSpec? Yeah! I felt it no good even if it were a generic type for various Name of something or its oid. RoleSpec sounds much better. Do we need a public_ok argument to get_nameid_oid() (get a better name for this function too) Maybe get_rolespec_oid() as a name ofter its parameter type? so that callers don't have to check for InvalidOid argument? I think the arrangement you propose is not very convenient; it'd be best to avoid duplicating the check for InvalidOid in all callers of the new function, particularly where there was no check before. I agree that It'd be better keeping away from duplicated InvalidOid checks, but public_ok seems a bit myopic. Since there's no reasonable border between functions accepting 'public' and others, such kind of solution would not be reasonable.. What about checking it being a PUBLIC or not *before* calling get_rolespec_oid()? The attached patch modified in the following points. - rename NameId to RoleSpec and NameIdType to RoleSpecTypes. - rename get_nameid_oid() to get_rolespec_oid(). - rename roleNamesToIds() to roleSpecsToIds(). - some struct members are changed such as authname to authrole. - check if rolespec is public or not before calling get_rolespec_oid() - ExecAlterDefaultPrivilegesStmt and ExecuteGrantStmt does slightly different things about ACL_ID_PUBLIC but I unified it to the latter. - rebased to the current master A new patch has been sent here but no review occurred, hence moving this item to CF 2014-12. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Hi, this is revised version. Kyotaro HORIGUCHI wrote: - Storage for new information The new struct NameId stores an identifier which telling what it logically is using the new enum NameIdTypes. I think NameId is a bad name for this. My point is that NameId, as it stands, might be a name for anything, not just a role; and the object it identifies is not an Id either. Maybe RoleSpec? Yeah! I felt it no good even if it were a generic type for various Name of something or its oid. RoleSpec sounds much better. Do we need a public_ok argument to get_nameid_oid() (get a better name for this function too) Maybe get_rolespec_oid() as a name ofter its parameter type? so that callers don't have to check for InvalidOid argument? I think the arrangement you propose is not very convenient; it'd be best to avoid duplicating the check for InvalidOid in all callers of the new function, particularly where there was no check before. I agree that It'd be better keeping away from duplicated InvalidOid checks, but public_ok seems a bit myopic. Since there's no reasonable border between functions accepting 'public' and others, such kind of solution would not be reasonable.. What about checking it being a PUBLIC or not *before* calling get_rolespec_oid()? The attached patch modified in the following points. - rename NameId to RoleSpec and NameIdType to RoleSpecTypes. - rename get_nameid_oid() to get_rolespec_oid(). - rename roleNamesToIds() to roleSpecsToIds(). - some struct members are changed such as authname to authrole. - check if rolespec is public or not before calling get_rolespec_oid() - ExecAlterDefaultPrivilegesStmt and ExecuteGrantStmt does slightly different things about ACL_ID_PUBLIC but I unified it to the latter. - rebased to the current master regards, -- Kyotaro Horiguchi NTT Open Source Software Center CreateStmt-authrole = NULL = ? From 307249654c97b6449261febbfd84190fbad9111d Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp Date: Fri, 14 Nov 2014 17:37:22 +0900 Subject: [PATCH] ALTER USER CURRENT_USER v3 --- src/backend/catalog/aclchk.c | 30 +++--- src/backend/commands/alter.c | 2 +- src/backend/commands/extension.c | 2 +- src/backend/commands/foreigncmds.c | 57 +-- src/backend/commands/schemacmds.c | 30 +- src/backend/commands/tablecmds.c | 4 +- src/backend/commands/tablespace.c | 2 +- src/backend/commands/user.c| 86 + src/backend/nodes/copyfuncs.c | 37 +--- src/backend/nodes/equalfuncs.c | 35 --- src/backend/parser/gram.y | 190 +++-- src/backend/parser/parse_utilcmd.c | 4 +- src/backend/utils/adt/acl.c| 34 +++ src/include/commands/user.h| 2 +- src/include/nodes/nodes.h | 1 + src/include/nodes/parsenodes.h | 48 +++--- src/include/utils/acl.h| 2 +- 17 files changed, 385 insertions(+), 181 deletions(-) diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index d30612c..24811c6 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -430,13 +430,16 @@ ExecuteGrantStmt(GrantStmt *stmt) foreach(cell, stmt-grantees) { PrivGrantee *grantee = (PrivGrantee *) lfirst(cell); + Oid grantee_uid = ACL_ID_PUBLIC; - if (grantee-rolname == NULL) - istmt.grantees = lappend_oid(istmt.grantees, ACL_ID_PUBLIC); - else - istmt.grantees = -lappend_oid(istmt.grantees, - get_role_oid(grantee-rolname, false)); + /* public is mapped to ACL_ID_PUBLIC */ + if (grantee-role-roltype != ROLESPEC_PUBLIC) + { + grantee_uid = get_rolespec_oid(grantee-role, false); + if (!OidIsValid(grantee_uid)) +grantee_uid = ACL_ID_PUBLIC; + } + istmt.grantees = lappend_oid(istmt.grantees, grantee_uid); } /* @@ -913,13 +916,16 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt) foreach(cell, action-grantees) { PrivGrantee *grantee = (PrivGrantee *) lfirst(cell); + Oid grantee_uid = ACL_ID_PUBLIC; - if (grantee-rolname == NULL) - iacls.grantees = lappend_oid(iacls.grantees, ACL_ID_PUBLIC); - else - iacls.grantees = -lappend_oid(iacls.grantees, - get_role_oid(grantee-rolname, false)); + /* public is mapped to ACL_ID_PUBLIC */ + if (grantee-role-roltype != ROLESPEC_PUBLIC) + { + grantee_uid = get_rolespec_oid(grantee-role, false); + if (!OidIsValid(grantee_uid)) +grantee_uid = ACL_ID_PUBLIC; + } + iacls.grantees = lappend_oid(iacls.grantees, grantee_uid); } /* diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index c9a9baf..c53d4e5 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -678,7 +678,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid) Oid ExecAlterOwnerStmt(AlterOwnerStmt *stmt) { - Oid newowner = get_role_oid(stmt-newowner, false); + Oid
Re: [HACKERS] alter user/role CURRENT_USER
Kyotaro HORIGUCHI wrote: - Storage for new information The new struct NameId stores an identifier which telling what it logically is using the new enum NameIdTypes. I think NameId is a bad name for this. My point is that NameId, as it stands, might be a name for anything, not just a role; and the object it identifies is not an Id either. Maybe RoleSpec? Do we need a public_ok argument to get_nameid_oid() (get a better name for this function too) so that callers don't have to check for InvalidOid argument? I think the arrangement you propose is not very convenient; it'd be best to avoid duplicating the check for InvalidOid in all callers of the new function, particularly where there was no check before. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Hello, This is the new version. (WIP v2) The first attachment is the patch and the second is test sql script. - Behavior changing Almost all syntax taking role accepts CURRENT_USER and SESSION_USER and they are distinguished from current_user and session_user. The exceptions are follows. - CREATE USER/GROUP roleid - ALTER ROLE/GROUP/USER roleid RENAME TO newname These syntax still do not accept the keywords like CURRENT_USER and special names like public at all, but accepts current_user. The error message is changed as follows. | postgres=# create user current_user; | ERROR: role name should not be a keyword nor reserved name. | LINE 1: create user current_user; | ^ # Some other messages may changed... USER and CURRENT_ROLE haven't been extended to other syntax. The former still can be used only in CREATE/ALTER/DROP USER MAPPING and the latter cannot be used out of function expressions. - Storage for new information The new struct NameId stores an identifier which telling what it logically is using the new enum NameIdTypes. This is still be a bit suffered by the difference between CURRENT_USER and PUBLIC but now it makes distinction between current_user and current_user. User oid does not have the room for representing the difference among PUBLIC, NONE and 'not found' as the result of get_nameid_oid(), so members of NameId is exposed in foreigncmds.c and it gets a bit uglier. - Changes of related structs and grammar. Type of role member is changed to NameId in some of parser structs. AlterTableCmd.name has many other usage so I added new member NameId *newowner for exclusive usage. Type of OWNER clause of CREATE TABLESPACE is changed to RoleId. I suppose there's no particular reason that the non-terminal was name. Usage of public and none had been blocked for CREATE/RENAME USER in user.c but now it is blocked in gram.y - New function to resolve NameId New function get_nameid_oid() is added. It is an alternative of get_role_oid which can handle current_user and current_user properly. get_role_oid() still be used in many places having no direct relation to syntax. - Others No doc provided for now. regards, Adam Brightwell adam.brightw...@crunchydatasolutions.com writes: FWIW, I found the following in the archives: http://www.postgresql.org/message-id/15516.1038718...@sss.pgh.pa.us Now this is from 2002 and it appears it wasn't necessary to change at the time, but I haven't yet found anything else related (it's a big archive). Though, as I understand it, PUBLIC is now non-reserved as of SQL:2011 which might make a compelling argument to leave it as is? The current spec does list PUBLIC as a non-reserved keyword, but it also says (5.4 Names and identifiers syntax rules) 20) No authorization identifier shall specify PUBLIC. which, oddly enough, seems to license us to handle PUBLIC the way we are doing. OTOH, it lists CURRENT_USER as a reserved word, suggesting that they don't think the same type of hack should be used for that. I'd be inclined to leave the grammar as such alone (ie CURRENT_USER is a keyword, PUBLIC isn't). Changing that has more downside than upside, and we do have justification in the spec for treating the two cases differently. However, I agree that we should fix the subsequent processing so that current_user is not confused with CURRENT_USER. Sure, maybe. - PUBLIC should be left as it is, as an supecial identifier which cannot be used even if quoted under some syntax. - CURRENT_USER should be a kayword as it is, but we shouldn't inhibit it from being used as an identifier if quoted. SESSION_USER and USER should be treated in the same way. We don't want to use something other than string (prefixed by zero-byte) as a matter of course:). And resolving the name to roleid instantly in gram.y is not allowed for the reason shown upthread. So it is necessary to add a new member for the struct, say int special_role;:... Let me have more sane name for it :( - USER and CURRENT_ROLE are not needed for the syntax other than them which already uses them. I will work on this way. Let me know if something is not acceptable. -- Kyotaro Horiguchi NTT Open Source Software Center From f18d078d5e6e4005e803ecc954e59c899dbfd557 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp Date: Mon, 10 Nov 2014 19:08:42 +0900 Subject: [PATCH] ALTER USER CURRENT_USER v2 --- src/backend/catalog/aclchk.c | 13 ++- src/backend/commands/alter.c | 2 +- src/backend/commands/foreigncmds.c | 39 - src/backend/commands/schemacmds.c | 26 +- src/backend/commands/tablecmds.c | 2 +- src/backend/commands/tablespace.c | 2 +- src/backend/commands/user.c| 70 +++ src/backend/nodes/copyfuncs.c | 37 +--- src/backend/nodes/equalfuncs.c | 35 +---
Re: [HACKERS] alter user/role CURRENT_USER
All, I agree that we should probably seperate the concerns here. Personally, I like the idea of being able to say CURRENT_USER in utility commands to refer to the current user where a role would normally be expected, as I could see it simplifying things for some applications, but that's a new feature and independent of making role-vs-user cases more consistent. So, I've been doing a little digging and it would appear that the ALTER ROLE/USER consistency was brought up earlier in the year. http://www.postgresql.org/message-id/cadyruxmv-tvsbv7mttcs+qedny6xj1+krtzfowvuhdjc5mg...@mail.gmail.com It was returned with feedback in Commitfest 2014-06 and apparently lost steam: https://commitfest.postgresql.org/action/patch_view?id=1408 Tom put forward a suggestion for how to fix it: http://www.postgresql.org/message-id/21570.1408832...@sss.pgh.pa.us I have taken that patch and updated the documentation (attached) and ran it through some cursory testing. At any rate, this is probably a good starting point for those changes. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com diff --git a/doc/src/sgml/ref/alter_user.sgml b/doc/src/sgml/ref/alter_user.sgml new file mode 100644 index 58ae1da..ac05682 *** a/doc/src/sgml/ref/alter_user.sgml --- b/doc/src/sgml/ref/alter_user.sgml *** ALTER USER replaceable class=PARAMETER *** 38,47 ALTER USER replaceable class=PARAMETERname/replaceable RENAME TO replaceablenew_name/replaceable ! ALTER USER replaceable class=PARAMETERname/replaceable SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT } ! ALTER USER replaceable class=PARAMETERname/replaceable SET replaceableconfiguration_parameter/replaceable FROM CURRENT ! ALTER USER replaceable class=PARAMETERname/replaceable RESET replaceableconfiguration_parameter/replaceable ! ALTER USER replaceable class=PARAMETERname/replaceable RESET ALL /synopsis /refsynopsisdiv --- 38,47 ALTER USER replaceable class=PARAMETERname/replaceable RENAME TO replaceablenew_name/replaceable ! ALTER USER replaceable class=PARAMETERname/replaceable [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT } ! ALTER USER { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable FROM CURRENT ! ALTER USER { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET replaceableconfiguration_parameter/replaceable ! ALTER USER { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET ALL /synopsis /refsynopsisdiv diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y new file mode 100644 index 0de9584..d7c0586 *** a/src/backend/parser/gram.y --- b/src/backend/parser/gram.y *** static Node *makeRecursiveViewSelect(cha *** 230,236 AlterFdwStmt AlterForeignServerStmt AlterGroupStmt AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterSystemStmt AlterTableStmt AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt AlterForeignTableStmt ! AlterCompositeTypeStmt AlterUserStmt AlterUserMappingStmt AlterUserSetStmt AlterRoleStmt AlterRoleSetStmt AlterPolicyStmt AlterDefaultPrivilegesStmt DefACLAction AnalyzeStmt ClosePortalStmt ClusterStmt CommentStmt --- 230,236 AlterFdwStmt AlterForeignServerStmt AlterGroupStmt AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterSystemStmt AlterTableStmt AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt AlterForeignTableStmt ! AlterCompositeTypeStmt AlterUserMappingStmt AlterRoleStmt AlterRoleSetStmt AlterPolicyStmt AlterDefaultPrivilegesStmt DefACLAction AnalyzeStmt ClosePortalStmt ClusterStmt CommentStmt *** static Node *makeRecursiveViewSelect(cha *** 520,525 --- 520,527 %type str opt_existing_window_name %type boolean opt_if_not_exists + %type str role_or_user + /* * Non-keyword token types. These are hard-wired into the flex lexer. * They must be listed first so that their numeric codes do not depend on *** stmt : *** 756,763 | AlterTSConfigurationStmt | AlterTSDictionaryStmt | AlterUserMappingStmt - | AlterUserSetStmt - | AlterUserStmt | AnalyzeStmt | CheckPointStmt | ClosePortalStmt --- 758,763 *** CreateUserStmt: *** 1033,1042 * * Alter a postgresql DBMS role * */ AlterRoleStmt: ! ALTER ROLE RoleId opt_with AlterOptRoleList { AlterRoleStmt *n =
Re: [HACKERS] alter user/role CURRENT_USER
Hello, At Tue, 28 Oct 2014 09:05:20 -0400, Stephen Frost sfr...@snowman.net wrote in 20141028130520.gl28...@tamriel.snowman.net As well, the originally proposed RoleId_or_curruser suffers from the same issue. I'm going to go out on a limb here, but is it not possible to consider current_user, etc. reserved in the same sense that we do with PUBLIC and NONE and disallow users/roles from being created as them? Maybe we could in some future release, but we can't for back-branches so I'm afraid we're going to have to figure out how to fix this to work regardless. Zero-length identifiers are rejected in scanner so RoleId cannot be a valid pointer to a zero-length string. (NULL is used as PUBLIC in auth_ident) | postgres=# create role ; | ERROR: zero-length delimited identifier at or near | postgres=# create role U\00; | ERROR: invalid Unicode escape value at or near \00 As a dirty and quick hack we can use some integer values prfixed by single zero byte to represent special roles such as CURRENT_USER. | RoleId_or_curruser: RoleId{ $$ = $1; } | | CURRENT_USER { $$ = \x00\x01;}; | Oid ResolveRoleId(const char *name, bool missing_ok) | { | Oid roleid; | | if (name[0] == 0 name[1] == 1) | roleid = GetUserId(); This is ugly but needs no additional struct member or special logics. (Macros could make them look better.) Taking a step back, I'm still not sure I understand the need for this feature or the use case. It seems to have started as a potential fix to an inconsistency between ALTER USER and ALTER ROLE syntax (which I think I could see some value in). However, I think it has been taken beyond just resolving the inconsistency and started to cross over into feature creep. Is the intent simply to resolve inconsistencies between what is now an alias of another command? Or is it to add new functionality? I think the original proposal needs to be revisited and more time needs to be spent defining the scope and purpose of this patch. I agree that we should probably seperate the concerns here. Personally, I like the idea of being able to say CURRENT_USER in utility commands to refer to the current user where a role would normally be expected, as I could see it simplifying things for some applications, but that's a new feature and independent of making role-vs-user cases more consistent. I agree that role-vs-user compatibility is another problem. In a sense, the CURRENT_USER problem is also a separate problem but simplly spreading current current_user mechanism (which cannot allow using the words literally even if double-quoted) out to other commands is a kind of pandemic so it should be fixed before making current_user usable in other commands. From a view of comptibility (specification stability), fixing this problem could break someone's application if he/she uses current_user in the meaning of CURRENT_USER intentionally but I think it is least likely. As another problem, in the defenition of grantee, there is the following comment. | /* This hack lets us avoid reserving PUBLIC as a keyword*/ | if (strcmp($1, public) == 0) Please let me know the reason to avoid registering new keyword making the word unusable as an literal identifier, if any? PUBLIC and NONE ought to can be identifiers but in reality unusable because they are handled as keywords in literal form. Thses are fixed by making them real keywords. So if there's no particular reason, I will register new keywords PUBLIC and NONE as another patch. # However, I don't think that considerable number of people want # to do that.. On the other hand, pg_* as shcmea names and operators are cases simply of special names, which cannot be identifiers from the first so it should be as it is, I think. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Kyotaro, Zero-length identifiers are rejected in scanner so RoleId cannot be a valid pointer to a zero-length string. (NULL is used as PUBLIC in auth_ident) | postgres=# create role ; | ERROR: zero-length delimited identifier at or near | postgres=# create role U\00; | ERROR: invalid Unicode escape value at or near \00 Err... what? I'm not sure what you are getting at here? Why would we ever have/want a zero-length identifier for a RoleId? Seems to me that anything requiring a RoleId should be explicit. As a dirty and quick hack we can use some integer values prfixed by single zero byte to represent special roles such as CURRENT_USER. | RoleId_or_curruser: RoleId{ $$ = $1; } | | CURRENT_USER { $$ = \x00\x01;}; | Oid ResolveRoleId(const char *name, bool missing_ok) | { | Oid roleid; | | if (name[0] == 0 name[1] == 1) | roleid = GetUserId(); This is ugly but needs no additional struct member or special logics. (Macros could make them look better.) Yeah, that's pretty ugly. I think Alvaro's recommendation of having the production return a node with a name or flag is the better approach. | /* This hack lets us avoid reserving PUBLIC as a keyword*/ | if (strcmp($1, public) == 0) Please let me know the reason to avoid registering new keyword making the word unusable as an literal identifier, if any? FWIW, I found the following in the archives: http://www.postgresql.org/message-id/15516.1038718...@sss.pgh.pa.us Now this is from 2002 and it appears it wasn't necessary to change at the time, but I haven't yet found anything else related (it's a big archive). Though, as I understand it, PUBLIC is now non-reserved as of SQL:2011 which might make a compelling argument to leave it as is? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] alter user/role CURRENT_USER
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: | RoleId_or_curruser: RoleId{ $$ = $1; } | | CURRENT_USER { $$ = \x00\x01;}; [...] This is ugly but needs no additional struct member or special logics. (Macros could make them look better.) Yeah, that's pretty ugly. I think Alvaro's recommendation of having the production return a node with a name or flag is the better approach. That's more than just 'ugly', in my view. I don't think there's any reason to avoid making this into a node with a field that can be set to indicate it's something special if we're going to support this. The other idea which comes to mind is- could we try to actually resolve what the 'right' answer is here, instead of setting a special value and then having to detect and fix it later? Perhaps have a Oid+Rolename structure and then fill in the Oid w/ GetUserId(), if we're called with CURRENT_USER, and otherwise just populate the Rolename field and have code later which fills in the Oid if it's InvalidOid. Please let me know the reason to avoid registering new keyword making the word unusable as an literal identifier, if any? We really don't want to introduce new keywords without very good reason, and adding to the list of can't be used even if quoted is all but completely forbidden. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] alter user/role CURRENT_USER
Stephen Frost sfr...@snowman.net writes: The other idea which comes to mind is- could we try to actually resolve what the 'right' answer is here, instead of setting a special value and then having to detect and fix it later? No, absolutely not. Read the NOTES at the head of gram.y. Or if you need it spelled out: when we run the bison parser, we may not be inside a transaction at all, and even if we are, we aren't necessarily going to be seeing the same current user that will apply when the parsetree is ultimately executed. (Consider a query inside a plpgsql function, which might be called under multiple userids over the course of a session.) I think Alvaro's suggestion is a perfectly appropriate solution. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Adam Brightwell adam.brightw...@crunchydatasolutions.com writes: FWIW, I found the following in the archives: http://www.postgresql.org/message-id/15516.1038718...@sss.pgh.pa.us Now this is from 2002 and it appears it wasn't necessary to change at the time, but I haven't yet found anything else related (it's a big archive). Though, as I understand it, PUBLIC is now non-reserved as of SQL:2011 which might make a compelling argument to leave it as is? The current spec does list PUBLIC as a non-reserved keyword, but it also says (5.4 Names and identifiers syntax rules) 20) No authorization identifier shall specify PUBLIC. which, oddly enough, seems to license us to handle PUBLIC the way we are doing. OTOH, it lists CURRENT_USER as a reserved word, suggesting that they don't think the same type of hack should be used for that. I'd be inclined to leave the grammar as such alone (ie CURRENT_USER is a keyword, PUBLIC isn't). Changing that has more downside than upside, and we do have justification in the spec for treating the two cases differently. However, I agree that we should fix the subsequent processing so that current_user is not confused with CURRENT_USER. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Hello, thank you all for many comments. At the first, I removed changes for role-vs-user consistency and remove all added role named other than current_user. The followings are one-by-one answer for the comments so far, please let me know if I missed anything. - The necessity of the new function ResolveRoleId() It is very brief function but used in many place where the role name should be treated in the same way, so I think encapsulation is needed in some extent and in any form. It could be merged with get_role_oid. - About changes in foreigncmds.c I removed the refactoring using ResolveRoleId() in this patch. - RoleId_or_curruser separate from RoleId. There seems to be places where 'current_user' and like is not appropraite to occur such as CREATE USER. I don't mind to remove the non-terminal if it is needless consideration. - GRANT is not modified. I thought GRANT is not appropriate but it seems appropriate seeing your example. And grantee takes public. I changed GRANT/REVOKE to take current_user in this patch. - (not a comment) CREATE SCHEMA needed additonal aid Schema name can be omitted in CREATE SCHEMA and role name is used for it, so CREATE SCHEMA AUTHORIZATION current_user crates the schema current_user in the previous patch. This should be the real name of current_user. This patch is for reviewing at a glance for food of discussion and tested very briefly (and what is worse, it might even not be applicable). I'll repost more refined version in this way and other portions. At Tue, 28 Oct 2014 12:16:13 -0400, Stephen Frost sfr...@snowman.net wrote in 20141028161613.gt28...@tamriel.snowman.net * Kevin Grittner (kgri...@ymail.com) wrote: It is very important that a quoted identifier not be treated as a keyword. I would be very interested in seeing that list, and in ensuring that it doesn't get any longer. It's object specific and not handled through the grammar, so that gets pretty annoying. :/ The ones I could find by a quick look through backend/commands are: roles public none schemas pg_* operator = (throws a warning at least) There may be other cases that my quick review didn't find, of course. Hmm... This seems to be another issue, though. I'll be careful not to make it worse.. regards, -- Kyotaro Horiguchi NTT Open Source Software Center From ebe2d8b5549eddbd073b65aabe15af9299b948f6 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp Date: Wed, 29 Oct 2014 17:38:46 +0900 Subject: [PATCH] CURRENT_USER_WIP_v1 --- src/backend/commands/alter.c | 2 +- src/backend/commands/schemacmds.c | 20 ++- src/backend/commands/tablecmds.c | 2 +- src/backend/commands/user.c | 48 ++ src/backend/parser/gram.y | 71 --- src/include/commands/dbcommands.h | 1 + src/include/commands/user.h | 1 + 7 files changed, 93 insertions(+), 52 deletions(-) diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index c9a9baf..1f598f6 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -678,7 +678,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid) Oid ExecAlterOwnerStmt(AlterOwnerStmt *stmt) { - Oid newowner = get_role_oid(stmt-newowner, false); + Oid newowner = ResolveRoleId(stmt-newowner, false); switch (stmt-objectType) { diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c index 03f5514..d67789a 100644 --- a/src/backend/commands/schemacmds.c +++ b/src/backend/commands/schemacmds.c @@ -23,8 +23,10 @@ #include catalog/namespace.h #include catalog/objectaccess.h #include catalog/pg_namespace.h +#include catalog/pg_authid.h #include commands/dbcommands.h #include commands/schemacmds.h +#include commands/user.h #include miscadmin.h #include parser/parse_utilcmd.h #include tcop/utility.h @@ -59,10 +61,26 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString) * Who is supposed to own the new schema? */ if (authId) - owner_uid = get_role_oid(authId, false); + owner_uid = ResolveRoleId(authId, false); else owner_uid = saved_uid; + /* Use the name for the owner_uid as the schema name if it is omitted */ + if (!schemaName) + { + HeapTuple tuple; + + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(owner_uid)); + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg(roleid %d does not exist, owner_uid))); + + schemaName = + strdup(((Form_pg_authid) GETSTRUCT(tuple))-rolname.data); + ReleaseSysCache(tuple); + } + /* * To create a schema, must have schema-create privilege on the current * database and must be able to become the target role (this does not diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index ecdff1e..0b82765 100644 --- a/src/backend/commands/tablecmds.c +++
Re: [HACKERS] alter user/role CURRENT_USER
+RoleId:CURRENT_USER{ $$ = current_user;} + | USER { $$ = current_user;} + | CURRENT_ROLE { $$ = current_user;} + | SESSION_USER { $$ = session_user;} This is kind of ugly, and it means you can't distinguish between a CURRENT_USER keyword and a quoted user name current_user. You are right. I'm not sure I have an opinion on how clean it is, but FWIW, it is similar to the way that the 'auth_ident' type in the grammar is defined (though, not to imply that it makes it right). As well, the originally proposed RoleId_or_curruser suffers from the same issue. I'm going to go out on a limb here, but is it not possible to consider current_user, etc. reserved in the same sense that we do with PUBLIC and NONE and disallow users/roles from being created as them? I mean, after all, they *are* already reserved keywords. Perhaps there is a very good reason why we wouldn't want to do that and I am sure there is since they have not been treated this way thus far. If anyone would like to share why, then I'd very much appreciate the lesson. It's a legitimate user name, so the behavior of the following now changes: CREATE ROLE current_user; Well, it does allow this one. ALTER ROLE current_user SET work_mem='10MB'; However, you are right, it does interfere with this command (and pretty much ALTER ROLE in general). :-/ Not sure what to offer there. ALTER USER USER does not seem like sane syntax that should be accepted. I think that I agree, I can't imagine this being acceptable. Taking a step back, I'm still not sure I understand the need for this feature or the use case. It seems to have started as a potential fix to an inconsistency between ALTER USER and ALTER ROLE syntax (which I think I could see some value in). However, I think it has been taken beyond just resolving the inconsistency and started to cross over into feature creep. Is the intent simply to resolve inconsistencies between what is now an alias of another command? Or is it to add new functionality? I think the original proposal needs to be revisited and more time needs to be spent defining the scope and purpose of this patch. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] alter user/role CURRENT_USER
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: +RoleId:CURRENT_USER{ $$ = current_user;} + | USER { $$ = current_user;} + | CURRENT_ROLE { $$ = current_user;} + | SESSION_USER { $$ = session_user;} This is kind of ugly, and it means you can't distinguish between a CURRENT_USER keyword and a quoted user name current_user. You are right. I'm not sure I have an opinion on how clean it is, but FWIW, it is similar to the way that the 'auth_ident' type in the grammar is defined (though, not to imply that it makes it right). No, it's not right and it's an existing problem. :( =*# create extension postgres_fdw; CREATE EXTENSION =# create server s1 foreign data wrapper postgres_fdw ; CREATE SERVER =*# create user mapping for current_user server s1; CREATE USER MAPPING =*# table pg_user_mappings; -[ RECORD 1 ]- umid | 24623 srvid | 24622 srvname | s1 umuser| 16384 usename | sfrost umoptions | As well, the originally proposed RoleId_or_curruser suffers from the same issue. I'm going to go out on a limb here, but is it not possible to consider current_user, etc. reserved in the same sense that we do with PUBLIC and NONE and disallow users/roles from being created as them? Maybe we could in some future release, but we can't for back-branches so I'm afraid we're going to have to figure out how to fix this to work regardless. I mean, after all, they *are* already reserved keywords. Perhaps there is a very good reason why we wouldn't want to do that and I am sure there is since they have not been treated this way thus far. If anyone would like to share why, then I'd very much appreciate the lesson. You can still double-quote reserved words and use them in general. What we're talking about here are cases where a word can't be used even if it's double-quoted, and we try really hard to keep those cases at an absolute minimum. We should also really be keeping a list of those cases somewhere, now that I think about it.. Taking a step back, I'm still not sure I understand the need for this feature or the use case. It seems to have started as a potential fix to an inconsistency between ALTER USER and ALTER ROLE syntax (which I think I could see some value in). However, I think it has been taken beyond just resolving the inconsistency and started to cross over into feature creep. Is the intent simply to resolve inconsistencies between what is now an alias of another command? Or is it to add new functionality? I think the original proposal needs to be revisited and more time needs to be spent defining the scope and purpose of this patch. I agree that we should probably seperate the concerns here. Personally, I like the idea of being able to say CURRENT_USER in utility commands to refer to the current user where a role would normally be expected, as I could see it simplifying things for some applications, but that's a new feature and independent of making role-vs-user cases more consistent. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] alter user/role CURRENT_USER
Stephen Frost sfr...@snowman.net wrote: You can still double-quote reserved words and use them in general. What we're talking about here are cases where a word can't be used even if it's double-quoted, and we try really hard to keep those cases at an absolute minimum. We should also really be keeping a list of those cases somewhere, now that I think about it.. It is very important that a quoted identifier not be treated as a keyword. I would be very interested in seeing that list, and in ensuring that it doesn't get any longer. I agree that we should probably seperate the concerns here. Personally, I like the idea of being able to say CURRENT_USER in utility commands to refer to the current user where a role would normally be expected, as I could see it simplifying things for some applications, but that's a new feature and independent of making role-vs-user cases more consistent. Yeah, let's not mix those in the same patch. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
On Tue, Oct 28, 2014 at 2:40 AM, Adam Brightwell adam.brightw...@crunchydatasolutions.com wrote: Taking a step back, I'm still not sure I understand the need for this feature or the use case. It seems to have started as a potential fix to an inconsistency between ALTER USER and ALTER ROLE syntax (which I think I could see some value in). However, I think it has been taken beyond just resolving the inconsistency and started to cross over into feature creep. Is the intent simply to resolve inconsistencies between what is now an alias of another command? Or is it to add new functionality? I think the original proposal needs to be revisited and more time needs to be spent defining the scope and purpose of this patch. +1. I've been reading this thread with some bemusement, but couldn't find a way articulate what you just said nearly as well as you just said it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
* Kevin Grittner (kgri...@ymail.com) wrote: Stephen Frost sfr...@snowman.net wrote: You can still double-quote reserved words and use them in general. What we're talking about here are cases where a word can't be used even if it's double-quoted, and we try really hard to keep those cases at an absolute minimum. We should also really be keeping a list of those cases somewhere, now that I think about it.. It is very important that a quoted identifier not be treated as a keyword. I would be very interested in seeing that list, and in ensuring that it doesn't get any longer. It's object specific and not handled through the grammar, so that gets pretty annoying. :/ The ones I could find by a quick look through backend/commands are: roles public none schemas pg_* operator = (throws a warning at least) There may be other cases that my quick review didn't find, of course. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] alter user/role CURRENT_USER
Marti Raudsepp wrote: On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp wrote: - 0001-ALTER-ROLE-CURRENT_USER_v2.patch - the patch. +RoleId:CURRENT_USER{ $$ = current_user;} + | USER { $$ = current_user;} + | CURRENT_ROLE { $$ = current_user;} + | SESSION_USER { $$ = session_user;} This is kind of ugly, and it means you can't distinguish between a CURRENT_USER keyword and a quoted user name current_user. It's a legitimate user name, so the behavior of the following now changes: CREATE ROLE current_user; ALTER ROLE current_user SET work_mem='10MB'; There ought to be a better way to represent this than using magic string values. Agreed. Since the current_user disease has already infected the USER MAPPING stuff, I think we need to solve that problem -- how about having this production return a new node which has either a string name or flags for the various acceptable keywords? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
All, I just ran through the patch giving it a good once over, some items to address/consider/discuss: * Trailing whitespace. * Why are you making changes in foreigncmds.c? These seem like unnecessary changes. I see that you are trying to consolidate but this refactor seems potentially out of scope. * To the above point, is ResolveRoleId really necessary? Also, I'm not sure I agree with passing in the tuple, I don't see any reason to pull that look up into this function. Also, couldn't you simply just add a short circuit in get_role_oid to check for current_user and return GetUserId() and similar for session_user? * In gram.y, is there really a need to have a separate RoleId_or_curruser? Why not: -RoleId:NonReservedWord { $$ = $1; }; +RoleId:CURRENT_USER{ $$ = current_user;} + | USER { $$ = current_user;} + | CURRENT_ROLE { $$ = current_user;} + | SESSION_USER { $$ = session_user;} + | NonReservedWord { $$ = $1; } + ; This would certainly reduce the number of changes to the grammar but might also help with covering the cases mentioned by Rushabh? Also are there cases when we don't want CURRENT_USER to be considered a RoleId? * The following seems like an unnecessary change: - | RoleId { $$ = (strcmp($1, public) == 0) ? NULL : $1; } + RoleId { $$ = (strcmp($1, public) == 0) ? + NULL : $1; } * Why is htup.h included in dbcommands.h? * What's up with the following in user.c, did you replace tab with spaces? - HeapTuple roletuple; + HeapTuple roletuple; -- Not working alter default privileges for role current_user grant SELECT ON TABLES TO current_user ; -- Not working grant user1 TO current_user; Agreed. The latter of the two seems like an interesting case to me though. But they both kind of jump out at me as potential for security concerns (but perhaps not given the role id checks, etc). At any rate, I'd still expect these to behave accordingly with notification/error messages when appropriate. Their might few more syntax like this. I think this can be covered inherently by the grammar changes recommended above (if such changes are appropriate). Though, I'd also recommend investigating which commands are affected via the grammar (or RoleId) and then making sure to update the regression tests and the documentation accordingly. I understand that patch is sightly getting bigger and complex then what it was originally proposed. IMHO, I think this patch has become more complex than is required. Before going back to more review on latest patch I would like to understand the requirement of this new feature. Also would like others to comment on where/how we should restrict this feature ? I think this is a fair request. I believe I can see the potential convenience of these changes, however, I'm uncertain of the necessity of them and whether or not it opens any security concerns (again, perhaps not, but I think it is worth asking). Also, how would this affect items like auditing? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] alter user/role CURRENT_USER
On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp wrote: - 0001-ALTER-ROLE-CURRENT_USER_v2.patch - the patch. +RoleId:CURRENT_USER{ $$ = current_user;} + | USER { $$ = current_user;} + | CURRENT_ROLE { $$ = current_user;} + | SESSION_USER { $$ = session_user;} This is kind of ugly, and it means you can't distinguish between a CURRENT_USER keyword and a quoted user name current_user. It's a legitimate user name, so the behavior of the following now changes: CREATE ROLE current_user; ALTER ROLE current_user SET work_mem='10MB'; There ought to be a better way to represent this than using magic string values. It accepts CURRENT_USER/USER/CURRENT_ROLE/SESSION_USER. On a stylistic note, do we really want to support the alternative spellings of CURRENT_USER, like CURRENT_ROLE and USER? The SQL standard is well-hated for inventing more keywords than necessary. There is no precedent for using/allowing these aliases in PostgreSQL DDL commands, and ALTER USER ROLE aren't SQL standard anyway. So maybe we should stick with just accepting one canonical syntax instead. I think the word USER may reasonably arise from an editing mistake, ALTER USER USER does not seem like sane syntax that should be accepted. From your original email: - Modify syntax of ALTER USER so as to be an alias of ALTER ROLE. - Added ALL syntax as user name to ALTER USER. But should ALTER USER ALL and ALTER ROLE ALL really do the same thing? A user is a role with the LOGIN option. Every user is a role, but not every role is a user. I suspect that ambiguity was why ALTER USER ALL wasn't originally implemented. Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Marti Raudsepp wrote On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI lt; horiguchi.kyotaro@.co gt; wrote: But should ALTER USER ALL and ALTER ROLE ALL really do the same thing? A user is a role with the LOGIN option. Every user is a role, but not every role is a user. I suspect that ambiguity was why ALTER USER ALL wasn't originally implemented. There is no system level distinction here - only semantic and only during the issuance of a CREATE without specifying an explicit value for LOGIN/NOLGIN. The documentation states user and group are aliases for ROLE, not subsets that require or disallow login privileges. I am OK with not making them true aliases in order to minimize user confusion w.r.t. the semantics implied by user and group but then I'd submit we simply note those two forms as deprecated in favor of role and that all new role-based functionality will only be attached to role-based commands. Since all of user, current_user and session_user have special syntactic consideration in SQL [1] I'd be generally in favor of trying to keep that dynamic intact while implementing this feature. And for the same reason I would not allow current_role as a keyword. We didn't add a current_role function but instead chose to rely on the standard keywords even when we introduced ROLE. I'm not clear on the keyword confusion since while current_user is a valid literal it requires quotation while the token current_user does not. 1. http://www.postgresql.org/docs/9.4/static/functions-info.html David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/alter-user-role-CURRENT-USER-tp5822520p5824528.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Thanks Kyotaro, I just did quickly looked at the patch and it does cover more syntax then earlier patch. But still if doesn't not cover the all the part of syntax where we can use CURRENT_USER/CURRENT_ROLE/USER/SESSION_USER. For example: -- Not working alter default privileges for role current_user grant SELECT ON TABLES TO current_user ; -- Not working grant user1 TO current_user; Their might few more syntax like this. I understand that patch is sightly getting bigger and complex then what it was originally proposed. Before going back to more review on latest patch I would like to understand the requirement of this new feature. Also would like others to comment on where/how we should restrict this feature ? On Fri, Oct 24, 2014 at 1:59 PM, Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp wrote: Hi, here is the revised patch. Attached files are the followings - 0001-ALTER-ROLE-CURRENT_USER_v2.patch - the patch. - testset.tar.bz2 - test set. Run by typing 'make check' as a superuser of the running postgreSQL server. It creates testdb and some roles. Documents are not edited this time. Considering your comments, I found more points to modify. - CREATE SCHEMA (IF NOT EXISTS) .. AUTHORIZATION role ... - ALTER AGGREAGE/COLLATION/etc... OWNER TO role - CREATE/ALTER/DROP USER MAPPING FOR role SERVER .. GRANT/REVOKE also takes role as an arguemnt but CURRENT_USER and the similar keywords seem to be useless for them. Finally, the new patch modifies the following points. In gram.y, - RoleId's are replaced with RoleId_or_curruser in more places. It accepts CURRENT_USER/USER/CURRENT_ROLE/SESSION_USER. - ALTER USER ALL syntax is added. (not changed from the previous patch) - The non-terminal auth_ident now uses RoleId_or_curruser instead of RoleId. This affects CREATE/ALTER/DROP USER MAPPING In user.c, new function ResolveRoleId() is added and used for all role ID resolutions that correspond to the syntax changes in parser. It is AlterRole() in user.c. In foreigncmds.c, GetUserOidFromMapping() is removed and ResolveRoleId is used instead. In alter.c and tablecmds.c, all calls to get_role_oid() correspond the the grammer change were replaced with ResolveRoleId(). The modifications are a bit complicated so I provided a comprehensive test set. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Rushabh Lathia
Re: [HACKERS] alter user/role CURRENT_USER
Hi, here is the revised patch. Attached files are the followings - 0001-ALTER-ROLE-CURRENT_USER_v2.patch - the patch. - testset.tar.bz2 - test set. Run by typing 'make check' as a superuser of the running postgreSQL server. It creates testdb and some roles. Documents are not edited this time. Considering your comments, I found more points to modify. - CREATE SCHEMA (IF NOT EXISTS) .. AUTHORIZATION role ... - ALTER AGGREAGE/COLLATION/etc... OWNER TO role - CREATE/ALTER/DROP USER MAPPING FOR role SERVER .. GRANT/REVOKE also takes role as an arguemnt but CURRENT_USER and the similar keywords seem to be useless for them. Finally, the new patch modifies the following points. In gram.y, - RoleId's are replaced with RoleId_or_curruser in more places. It accepts CURRENT_USER/USER/CURRENT_ROLE/SESSION_USER. - ALTER USER ALL syntax is added. (not changed from the previous patch) - The non-terminal auth_ident now uses RoleId_or_curruser instead of RoleId. This affects CREATE/ALTER/DROP USER MAPPING In user.c, new function ResolveRoleId() is added and used for all role ID resolutions that correspond to the syntax changes in parser. It is AlterRole() in user.c. In foreigncmds.c, GetUserOidFromMapping() is removed and ResolveRoleId is used instead. In alter.c and tablecmds.c, all calls to get_role_oid() correspond the the grammer change were replaced with ResolveRoleId(). The modifications are a bit complicated so I provided a comprehensive test set. regards, -- Kyotaro Horiguchi NTT Open Source Software Center From f55494a49b6d4c7eb32665ea9cc63634f5000c99 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp Date: Tue, 9 Sep 2014 19:26:24 +0900 Subject: [PATCH] ALTER ROLE CURRENT_USER --- src/backend/commands/alter.c | 2 +- src/backend/commands/foreigncmds.c | 25 ++-- src/backend/commands/schemacmds.c | 3 +- src/backend/commands/tablecmds.c | 2 +- src/backend/commands/user.c| 56 ++- src/backend/parser/gram.y | 78 ++ src/include/commands/dbcommands.h | 1 + src/include/commands/user.h| 2 + 8 files changed, 96 insertions(+), 73 deletions(-) diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index c9a9baf..15f254e 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -678,7 +678,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid) Oid ExecAlterOwnerStmt(AlterOwnerStmt *stmt) { - Oid newowner = get_role_oid(stmt-newowner, false); + Oid newowner = ResolveRoleId(stmt-newowner, false, NULL); switch (stmt-objectType) { diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c index ab4ed6c..9878fca 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -27,6 +27,7 @@ #include catalog/pg_type.h #include catalog/pg_user_mapping.h #include commands/defrem.h +#include commands/user.h #include foreign/fdwapi.h #include foreign/foreign.h #include miscadmin.h @@ -198,24 +199,6 @@ transformGenericOptions(Oid catalogId, /* - * Convert the user mapping user name to OID - */ -static Oid -GetUserOidFromMapping(const char *username, bool missing_ok) -{ - if (!username) - /* PUBLIC user mapping */ - return InvalidOid; - - if (strcmp(username, current_user) == 0) - /* map to the owner */ - return GetUserId(); - - /* map to provided user */ - return get_role_oid(username, missing_ok); -} - -/* * Internal workhorse for changing a data wrapper's owner. * * Allow this only for superusers; also the new owner must be a @@ -1099,7 +1082,7 @@ CreateUserMapping(CreateUserMappingStmt *stmt) rel = heap_open(UserMappingRelationId, RowExclusiveLock); - useId = GetUserOidFromMapping(stmt-username, false); + useId = ResolveRoleId(stmt-username, false, NULL); /* Check that the server exists. */ srv = GetForeignServerByName(stmt-servername, false); @@ -1194,7 +1177,7 @@ AlterUserMapping(AlterUserMappingStmt *stmt) rel = heap_open(UserMappingRelationId, RowExclusiveLock); - useId = GetUserOidFromMapping(stmt-username, false); + useId = ResolveRoleId(stmt-username, false, NULL); srv = GetForeignServerByName(stmt-servername, false); umId = GetSysCacheOid2(USERMAPPINGUSERSERVER, @@ -1276,7 +1259,7 @@ RemoveUserMapping(DropUserMappingStmt *stmt) Oid umId; ForeignServer *srv; - useId = GetUserOidFromMapping(stmt-username, stmt-missing_ok); + useId = ResolveRoleId(stmt-username, stmt-missing_ok, NULL); srv = GetForeignServerByName(stmt-servername, true); if (stmt-username !OidIsValid(useId)) diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c index 03f5514..4f97f23 100644 --- a/src/backend/commands/schemacmds.c +++ b/src/backend/commands/schemacmds.c @@ -25,6 +25,7 @@ #include catalog/pg_namespace.h #include commands/dbcommands.h #include
Re: [HACKERS] alter user/role CURRENT_USER
Thank you for reviewing, 2014 13:10:57 +0530, Rushabh Lathia rushabh.lat...@gmail.com wrote in cagpqqf0kdfajizx0vca_-wazwu+xj5mdnl-hgg1sez9aw3c...@mail.gmail.com I gone through patch and here is the review for this patch: .) patch go applied on master branch with patch -p1 command (git apply failed) .) regression make check run fine .) testcase coverage is missing in the patch .) Over all coding/patch looks good. Few comments: 1) Any particular reason for not adding SESSION_USER/USER also made usable for this command ? No particular reson. This patch was the first step and if this is the adequate way and adding them is desirable, I will add them. 2) I think RoleId_or_curruser can be used for following role: /* ALTER TABLE name OWNER TO RoleId */ | OWNER TO RoleId Good catch. I'll try it. 3) In the documentation patch, added for CURRENT_USER but CURRENT_ROLE is missing. Mmm.. I didn't added CURRENT_ROLE in the previous patch. I suppose CURRENT_ROLE is an implicit alias of CURRENT_USER because it is not explained in the page below in spite of existing in syntax. http://www.postgresql.org/docs/9.4/static/functions-info.html and it is currently usable only in expressions, so the following SQL failed and all syntax using auth_ident will fail. postgres=# CREATE USER MAPPING FOR CURRENT_ROLE SERVER s1; ERROR: syntax error at or near current_role share/doc/html/sql-createusermapping.html | Synopsis | | CREATE USER MAPPING FOR { user_name | USER | CURRENT_USER | PUBLIC } I don't know why the 'USER' is added here, but anyway I can add 'CURRENT_ROLE' there in gram.y but it seems not necessary to add it to document. Ok, I'll modify this patch so that, - Make CURRENT_USER/ROLE usable in TABLE OWNER TO. and since adding CURRENT_ROLE is the another thing, I'll do the following things as additional patch. - Add USER, CURRENT_ROLE and SESSION_* for the place where CURRENT_USER is usable now. auth_ident and RoleId_or_curruser. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
Hello, Kyotaro, Food for thought. Couldn't you reduce the following block: + if (strcmp(stmt-role, current_user) == 0) + { + roleid = GetUserId(); + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid)); + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg(roleid %d does not exist, roleid))); + } + else + { + tuple = SearchSysCache1(AUTHNAME, PointerGetDatum(stmt-role)); + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg(role \%s\ does not exist, stmt-role))); To: if (strcmp(stmt-role, current_user) == 0) roleid = GetUserId(); else roleid = get_role_oid(stmt-role, false); tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid)); if (!HeapTupleIsValid(tuple)) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg(roleid %d does not exist, roleid))); I think this makes it a bit cleaner. It also reuses existing code as 'get_role_oid()' already does a valid role name check and will raise the proper error. Year, far cleaner. I missed the function. Thank you. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] alter user/role CURRENT_USER
I gone through patch and here is the review for this patch: .) patch go applied on master branch with patch -p1 command (git apply failed) .) regression make check run fine .) testcase coverage is missing in the patch .) Over all coding/patch looks good. Few comments: 1) Any particular reason for not adding SESSION_USER/USER also made usable for this command ? 2) I think RoleId_or_curruser can be used for following role: /* ALTER TABLE name OWNER TO RoleId */ | OWNER TO RoleId 3) In the documentation patch, added for CURRENT_USER but CURRENT_ROLE is missing. On Fri, Oct 10, 2014 at 1:57 PM, Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp wrote: Hello, on the way considering alter role set .., I found that ALTER ROLE/USER cannot take CURRENT_USER as the role name. In addition to that, documents of ALTER ROLE / USER are inconsistent with each other in spite of ALTER USER is said to be an alias for ALTER ROLE. Plus, ALTER USER cannot take ALL as user name although ALTER ROLE can. This patch does following things, - ALTER USER/ROLE now takes CURRENT_USER as user name. - Rewrite sysnopsis of the documents for ALTER USER and ALTER ROLE so as to they have exactly same syntax. - Modify syntax of ALTER USER so as to be an alias of ALTER ROLE. - Added CURRENT_USER/CURRENT_ROLE syntax to both. - Added ALL syntax as user name to ALTER USER. - Added IN DATABASE syntax to ALTER USER. x Integrating ALTER ROLE/USER syntax could not be done because of shift/reduce error of bison. x This patch contains no additional regressions. Is it needed? SESSION_USER/USER also can be made usable for this command, but this patch doesn't so (yet). regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Rushabh Lathia
Re: [HACKERS] alter user/role CURRENT_USER
Kyotaro, Food for thought. Couldn't you reduce the following block: + if (strcmp(stmt-role, current_user) == 0) + { + roleid = GetUserId(); + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid)); + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg(roleid %d does not exist, roleid))); + } + else + { + tuple = SearchSysCache1(AUTHNAME, PointerGetDatum(stmt-role)); + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg(role \%s\ does not exist, stmt-role))); To: if (strcmp(stmt-role, current_user) == 0) roleid = GetUserId(); else roleid = get_role_oid(stmt-role, false); tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid)); if (!HeapTupleIsValid(tuple)) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg(roleid %d does not exist, roleid))); I think this makes it a bit cleaner. It also reuses existing code as 'get_role_oid()' already does a valid role name check and will raise the proper error. -Adam On Mon, Oct 20, 2014 at 3:40 AM, Rushabh Lathia rushabh.lat...@gmail.com wrote: I gone through patch and here is the review for this patch: .) patch go applied on master branch with patch -p1 command (git apply failed) .) regression make check run fine .) testcase coverage is missing in the patch .) Over all coding/patch looks good. Few comments: 1) Any particular reason for not adding SESSION_USER/USER also made usable for this command ? 2) I think RoleId_or_curruser can be used for following role: /* ALTER TABLE name OWNER TO RoleId */ | OWNER TO RoleId 3) In the documentation patch, added for CURRENT_USER but CURRENT_ROLE is missing. On Fri, Oct 10, 2014 at 1:57 PM, Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp wrote: Hello, on the way considering alter role set .., I found that ALTER ROLE/USER cannot take CURRENT_USER as the role name. In addition to that, documents of ALTER ROLE / USER are inconsistent with each other in spite of ALTER USER is said to be an alias for ALTER ROLE. Plus, ALTER USER cannot take ALL as user name although ALTER ROLE can. This patch does following things, - ALTER USER/ROLE now takes CURRENT_USER as user name. - Rewrite sysnopsis of the documents for ALTER USER and ALTER ROLE so as to they have exactly same syntax. - Modify syntax of ALTER USER so as to be an alias of ALTER ROLE. - Added CURRENT_USER/CURRENT_ROLE syntax to both. - Added ALL syntax as user name to ALTER USER. - Added IN DATABASE syntax to ALTER USER. x Integrating ALTER ROLE/USER syntax could not be done because of shift/reduce error of bison. x This patch contains no additional regressions. Is it needed? SESSION_USER/USER also can be made usable for this command, but this patch doesn't so (yet). regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Rushabh Lathia -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com