Обсуждение: change alter user to be a true alias for alter role

Jov <amutu@amutu.com> wrote:

> attach patch add the per database set for alter user

Please add to the open CommitFest so that the patch doesn't "fall
through the cracks":

https://commitfest.postgresql.org/action/commitfest_view/open

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 01/20/2014 03:31 PM, Jov wrote:
> the doc say:
> 
>     ALTER USER is now an alias for ALTER ROLE
>     <http://www.postgresql.org/docs/devel/static/sql-alterrole.html>.
> 
>  
> but alter user lack the following format:
> 
> [...]
> 
> this make alter user a true alias for alter role.

This is a straightforward patch that does exactly what it says on the
tin.  I have marked it ready for committer.
-- 
Vik

Jov <amutu@amutu.com> writes:
> the doc say:
>> ALTER USER is now an alias for ALTER ROLE<http://www.postgresql.org/docs/devel/static/sql-alterrole.html>

> but alter user lack the following format:
> ...

If we're going to have a policy that these commands be exactly equivalent,
it seems like this patch is just sticking a finger into the dike.  It does
nothing to prevent anyone from making the same mistake again in future.

What about collapsing both sets of productions into one, along the lines
of

role_or_user: ROLE | USER;

AlterRoleSetStmt:ALTER role_or_user RoleId opt_in_database SetResetClause

(and similarly to the latter for every existing ALTER ROLE variant).

Because MAPPING is an unreserved keyword, I think that this approach
might force us to also change ALTER USER MAPPING to ALTER role_or_user
MAPPING, which is not contemplated by the SQL standard.  But hey,
it would satisfy the principle of least surprise no?  Anyway we don't
have to document that that would work.
        regards, tom lane

On 06/19/2014 07:18 PM, Tom Lane wrote:
> Jov <amutu@amutu.com> writes:
>> the doc say:
>>> ALTER USER is now an alias for ALTER ROLE<http://www.postgresql.org/docs/devel/static/sql-alterrole.html>
> 
>> but alter user lack the following format:
>> ...
> 
> If we're going to have a policy that these commands be exactly equivalent,
> it seems like this patch is just sticking a finger into the dike.  It does
> nothing to prevent anyone from making the same mistake again in future.
> 
> What about collapsing both sets of productions into one, along the lines
> of
> 
> role_or_user: ROLE | USER;
> 
> AlterRoleSetStmt:
>     ALTER role_or_user RoleId opt_in_database SetResetClause
> 
> (and similarly to the latter for every existing ALTER ROLE variant).

I thought about suggesting that, and it seems that I should have.  I
also thought about suggesting adding GROUP as an alias, too.  That's
probably not as good of an idea.

> Because MAPPING is an unreserved keyword, I think that this approach
> might force us to also change ALTER USER MAPPING to ALTER role_or_user
> MAPPING, which is not contemplated by the SQL standard.  But hey,
> it would satisfy the principle of least surprise no?  Anyway we don't
> have to document that that would work.

That's a small price to pay, so I'm all for accepting it.  I agree that
it doesn't need to be documented.
-- 
Vik

On 06/19/2014 07:18 PM, Tom Lane wrote:
> Jov <amutu@amutu.com> writes:
>> the doc say:
>>> ALTER USER is now an alias for ALTER ROLE<http://www.postgresql.org/docs/devel/static/sql-alterrole.html>
> 
>> but alter user lack the following format:
>> ...
> 
> If we're going to have a policy that these commands be exactly equivalent,
> it seems like this patch is just sticking a finger into the dike.  It does
> nothing to prevent anyone from making the same mistake again in future.
> 
> What about collapsing both sets of productions into one, along the lines
> of
> 
> role_or_user: ROLE | USER;
> 
> AlterRoleSetStmt:
>     ALTER role_or_user RoleId opt_in_database SetResetClause
> 
> (and similarly to the latter for every existing ALTER ROLE variant).
> 
> Because MAPPING is an unreserved keyword, I think that this approach
> might force us to also change ALTER USER MAPPING to ALTER role_or_user
> MAPPING, which is not contemplated by the SQL standard.  But hey,
> it would satisfy the principle of least surprise no?  Anyway we don't
> have to document that that would work.

After a week of silence from Jov, I decided to do this myself since it
didn't seem very hard.

Many frustrating hours of trying to understand why I'm getting
shift/reduce conflicts by the hundreds later, I've decided to give up
for now.
-- 
Vik

At 2014-06-27 16:11:21 +0200, vik.fearing@dalibo.com wrote:
>
> After a week of silence from Jov, I decided to do this myself since it
> didn't seem very hard.
> 
> Many frustrating hours of trying to understand why I'm getting
> shift/reduce conflicts by the hundreds later, I've decided to give up
> for now.

Jov, do you expect to be able to work on the patch along the lines Tom
suggested and resubmit during this CF?

If not, since Vik gave up on it, it seems to me that it would be best to
mark it returned with feedback (which I'll do in a couple of days if I
don't hear anything to the contrary).

-- Abhijit

Jov <amutu@amutu.com> writes:
> I make the v2 of the patch,use Tom's advice.
> But I can't make ROLE and USER in the keyword list,it is hard to solve the
> conflict,or rewrite many gram rules.

This patch seems to be trying to make ROLE and USER interchangeable
*everywhere* in the grammar, which is moving the goalposts quite a long
way to little purpose.  The idea seems rather misguided to me, since
certainly CREATE USER and CREATE ROLE will never be exact synonyms.
I think it's sufficient to solve the original complaint about them not
being interchangeable in ALTER.

The patch as given is broken anyway: it removes both those keywords
from the keyword lists, which effectively makes them fully reserved,
in fact worse than fully reserved.

What I had in mind was more like the attached, which I've not tested
but bison does compile it without complaint.  I've not looked at the
documentation end of it, either.

            regards, tom lane

*** src/backend/parser/gram.y~    Sat Aug 23 14:53:23 2014
--- src/backend/parser/gram.y    Sat Aug 23 18:10:56 2014
*************** static Node *makeRecursiveViewSelect(cha
*** 230,236 ****
          AlterFdwStmt AlterForeignServerStmt AlterGroupStmt
          AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterSystemStmt AlterTableStmt
          AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt AlterForeignTableStmt
!         AlterCompositeTypeStmt AlterUserStmt AlterUserMappingStmt AlterUserSetStmt
          AlterRoleStmt AlterRoleSetStmt
          AlterDefaultPrivilegesStmt DefACLAction
          AnalyzeStmt ClosePortalStmt ClusterStmt CommentStmt
--- 230,236 ----
          AlterFdwStmt AlterForeignServerStmt AlterGroupStmt
          AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterSystemStmt AlterTableStmt
          AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt AlterForeignTableStmt
!         AlterCompositeTypeStmt AlterUserMappingStmt
          AlterRoleStmt AlterRoleSetStmt
          AlterDefaultPrivilegesStmt DefACLAction
          AnalyzeStmt ClosePortalStmt ClusterStmt CommentStmt
*************** stmt :
*** 749,756 ****
              | AlterTSConfigurationStmt
              | AlterTSDictionaryStmt
              | AlterUserMappingStmt
-             | AlterUserSetStmt
-             | AlterUserStmt
              | AnalyzeStmt
              | CheckPointStmt
              | ClosePortalStmt
--- 749,754 ----
*************** CreateUserStmt:
*** 1020,1029 ****
   *
   * Alter a postgresql DBMS role
   *
   *****************************************************************************/

  AlterRoleStmt:
!             ALTER ROLE RoleId opt_with AlterOptRoleList
                   {
                      AlterRoleStmt *n = makeNode(AlterRoleStmt);
                      n->role = $3;
--- 1018,1029 ----
   *
   * 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:
*** 1033,1045 ****
                   }
          ;

  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;
--- 1033,1050 ----
                   }
          ;

+ role_or_user:
+             ROLE
+             | USER
+         ;
+
  opt_in_database:
                 /* EMPTY */                    { $$ = NULL; }
              | IN_P DATABASE database_name    { $$ = $3; }
          ;

  AlterRoleSetStmt:
!             ALTER role_or_user RoleId opt_in_database SetResetClause
                  {
                      AlterRoleSetStmt *n = makeNode(AlterRoleSetStmt);
                      n->role = $3;
*************** AlterRoleSetStmt:
*** 1047,1053 ****
                      n->setstmt = $5;
                      $$ = (Node *)n;
                  }
!             | ALTER ROLE ALL opt_in_database SetResetClause
                  {
                      AlterRoleSetStmt *n = makeNode(AlterRoleSetStmt);
                      n->role = NULL;
--- 1052,1058 ----
                      n->setstmt = $5;
                      $$ = (Node *)n;
                  }
!             | ALTER role_or_user ALL opt_in_database SetResetClause
                  {
                      AlterRoleSetStmt *n = makeNode(AlterRoleSetStmt);
                      n->role = NULL;
*************** AlterRoleSetStmt:
*** 1060,1095 ****

  /*****************************************************************************
   *
-  * Alter a postgresql DBMS user
-  *
-  *****************************************************************************/
-
- AlterUserStmt:
-             ALTER USER RoleId opt_with AlterOptRoleList
-                  {
-                     AlterRoleStmt *n = makeNode(AlterRoleStmt);
-                     n->role = $3;
-                     n->action = +1;    /* add, if there are members */
-                     n->options = $5;
-                     $$ = (Node *)n;
-                  }
-         ;
-
-
- AlterUserSetStmt:
-             ALTER USER RoleId SetResetClause
-                 {
-                     AlterRoleSetStmt *n = makeNode(AlterRoleSetStmt);
-                     n->role = $3;
-                     n->database = NULL;
-                     n->setstmt = $4;
-                     $$ = (Node *)n;
-                 }
-             ;
-
-
- /*****************************************************************************
-  *
   * Drop a postgresql DBMS role
   *
   * XXX Ideally this would have CASCADE/RESTRICT options, but since a role
--- 1065,1070 ----
*************** DropUserMappingStmt: DROP USER MAPPING F
*** 4463,4471 ****
   *        QUERY :
   *                ALTER USER MAPPING FOR auth_ident SERVER name OPTIONS
   *
   ****************************************************************************/

! AlterUserMappingStmt: ALTER USER MAPPING FOR auth_ident SERVER name alter_generic_options
                  {
                      AlterUserMappingStmt *n = makeNode(AlterUserMappingStmt);
                      n->username = $5;
--- 4438,4451 ----
   *        QUERY :
   *                ALTER USER MAPPING FOR auth_ident SERVER name OPTIONS
   *
+  * Note: we also accept "ALTER ROLE MAPPING", because distinguishing that
+  * case in the grammar would require us to distinguish ROLE from USER in all
+  * other ALTER cases, and we don't want to do that.  However, that spelling
+  * of the command is nonstandard and is not documented.
+  *
   ****************************************************************************/

! AlterUserMappingStmt: ALTER role_or_user MAPPING FOR auth_ident SERVER name alter_generic_options
                  {
                      AlterUserMappingStmt *n = makeNode(AlterUserMappingStmt);
                      n->username = $5;
*************** RenameStmt: ALTER AGGREGATE func_name ag
*** 7464,7479 ****
                      n->newname = $7;
                      $$ = (Node *)n;
                  }
!             | ALTER ROLE RoleId RENAME TO RoleId
!                 {
!                     RenameStmt *n = makeNode(RenameStmt);
!                     n->renameType = OBJECT_ROLE;
!                     n->subname = $3;
!                     n->newname = $6;
!                     n->missing_ok = false;
!                     $$ = (Node *)n;
!                 }
!             | ALTER USER RoleId RENAME TO RoleId
                  {
                      RenameStmt *n = makeNode(RenameStmt);
                      n->renameType = OBJECT_ROLE;
--- 7444,7450 ----
                      n->newname = $7;
                      $$ = (Node *)n;
                  }
!             | ALTER role_or_user RoleId RENAME TO RoleId
                  {
                      RenameStmt *n = makeNode(RenameStmt);
                      n->renameType = OBJECT_ROLE;