Re: [HACKERS] change alter user to be a true alias for alter role

[Tom Lane]

Jov  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;
- }
- 			;
- 
- 
- /*
-  *
   *

Re: [HACKERS] change alter user to be a true alias for alter role

[Jov]

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.
the problem is :

role_or_user : ROLE | USER;
xx_keyword:...| ROLE|...|USER..;

this two rules produce conflict.So in v2,I remove ROLE and USER from the
xx_keyword rule now?
Any idea?

Jov
blog: http:amutu.com/blog 


2014-07-09 18:27 GMT+08:00 Jov :

> Sorry for the late resp,I will try to update the patch.
>
> Jov
> blog: http:amutu.com/blog 
>
>
> 2014-07-02 4:17 GMT+08:00 Abhijit Menon-Sen :
>
> At 2014-06-27 16:11:21 +0200, vik.fear...@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
>>
>
>


full-imp-alter-user-v2.patch.gz
Description: GNU Zip compressed data

-- 
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] change alter user to be a true alias for alter role

[Jov]

Sorry for the late resp,I will try to update the patch.

Jov
blog: http:amutu.com/blog 


2014-07-02 4:17 GMT+08:00 Abhijit Menon-Sen :

> At 2014-06-27 16:11:21 +0200, vik.fear...@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
>

Re: [HACKERS] change alter user to be a true alias for alter role

[Abhijit Menon-Sen]

At 2014-06-27 16:11:21 +0200, vik.fear...@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


-- 
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] change alter user to be a true alias for alter role

[Vik Fearing]

On 06/19/2014 07:18 PM, Tom Lane wrote:
> Jov  writes:
>> the doc say:
>>> ALTER USER is now an alias for ALTER 
>>> ROLE
> 
>> 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


-- 
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] change alter user to be a true alias for alter role

[Vik Fearing]

On 06/19/2014 07:18 PM, Tom Lane wrote:
> Jov  writes:
>> the doc say:
>>> ALTER USER is now an alias for ALTER 
>>> ROLE
> 
>> 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


-- 
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] change alter user to be a true alias for alter role

[Tom Lane]

Jov  writes:
> the doc say:
>> ALTER USER is now an alias for ALTER 
>> ROLE

> 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


-- 
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] change alter user to be a true alias for alter role

[Vik Fearing]

On 01/20/2014 03:31 PM, Jov wrote:
> the doc say:
> 
> ALTER USER is now an alias for ALTER ROLE
> .
> 
>  
> 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


-- 
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] change alter user to be a true alias for alter role

[Kevin Grittner]

Jov  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers