Re: [HACKERS] alter user/role CURRENT_USER
Hello, At Thu, 30 Apr 2015 17:12:25 -0300, Alvaro Herrera 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 ON > > # TO " looks a bit wired, it might be better be to > > # be "ON APPLY TO " 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 TO " and "REVOKE FROM " are > > the modern equivalents of the deprecated syntaxes "ALTER > > ADD USER " and "ALTER DROP USER " > > 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 ON > # TO " looks a bit wired, it might be better be to > # be "ON APPLY TO " 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 TO " and "REVOKE FROM " are > the modern equivalents of the deprecated syntaxes "ALTER > ADD USER " and "ALTER DROP USER " > 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 TO syntax. At Mon, 9 Mar 2015 15:50:32 -0300, Alvaro Herrera 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 ON # TO " looks a bit wired, it might be better be to # be "ON APPLY TO " 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 TO " and "REVOKE FROM " are the modern equivalents of the deprecated syntaxes "ALTER ADD USER " and "ALTER DROP USER " 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 ON ..." "GRANT 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 ON TO properly rejects CURRENT_USER as . 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 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 TO " and "REVOKE FROM " 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 the syntax of GRANT command to accept CURRENT_ROLE and SESSION_ROLE as other similar command does.
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 -ALTER GROUP group_name ADD USER user_name [, ... ] -ALTER GROUP group_name DROP USER user_name [, ... ] +ALTER GROUP { group_name | CURRENT_USER | SESSION_USER } ADD USER user_name [, ... ] +ALTER GROUP { group_name | CURRENT_USER | SESSION_USER } DROP USER user_name [, ... ] -ALTER GROUP group_name RENAME TO new_name +ALTER GROUP { group_name | CURRENT_USER | SESSION_USER } RENAME TO new_name 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 -ALTER ROLE name [ [ WITH ] option [ ... ] ] +ALTER ROLE { name | CURRENT_USER | SESSION_USER } [ WITH ] option [ ... ] where option can be: @@ -39,10 +39,10 @@ ALTER ROLE name [ [ WITH ] name RENAME TO new_name -ALTER ROLE name [ IN DATABASE database_name ] SET configuration_parameter { TO | = } { value | DEFAULT } -ALTER ROLE { name | ALL } [ IN DATABASE database_name ] SET configuration_parameter FROM CURRENT -ALTER ROLE { name | ALL } [ IN DATABASE database_name ] RESET configuration_parameter -ALTER ROLE { name | ALL } [ IN DATABASE database_name ] RESET ALL +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 @@ -129,6 +129,25 @@ ALTER ROLE { name | ALL } [ IN DATA + CURRENT_USER + + +Indicates to +Alter the current user instead of a specifically named role. + + + + + + SESSION_USER + + +Alter the current session role instead of a specifically named role. + + + + + SUPERUSER NOSUPERUSER CREATEDB diff --git a/doc/src/sgml/ref/alter_user.sgml b/doc/src/sgml/ref/alter_user.sgml index 58ae1da..628729d 100644 --- a/doc/src/sgml/ref/alter_user.sgml +++ b/doc/src/sgml/ref/alter_user.sgml @@ -21,7 +21,7 @@ PostgreSQL documentation -ALTER USER name [ [ WITH ] option [ ... ] ] +ALTER USER { name | CURRENT_USER | SESSION_USER } [ WITH ] option [ ... ] where option can be: @@ -38,10 +38,10 @@ ALTER USER name [ [ WITH ] name RENAME TO new_name -ALTER USER name SET configuration_parameter { TO | = } { value | DEFAULT } -ALTER USER name SET configuration_parameter FROM CURRENT -ALTER USER name RESET configuration_parameter -ALTER USER name RESET ALL +ALTER USER { name | CURRENT_USER | SESSION_USER } SET configuration_parameter { TO | = } { value | DEFAULT } +ALTER USER { name | CURRENT_USER | SESSION_USER } SET configurati
Re: [HACKERS] alter user/role CURRENT_USER
Actually this is better -- I added token location tracking, and changed RoleId to use RoleSpec which means it can throw errors with locations when "public" or "none" are specified. I think the checks for public/none in CreateRole and AlterRole are dead code now. -- Á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..e88c8c3 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -421,22 +421,25 @@ ExecuteGrantStmt(GrantStmt *stmt) istmt.behavior = stmt->behavior; /* - * 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. + * Convert the RoleSpec list into an Oid list. Note that at this point + * 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); + RoleSpec *grantee = (RoleSpec *) lfirst(cell); + Oid grantee_uid; - 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->roletype) + { + case ROLESPEC_PUBLIC: +grantee_uid = ACL_ID_PUBLIC; +break; + default: +grantee_uid = get_rolespec_oid((Node *) grantee, false); +break; + } + istmt.grantees = lappend_oid(istmt.grantees, grantee_uid); } /* @@ -904,22 +907,25 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt) iacls.behavior = action->behavior; /* - * 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. + * Convert the RoleSpec list into an Oid list. Note that at this point + * 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); + RoleSpec *grantee = (RoleSpec *) lfirst(cell); + Oid grantee_uid; - 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->roletype) + { + case ROLESPEC_PUBLIC: +grantee_uid = ACL_ID_PUBLIC; +break; + default: +grantee_uid = get_rolespec_oid((Node *) grantee, 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 objid, Oid nspOid) Oid ExecAlterOwnerStmt(AlterOwnerStmt *stmt) { - Oid newowner = get_role_oid(stmt->newowner, false); + Oid newowner = get_rolespec_oid(stmt->newowner, false); switch (stmt->objectType) { diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c index 3b95552..2a8b2a0 100644 --- a/src/backend/commands/extension.c +++ b/src/backend/commands/extension.c @@ -1370,7 +1370,7 @@ CreateExtension(CreateExtensionStmt *stmt) CreateSchemaStmt *csstmt = makeNode(CreateSchemaStmt); csstmt->schemaname = schemaName; - csstmt->authid = NULL; /* will be created by current user */ + csstmt->authrole = NULL; /* will be created by current user */ csstmt->schemaElts = NIL; csstmt->if_not_exists = false; CreateSchemaCommand(csstmt, NULL); diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c index 537e31c..adf4c79 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -198,24 +198,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 @@ -1148,10 +1130,14
Re: [HACKERS] alter user/role CURRENT_USER
Alvaro Herrera wrote: > Kyotaro HORIGUCHI wrote: > 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. I tried that but it's way too messy, so I readded them. > 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 later noticed that you had already submitted a test.sql file, so I adopted it as rolenames.sql and added it to the schedule files. I still have to read through the results and make sure they make sense, so the expected file is not in this patch. I made some more changes to the code; unless the tests uncover something ugly, the code in this patch is what will be committed. > I'm pretty sure, thought I haven't tried yet, that we can now remove the > PrivGrantee node completely. That's done in the attached. Documentation is still missing. Are you submitting doc changes soon? I would like to get this committed. -- Á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..e88c8c3 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -421,22 +421,25 @@ ExecuteGrantStmt(GrantStmt *stmt) istmt.behavior = stmt->behavior; /* - * 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. + * Convert the RoleSpec list into an Oid list. Note that at this point + * 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); + RoleSpec *grantee = (RoleSpec *) lfirst(cell); + Oid grantee_uid; - 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->roletype) + { + case ROLESPEC_PUBLIC: +grantee_uid = ACL_ID_PUBLIC; +break; + default: +grantee_uid = get_rolespec_oid((Node *) grantee, false); +break; + } + istmt.grantees = lappend_oid(istmt.grantees, grantee_uid); } /* @@ -904,22 +907,25 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt) iacls.behavior = action->behavior; /* - * 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. + * Convert the RoleSpec list into an Oid list. Note that at this point + * 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); + RoleSpec *grantee = (RoleSpec *) lfirst(cell); + Oid grantee_uid; - 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->roletype) + { + case ROLESPEC_PUBLIC: +grantee_uid = ACL_ID_PUBLIC; +break; + default: +grantee_uid = get_rolespec_oid((Node *) grantee, 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(Rela
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 +6
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 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 ExecAlt
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 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 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);
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 - ALTER ROLE/GROUP/USER RENAME TO 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 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 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 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 +--- src/backe
Re: [HACKERS] alter user/role CURRENT_USER
Hello, > Adam Brightwell 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 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. 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
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 name RENAME TO new_name ! ALTER USER name SET configuration_parameter { TO | = } { value | DEFAULT } ! ALTER USER name SET configuration_parameter FROM CURRENT ! ALTER USER name RESET configuration_parameter ! ALTER USER name RESET ALL --- 38,47 ALTER USER name RENAME TO new_name ! ALTER USER name [ IN DATABASE database_name ] SET configuration_parameter { TO | = } { value | DEFAULT } ! ALTER USER { name | ALL } [ IN DATABASE database_name ] SET configuration_parameter FROM CURRENT ! ALTER USER { name | ALL } [ IN DATABASE database_name ] RESET configuration_parameter ! ALTER USER { name | ALL } [ IN DATABASE database_name ] RESET ALL 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 opt_existing_window_name %type opt_if_not_exists + %type 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 = makeNode(AlterRoleStmt); n->role = $3; --- 1033,1044 * * Alter a postgresql DBMS role * + * ALTER USER is accepted interchangeably with ALTER ROLE. + * */ AlterRoleStmt: ! ALTER role_or_user RoleId opt_with AlterOptRoleList { AlterRoleStmt *n = makeNode(AlterRoleStmt); n->role = $3; *** AlterRoleStmt: *** 1046,1058 } ; opt_in_database: /* EMPTY */ { $$ = NULL; } | IN_P DATABASE database_name { $$ = $3; } ; AlterRoleSetStmt: ! ALTER ROLE RoleId opt_in_database SetResetClause { AlterRoleSetStmt *n = makeNode(AlterRoleSetStmt); n->role = $3; --- 1048,1065 } ; + role_or_user: + ROLE{} + | USER{} + ; + opt_in_database: /* EMPT
Re: [HACKERS] alter user/role CURRENT_USER
Adam Brightwell 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 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
Stephen Frost 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) 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
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
Hello, At Tue, 28 Oct 2014 09:05:20 -0400, Stephen Frost 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
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 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 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 +++ b/sr
Re: [HACKERS] alter user/role CURRENT_USER
Marti Raudsepp wrote: > On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI > 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
* Kevin Grittner (kgri...@ymail.com) wrote: > Stephen Frost 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
On Tue, Oct 28, 2014 at 2:40 AM, Adam Brightwell 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
Stephen Frost 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
* 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
> > +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
Marti Raudsepp wrote > On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI > < > horiguchi.kyotaro@.co > > 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
On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI 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
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
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 ... > > - ALTER AGGREAGE/COLLATION/etc... OWNER TO > > - CREATE/ALTER/DROP USER MAPPING FOR 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 ... - ALTER AGGREAGE/COLLATION/etc... OWNER TO - CREATE/ALTER/DROP USER MAPPING FOR 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 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 "com
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
Thank you for reviewing, 2014 13:10:57 +0530, Rushabh Lathia wrote in > 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 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
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 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 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
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 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
[HACKERS] alter user/role CURRENT_USER
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 >From d12f479de845f55f77096e79fea69930bd665416 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi Date: Tue, 9 Sep 2014 19:26:33 +0900 Subject: [PATCH 2/2] ALTER ROLE CURRENT_USER document --- doc/src/sgml/ref/alter_role.sgml | 15 --- doc/src/sgml/ref/alter_user.sgml | 13 +++-- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml index 0471daa..e6f8093 100644 --- a/doc/src/sgml/ref/alter_role.sgml +++ b/doc/src/sgml/ref/alter_role.sgml @@ -21,7 +21,7 @@ PostgreSQL documentation -ALTER ROLE name [ [ WITH ] option [ ... ] ] +ALTER ROLE { name | CURRENT_USER } [ [ WITH ] option [ ... ] ] where option can be: @@ -37,12 +37,12 @@ ALTER ROLE name [ [ WITH ] password' | VALID UNTIL 'timestamp' -ALTER ROLE name RENAME TO new_name +ALTER ROLE name RENAME TO new_name -ALTER ROLE name [ IN DATABASE database_name ] SET configuration_parameter { TO | = } { value | DEFAULT } -ALTER ROLE { name | ALL } [ IN DATABASE database_name ] SET configuration_parameter FROM CURRENT -ALTER ROLE { name | ALL } [ IN DATABASE database_name ] RESET configuration_parameter -ALTER ROLE { name | ALL } [ IN DATABASE database_name ] RESET ALL +ALTER ROLE { name | CURRENT_USER | ALL } [ IN DATABASE database_name ] SET configuration_parameter { TO | = } { value | DEFAULT } +ALTER ROLE { name | CURRENT_USER | ALL } [ IN DATABASE database_name ] SET configuration_parameter FROM CURRENT +ALTER ROLE { name | CURRENT_USER | ALL } [ IN DATABASE database_name ] RESET configuration_parameter +ALTER ROLE { name | CURRENT_USER | ALL } [ IN DATABASE database_name ] RESET ALL @@ -123,7 +123,8 @@ ALTER ROLE { name | ALL } [ IN DATA name -The name of the role whose attributes are to be altered. +The name of the role whose attributes are to be +altered. CURRENT_USER matches the name of the current user. diff --git a/doc/src/sgml/ref/alter_user.sgml b/doc/src/sgml/ref/alter_user.sgml index 58ae1da..feb1197 100644 --- a/doc/src/sgml/ref/alter_user.sgml +++ b/doc/src/sgml/ref/alter_user.sgml @@ -21,7 +21,7 @@ PostgreSQL documentation -ALTER USER name [ [ WITH ] option [ ... ] ] +ALTER USER { name | CURRENT_USER } [ [ WITH ] option [ ... ] ] where option can be: @@ -32,16 +32,17 @@ ALTER USER name [ [ WITH ] connlimit | [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password' | VALID UNTIL 'timestamp' -ALTER USER name RENAME TO new_name +ALTER USER name RENAME TO new_name -ALTER USER name SET configuration_parameter { TO | = } { value | DEFAULT } -ALTER USER name SET configuration_parameter FROM CURRENT -ALTER USER name RESET configuration_parameter -ALTER USER name RESET ALL +ALTER USER { name | CURRENT_USER | ALL } [ IN DATABASE database_name ] SET configuration_parameter { TO | = } { value | DEFAULT } +ALTER USER { name | CURRENT_USER | ALL } [ IN DATABASE database_name ] SET configuration_parameter FROM CURRENT +ALTER USER { name | CURRENT_USER | ALL } [ IN DATABASE database_name ] RESET configuration_parameter +ALTER USER { name | CURRENT_USER | ALL } [ IN DATABASE database_name ] RESET ALL -- 1.7.1 >From 9be0ca6f7961ccadf665867c52233079a1024737 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi Date: Tue, 9 Sep 2014 19:26:24 +0900 Subject: [PATCH 1/2] ALTER ROLE CURRENT_USER --- src/backend/commands/user.c | 48 -- src/backend/parser/gram.y | 27 +-- 2 files changed, 56 insertions(+), 19 deletions(-) diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 1a73fd8..8630323 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -649,13 +649,25 @@ AlterRole(AlterRoleStmt *stmt) pg_authid_rel = heap_open(AuthIdRelationId, RowExclusiveLock); pg_authi