subject:"\[HACKERS\] alter user\/role CURRENT

Re: [HACKERS] alter user/role CURRENT_USER

2015-05-01 Thread Kyotaro HORIGUCHI

Hello,

At Thu, 30 Apr 2015 17:12:25 -0300, Alvaro Herrera alvhe...@2ndquadrant.com 
wrote in 20150430201225.gv4...@alvh.no-ip.org
 Kyotaro HORIGUCHI wrote:
  Thank you for completing this and very sorry not to respond these
  days.
  
  I understood that it is committed after I noticed that rebasing
  my code failed..
 
 You'd do well to check your email, I guess :-)

Yeah, I agree with you since I noticed that before I read the
mail mentioning that. I should be more carefull:( Sorry to bother
you and thank you for your kindness.

  | =# alter role current_user rename to PubLic;
  | ERROR:  CURRENT_USER cannot be used as a role name
  | LINE 1: alter role current_user rename to PubLic;
  |^
  
  The error message sounds somewhat different from the intention. I
  think the following message would be clearer.
  
  | ERROR:  CURRENT_USER cannot be used as a role name here
 
 Okay, changed.
 
 
  
  The document sql-altergroup.html says
  
  | ALTER GROUP role_specification ADD USER user_name [, ... ]
  
  But current_user is also usable in user_name list. So the doc
  should be as following, but it would not be necessary to be fixed
  because it is an obsolete commnand..
  
  | ALTER GROUP role_specification ADD USER role_specification [, ... ]
 
 Yeah, EDONTCARE.
 
  ALTER GROUP role_spec ADD/DROP USER role_spec is naturally
  denied so I think no additional description is needed.
 
 +1
 
  
  sql-alterpolicy.html
  
  ALTER POLICY name ON table_name TO also accepts current_user
  and so as the role to which the policy applies.
 
 Changed.
 
  # As a different topic, the syntax ALTER POLICY pname ON
  # tname TO user looks a bit wired, it might be better be to
  # be ON tname APPLY TO user but I shouldn't try to fix it
  # since it is a long standing syntax..
 
 Yeah, it's a bit strange.  Not a strong opinion.  Maybe you should raise
 it as a separate thread.
 
  
  sql-createtablespace.html
  sql-drop-owned.html, sql-reassign-owned.html
 
 Changed.

Thank you applying the changes above.

  ==
  sql-grant.html, sql-revoke.html,
  
  GRANT roles TO roles and REVOKE roles FROM roles are
  the modern equivalents of the deprecated syntaxes ALTER roles
  ADD USER roles and ALTER roles DROP USER roles
  respectively. But the current parser infrastructure doesn't allow
  coexistence of the two following syntaxes but I couldn't find the
  way to their coexistence.
 
 I decided to leave this out.  I think we should consider it as a new
 patch for 9.6; these changes aren't as clear-cut as the rest of your
 patch.  I didn't want to have to research the ecpg changes.

Ok, it sounds fair enough.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] alter user/role CURRENT_USER

2015-04-30 Thread Alvaro Herrera

Kyotaro HORIGUCHI wrote:
 Thank you for completing this and very sorry not to respond these
 days.
 
 I understood that it is committed after I noticed that rebasing
 my code failed..

You'd do well to check your email, I guess :-)

 Although after committed, I found some issues as I looked on
 it. Please forgive me to comment it now after all this time.

 
 | =# alter role current_user rename to PubLic;
 | ERROR:  CURRENT_USER cannot be used as a role name
 | LINE 1: alter role current_user rename to PubLic;
 |^
 
 The error message sounds somewhat different from the intention. I
 think the following message would be clearer.
 
 | ERROR:  CURRENT_USER cannot be used as a role name here

Okay, changed.


 
 The document sql-altergroup.html says
 
 | ALTER GROUP role_specification ADD USER user_name [, ... ]
 
 But current_user is also usable in user_name list. So the doc
 should be as following, but it would not be necessary to be fixed
 because it is an obsolete commnand..
 
 | ALTER GROUP role_specification ADD USER role_specification [, ... ]

Yeah, EDONTCARE.

 ALTER GROUP role_spec ADD/DROP USER role_spec is naturally
 denied so I think no additional description is needed.

+1

 
 sql-alterpolicy.html
 
 ALTER POLICY name ON table_name TO also accepts current_user
 and so as the role to which the policy applies.

Changed.

 # As a different topic, the syntax ALTER POLICY pname ON
 # tname TO user looks a bit wired, it might be better be to
 # be ON tname APPLY TO user but I shouldn't try to fix it
 # since it is a long standing syntax..

Yeah, it's a bit strange.  Not a strong opinion.  Maybe you should raise
it as a separate thread.

 
 sql-createtablespace.html
 sql-drop-owned.html, sql-reassign-owned.html

Changed.

 ==
 sql-grant.html, sql-revoke.html,
 
 GRANT roles TO roles and REVOKE roles FROM roles are
 the modern equivalents of the deprecated syntaxes ALTER roles
 ADD USER roles and ALTER roles DROP USER roles
 respectively. But the current parser infrastructure doesn't allow
 coexistence of the two following syntaxes but I couldn't find the
 way to their coexistence.

I decided to leave this out.  I think we should consider it as a new
patch for 9.6; these changes aren't as clear-cut as the rest of your
patch.  I didn't want to have to research the ecpg changes.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


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

Re: [HACKERS] alter user/role CURRENT_USER

2015-03-12 Thread Kyotaro HORIGUCHI

Thank you for completing this and very sorry not to respond these
days.

I understood that it is committed after I noticed that rebasing
my code failed..

Although after committed, I found some issues as I looked on
it. Please forgive me to comment it now after all this time.

Several changes in docs according to the changed syntax and one
change in code itself to allow CURRENT_USER in GRANT roleid TO
roleid syntax.


At Mon, 9 Mar 2015 15:50:32 -0300, Alvaro Herrera alvhe...@2ndquadrant.com 
wrote in 20150309185032.gq3...@alvh.no-ip.org
 Alvaro Herrera wrote:
 
  With this patch applied, doing
  \h ALTER ROLE
  in psql looks quite odd: note how wide it has become.  Maybe we should
  be doing this differently?  (Hmm, why don't we accept ALL in the first SET
  line?  Maybe that's just a mistake and the four lines should be all
  identical in the first half ...)
 
 I have fixed the remaining issues, completed the doc changes, and
 pushed.  Given the lack of feedback I had to follow my gut on the best
 way to change the docs.  I added the regression test you submitted with
 some additional changes, mainly to make sure they don't fail immediately
 when other databases exist; maybe some more concurrency or platform
 issues will show up there, but let's see what the buildfarm says.
 
 Thanks Horiguchi-san for the patch and everyone for the reviews.  (It's
 probably worthwhile giving things an extra look.)


| =# alter role current_user rename to PubLic;
| ERROR:  CURRENT_USER cannot be used as a role name
| LINE 1: alter role current_user rename to PubLic;
|^

The error message sounds somewhat different from the intention. I
think the following message would be clearer.

| ERROR:  CURRENT_USER cannot be used as a role name here


The document sql-altergroup.html says

| ALTER GROUP role_specification ADD USER user_name [, ... ]

But current_user is also usable in user_name list. So the doc
should be as following, but it would not be necessary to be fixed
because it is an obsolete commnand..

| ALTER GROUP role_specification ADD USER role_specification [, ... ]

ALTER GROUP role_spec ADD/DROP USER role_spec is naturally
denied so I think no additional description is needed.


sql-alterpolicy.html

ALTER POLICY name ON table_name TO also accepts current_user
and so as the role to which the policy applies.

# As a different topic, the syntax ALTER POLICY pname ON
# tname TO user looks a bit wired, it might be better be to
# be ON tname APPLY TO user but I shouldn't try to fix it
# since it is a long standing syntax..


sql-createtablespace.html

OWNER username should be OWNER user_name | (CURRENT|SESSION)_USER


sql-drop-owned.html, sql-reassign-owned.html

name should be  {name | (CURRENT|SESSION)_USER}

For REASSIGN OWNED, TO clause also needs the same fix.

==
sql-grant.html, sql-revoke.html,

GRANT roles TO roles and REVOKE roles FROM roles are
the modern equivalents of the deprecated syntaxes ALTER roles
ADD USER roles and ALTER roles DROP USER roles
respectively. But the current parser infrastructure doesn't allow
coexistence of the two following syntaxes but I couldn't find the
way to their coexistence.

# more precisely, I guess the GRANT followed by both
# privelege_list and role_list will steps out of the realm of
# LALR(1), which I don't know well of..

GRANT privs ON ... 
GRANT roles TO ...

After some struggle, I decided to add special rules
(CURRENT|SESSION)_USER to the non-terminal privilege and make a
room to store RoleSpec in AccessPriv. This is quite ugly but
there seems no way other than that to accomplish it.. (AccessPriv
already conveys other than the information different from what
its name represents:p)

After this fix, the commands like following are processed
properly. public and none are simply handled as nonexistent
names.

GRANT test1 TO CURRENT_USER;

GRANT priv ON table TO role properly rejects CURRENT_USER
as priv.

But the change in gram.y cuases changes in preproc.y as following,

  privilege:
  SELECT opt_column_list
  { 
...
 |  ColId opt_column_list
  { 
  $$ = cat_str(2,$1,$2);
 }
+ |  CURRENT_USER
+  { 
+  $$ = mm_strdup(current_user);
+ }
+ |  SESSION_USER
+  { 
+  $$ = mm_strdup(session_user);
+ }
 ;

I don't understand what this change causes...

=

I haven't looked on the docs for syntaxes related to
AlterOwnerStatement. But perhaps they don't be wrong.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 26afc656576c8778921ff44519e3de86866ab138 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp
Date: Thu, 12 Mar 2015 20:56:14 +0900
Subject: [PATCH] Some additional changes for ALTER ROLE/USER CURRENT_USER.

Some documents are not edit according to new specs. Addition to it,
GRANT roles TO roles and REVOKE roles FROM roles syntaxes,
which are the modern replacement of ALTER GROUP ADD/DROP USER
syntax, are not accepted by the previous patch.

This patch fixes some docs and changes

Re: [HACKERS] alter user/role CURRENT_USER

2015-03-09 Thread Alvaro Herrera

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

2015-03-06 Thread Alvaro Herrera

There is something odd here going on:

alvherre=# alter group test add user current_user;
ERROR:  role test is a member of role (null)

Surely (null) is not the right name to be reporting there ...

Attached is a rebased patch, which also has some incomplete doc changes.

With this patch applied, doing
\h ALTER ROLE
in psql looks quite odd: note how wide it has become.  Maybe we should
be doing this differently?  (Hmm, why don't we accept ALL in the first SET
line?  Maybe that's just a mistake and the four lines should be all
identical in the first half ...)


alvherre=# \h alter role
Command: ALTER ROLE
Description: change a database role
Syntax:
ALTER ROLE { name | CURRENT_USER | SESSION_USER } [ WITH ] option [ ... ]

where option can be:

  SUPERUSER | NOSUPERUSER
| CREATEDB | NOCREATEDB
| CREATEROLE | NOCREATEROLE
| CREATEUSER | NOCREATEUSER
| INHERIT | NOINHERIT
| LOGIN | NOLOGIN
| REPLICATION | NOREPLICATION
| BYPASSRLS | NOBYPASSRLS
| CONNECTION LIMIT connlimit
| [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password'
| VALID UNTIL 'timestamp'

ALTER ROLE name RENAME TO new_name

ALTER ROLE { name | CURRENT_USER | SESSION_USER } [ IN DATABASE database_name ] 
SET configuration_parameter { TO | = } { value | DEFAULT }
ALTER ROLE { name | CURRENT_USER | SESSION_USER | ALL } [ IN DATABASE 
database_name ] SET configuration_parameter FROM CURRENT
ALTER ROLE { name | CURRENT_USER | SESSION_USER | ALL } [ IN DATABASE 
database_name ] RESET configuration_parameter
ALTER ROLE { name | CURRENT_USER | SESSION_USER | ALL } [ IN DATABASE 
database_name ] RESET ALL

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services
diff --git a/doc/src/sgml/ref/alter_group.sgml b/doc/src/sgml/ref/alter_group.sgml
index 1432242..5f0d8b4 100644
--- a/doc/src/sgml/ref/alter_group.sgml
+++ b/doc/src/sgml/ref/alter_group.sgml
@@ -21,10 +21,10 @@ PostgreSQL documentation
 
  refsynopsisdiv
 synopsis
-ALTER GROUP replaceable class=PARAMETERgroup_name/replaceable ADD USER replaceable class=PARAMETERuser_name/replaceable [, ... ]
-ALTER GROUP replaceable class=PARAMETERgroup_name/replaceable DROP USER replaceable class=PARAMETERuser_name/replaceable [, ... ]
+ALTER GROUP { replaceable class=PARAMETERgroup_name/replaceable | CURRENT_USER | SESSION_USER } ADD USER replaceable class=PARAMETERuser_name/replaceable [, ... ]
+ALTER GROUP { replaceable class=PARAMETERgroup_name/replaceable | CURRENT_USER | SESSION_USER } DROP USER replaceable class=PARAMETERuser_name/replaceable [, ... ]
 
-ALTER GROUP replaceable class=PARAMETERgroup_name/replaceable RENAME TO replaceablenew_name/replaceable
+ALTER GROUP { replaceable class=PARAMETERgroup_name/replaceable | CURRENT_USER | SESSION_USER } RENAME TO replaceablenew_name/replaceable
 /synopsis
  /refsynopsisdiv
 
diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
index 0471daa..59f6499 100644
--- a/doc/src/sgml/ref/alter_role.sgml
+++ b/doc/src/sgml/ref/alter_role.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  refsynopsisdiv
 synopsis
-ALTER ROLE replaceable class=PARAMETERname/replaceable [ [ WITH ] replaceable class=PARAMETERoption/replaceable [ ... ] ]
+ALTER ROLE { replaceable class=PARAMETERname/replaceable | CURRENT_USER | SESSION_USER } [ WITH ] replaceable class=PARAMETERoption/replaceable [ ... ]
 
 phrasewhere replaceable class=PARAMETERoption/replaceable can be:/phrase
 
@@ -39,10 +39,10 @@ ALTER ROLE replaceable class=PARAMETERname/replaceable [ [ WITH ] replace
 
 ALTER ROLE replaceable class=PARAMETERname/replaceable RENAME TO replaceablenew_name/replaceable
 
-ALTER ROLE replaceable class=PARAMETERname/replaceable [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT }
-ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable FROM CURRENT
-ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET replaceableconfiguration_parameter/replaceable
-ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET ALL
+ALTER ROLE { replaceable class=PARAMETERname/replaceable | CURRENT_USER | SESSION_USER } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT }
+ALTER ROLE { replaceable class=PARAMETERname/replaceable | CURRENT_USER | SESSION_USER | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable FROM CURRENT
+ALTER ROLE { replaceable

Re: [HACKERS] alter user/role CURRENT_USER

2015-03-02 Thread Alvaro Herrera

Kyotaro HORIGUCHI wrote:
 Hello, thank you for the comment.
 
  Looking at this a bit, I'm not sure completely replacing RoleId with a
  node is the best idea; some of the users of that production in the
  grammar are okay with accepting only normal strings as names, and don't
  need all the CURRENT_USER etc stuff.
 
 You're right. the productions don't need RoleSpec already uses
 char* for the role member in its *Stmt structs. I might have done
 a bit too much.
 
 Adding new nonterminal RoleId and using it makes a bit duplicate
 of check code for public/none and others but removes other
 redundant check for != CSTRING from some production, CREATE
 ROLE/USER/GROUP, ALTER ROLE/USER/GROUP RENAME.

Thanks for doing the fiddly work here.  Attached is a new version of
this patch.  I simplified some things, including removing those rules
you added to RoleId.  It seems to me that this problem:

 RoleId in the patch still has rule components for CURRENT_USER,
 SESSION_USER, and CURRENT_ROLE. Without them, the parser prints
 an error ununderstandable to users.
 
 | =# alter role current_user rename to PuBlic;
 | ERROR:  syntax error at or near rename
 | LINE 1: alter role current_user rename to PuBlic;
 | ^

can be fixed without complicating the rest of the stuff simply by using
RoleSpec instead of RoleId and doing the error checks at the RenameStmt
production.  Altering the current user and session user is disallowed
downstream, so there's no reason we can't just have gram.y throw the
same error when special node types are used; so we end up passing the
role name only (just as currently) and the error message remains clear.

I couldn't find any further problems with this version of the code,
though I also noticed that a lot of things are not being tested in the
regression tests, such as create user public or alter user none.  It
would be good to have tests for such cases, to avoid breaking them
accidentally.  If you can spare some time to submit test cases for such
commands, I would be thankful.

I'm pretty sure, thought I haven't tried yet, that we can now remove the
PrivGrantee node completely.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 1e3888e..56cafa8 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -422,21 +422,24 @@ ExecuteGrantStmt(GrantStmt *stmt)
 
 	/*
 	 * Convert the PrivGrantee list into an Oid list.  Note that at this point
-	 * we insert an ACL_ID_PUBLIC into the list if an empty role name is
-	 * detected (which is what the grammar uses if PUBLIC is found), so
-	 * downstream there shouldn't be any additional work needed to support
-	 * this case.
+	 * we insert an ACL_ID_PUBLIC into the list if appropriate, so downstream
+	 * there shouldn't be any additional work needed to support this case.
 	 */
 	foreach(cell, stmt-grantees)
 	{
 		PrivGrantee *grantee = (PrivGrantee *) lfirst(cell);
+		Oid grantee_uid = ACL_ID_PUBLIC;
 
-		if (grantee-rolname == NULL)
-			istmt.grantees = lappend_oid(istmt.grantees, ACL_ID_PUBLIC);
-		else
-			istmt.grantees =
-lappend_oid(istmt.grantees,
-			get_role_oid(grantee-rolname, false));
+		switch (grantee-role-roltype)
+		{
+			case ROLESPEC_PUBLIC:
+grantee_uid = ACL_ID_PUBLIC;
+break;
+			default:
+grantee_uid = get_rolespec_oid(grantee-role, false);
+break;
+		}
+		istmt.grantees = lappend_oid(istmt.grantees, grantee_uid);
 	}
 
 	/*
@@ -905,21 +908,24 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt)
 
 	/*
 	 * Convert the PrivGrantee list into an Oid list.  Note that at this point
-	 * we insert an ACL_ID_PUBLIC into the list if an empty role name is
-	 * detected (which is what the grammar uses if PUBLIC is found), so
-	 * downstream there shouldn't be any additional work needed to support
-	 * this case.
+	 * we insert an ACL_ID_PUBLIC into the list if appropriate, so downstream
+	 * there shouldn't be any additional work needed to support this case.
 	 */
 	foreach(cell, action-grantees)
 	{
 		PrivGrantee *grantee = (PrivGrantee *) lfirst(cell);
+		Oid grantee_uid = ACL_ID_PUBLIC;
 
-		if (grantee-rolname == NULL)
-			iacls.grantees = lappend_oid(iacls.grantees, ACL_ID_PUBLIC);
-		else
-			iacls.grantees =
-lappend_oid(iacls.grantees,
-			get_role_oid(grantee-rolname, false));
+		switch (grantee-role-roltype)
+		{
+			case ROLESPEC_PUBLIC:
+grantee_uid = ACL_ID_PUBLIC;
+break;
+			default:
+grantee_uid = get_rolespec_oid(grantee-role, false);
+break;
+		}
+		iacls.grantees = lappend_oid(iacls.grantees, grantee_uid);
 	}
 
 	/*
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 78b54b4..1d8799b 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -679,7 +679,7 @@ AlterObjectNamespace_internal(Relation rel, Oid

Re: [HACKERS] alter user/role CURRENT_USER

2015-01-27 Thread Kyotaro HORIGUCHI

Hello, thank you for the comment.

 Looking at this a bit, I'm not sure completely replacing RoleId with a
 node is the best idea; some of the users of that production in the
 grammar are okay with accepting only normal strings as names, and don't
 need all the CURRENT_USER etc stuff.

You're right. the productions don't need RoleSpec already uses
char* for the role member in its *Stmt structs. I might have done
a bit too much.

Adding new nonterminal RoleId and using it makes a bit duplicate
of check code for public/none and others but removes other
redundant check for != CSTRING from some production, CREATE
ROLE/USER/GROUP, ALTER ROLE/USER/GROUP RENAME.

RoleId in the patch still has rule components for CURRENT_USER,
SESSION_USER, and CURRENT_ROLE. Without them, the parser prints
an error ununderstandable to users.

| =# alter role current_user rename to PuBlic;
| ERROR:  syntax error at or near rename
| LINE 1: alter role current_user rename to PuBlic;
| ^

With the components, the error message becomes like this.

| =# alter role current_role rename to PuBlic;
| ERROR:  reserved word cannot be used
| LINE 1: alter role current_role rename to PuBlic;
|^


   Maybe we need a new production,
 say RoleSpec, and we modify the few productions that need the extra
 flexibility?  For instance we could have ALTER USER RoleSpec instead of
 ALTER USER RoleId.  But we would keep CREATE USER RoleId, because it
 doesn't make any sense to accept CREATE USER CURRENT_USER.
 I think that would lead to a less invasive patch also.
 Am I making sense?

I think I did what you expected. It was good as the code got
simpler but the invasive-ness dosn't seem to be reduced..

What do you think about this?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From d90b7e09968f32a5b543242469fb65e304df3318 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp
Date: Fri, 14 Nov 2014 17:37:22 +0900
Subject: [PATCH] ALTER USER CURRENT_USER v4

Added RoleId for the use in where CURRENT_USER-like sutff is not used.
CREATE ROLE/USER/GROUP, ALTER ROLE/USER/GROUP RENAME are changed to
use RoleId.
---
 src/backend/catalog/aclchk.c   |  30 +++--
 src/backend/commands/alter.c   |   2 +-
 src/backend/commands/extension.c   |   2 +-
 src/backend/commands/foreigncmds.c |  57 -
 src/backend/commands/schemacmds.c  |  30 -
 src/backend/commands/tablecmds.c   |   4 +-
 src/backend/commands/tablespace.c  |   2 +-
 src/backend/commands/user.c|  86 +++---
 src/backend/nodes/copyfuncs.c  |  37 --
 src/backend/nodes/equalfuncs.c |  35 --
 src/backend/parser/gram.y  | 229 +
 src/backend/parser/parse_utilcmd.c |   4 +-
 src/backend/utils/adt/acl.c|  34 ++
 src/include/commands/user.h|   2 +-
 src/include/nodes/nodes.h  |   1 +
 src/include/nodes/parsenodes.h |  48 ++--
 src/include/utils/acl.h|   2 +-
 17 files changed, 400 insertions(+), 205 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 1e3888e..1c90626 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -430,13 +430,16 @@ ExecuteGrantStmt(GrantStmt *stmt)
 	foreach(cell, stmt-grantees)
 	{
 		PrivGrantee *grantee = (PrivGrantee *) lfirst(cell);
+		Oid grantee_uid = ACL_ID_PUBLIC;
 
-		if (grantee-rolname == NULL)
-			istmt.grantees = lappend_oid(istmt.grantees, ACL_ID_PUBLIC);
-		else
-			istmt.grantees =
-lappend_oid(istmt.grantees,
-			get_role_oid(grantee-rolname, false));
+		/* public is mapped to ACL_ID_PUBLIC */
+		if (grantee-role-roltype != ROLESPEC_PUBLIC)
+		{
+			grantee_uid = get_rolespec_oid(grantee-role, false);
+			if (!OidIsValid(grantee_uid))
+grantee_uid = ACL_ID_PUBLIC;
+		}
+		istmt.grantees = lappend_oid(istmt.grantees, grantee_uid);
 	}
 
 	/*
@@ -913,13 +916,16 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt)
 	foreach(cell, action-grantees)
 	{
 		PrivGrantee *grantee = (PrivGrantee *) lfirst(cell);
+		Oid grantee_uid = ACL_ID_PUBLIC;
 
-		if (grantee-rolname == NULL)
-			iacls.grantees = lappend_oid(iacls.grantees, ACL_ID_PUBLIC);
-		else
-			iacls.grantees =
-lappend_oid(iacls.grantees,
-			get_role_oid(grantee-rolname, false));
+		/* public is mapped to ACL_ID_PUBLIC */
+		if (grantee-role-roltype != ROLESPEC_PUBLIC)
+		{
+			grantee_uid = get_rolespec_oid(grantee-role, false);
+			if (!OidIsValid(grantee_uid))
+grantee_uid = ACL_ID_PUBLIC;
+		}
+		iacls.grantees = lappend_oid(iacls.grantees, grantee_uid);
 	}
 
 	/*
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 78b54b4..1d8799b 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -679,7 +679,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
 Oid

Re: [HACKERS] alter user/role CURRENT_USER

2015-01-22 Thread Alvaro Herrera

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

2014-12-15 Thread Kyotaro HORIGUCHI

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

2014-12-14 Thread Michael Paquier

On Fri, Nov 14, 2014 at 5:39 PM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
 Hi, this is revised version.

 Kyotaro HORIGUCHI wrote:

  - Storage for new information
 
  The new struct NameId stores an identifier which telling what it
  logically is using the new enum NameIdTypes.

 I think NameId is a bad name for this.  My point is that NameId, as it
 stands, might be a name for anything, not just a role; and the object it
 identifies is not an Id either.  Maybe RoleSpec?

 Yeah! I felt it no good even if it were a generic type for
 various Name of something or its oid. RoleSpec sounds much better.

 Do we need a public_ok
 argument to get_nameid_oid() (get a better name for this function too)

 Maybe get_rolespec_oid() as a name ofter its parameter type?

 so that callers don't have to check for InvalidOid argument?  I think
 the arrangement you propose is not very convenient; it'd be best to
 avoid duplicating the check for InvalidOid in all callers of the new
 function, particularly where there was no check before.

 I agree that It'd be better keeping away from duplicated
 InvalidOid checks, but public_ok seems a bit myopic. Since
 there's no reasonable border between functions accepting 'public'
 and others, such kind of solution would not be reasonable..

 What about checking it being a PUBLIC or not *before* calling
 get_rolespec_oid()?

 The attached patch modified in the following points.

  - rename NameId to RoleSpec and NameIdType to RoleSpecTypes.
  - rename get_nameid_oid() to get_rolespec_oid().
  - rename roleNamesToIds() to roleSpecsToIds().
  - some struct members are changed such as authname to authrole.
  - check if rolespec is public or not before calling get_rolespec_oid()
  - ExecAlterDefaultPrivilegesStmt and ExecuteGrantStmt does
slightly different things about ACL_ID_PUBLIC but I unified it
to the latter.
  - rebased to the current master
A new patch has been sent here but no review occurred, hence moving
this item to CF 2014-12.
-- 
Michael


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

Re: [HACKERS] alter user/role CURRENT_USER

2014-11-14 Thread Kyotaro HORIGUCHI

Hi, this is revised version.

 Kyotaro HORIGUCHI wrote:
 
  - Storage for new information
  
  The new struct NameId stores an identifier which telling what it
  logically is using the new enum NameIdTypes.
 
 I think NameId is a bad name for this.  My point is that NameId, as it
 stands, might be a name for anything, not just a role; and the object it
 identifies is not an Id either.  Maybe RoleSpec?

Yeah! I felt it no good even if it were a generic type for
various Name of something or its oid. RoleSpec sounds much better.

 Do we need a public_ok
 argument to get_nameid_oid() (get a better name for this function too)

Maybe get_rolespec_oid() as a name ofter its parameter type?

 so that callers don't have to check for InvalidOid argument?  I think
 the arrangement you propose is not very convenient; it'd be best to
 avoid duplicating the check for InvalidOid in all callers of the new
 function, particularly where there was no check before.

I agree that It'd be better keeping away from duplicated
InvalidOid checks, but public_ok seems a bit myopic. Since
there's no reasonable border between functions accepting 'public'
and others, such kind of solution would not be reasonable..

What about checking it being a PUBLIC or not *before* calling
get_rolespec_oid()?

The attached patch modified in the following points.

 - rename NameId to RoleSpec and NameIdType to RoleSpecTypes.
 - rename get_nameid_oid() to get_rolespec_oid().
 - rename roleNamesToIds() to roleSpecsToIds().
 - some struct members are changed such as authname to authrole.
 - check if rolespec is public or not before calling get_rolespec_oid()
 - ExecAlterDefaultPrivilegesStmt and ExecuteGrantStmt does
   slightly different things about ACL_ID_PUBLIC but I unified it
   to the latter.
 - rebased to the current master

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

CreateStmt-authrole = NULL = ?
From 307249654c97b6449261febbfd84190fbad9111d Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp
Date: Fri, 14 Nov 2014 17:37:22 +0900
Subject: [PATCH] ALTER USER CURRENT_USER v3

---
 src/backend/catalog/aclchk.c   |  30 +++---
 src/backend/commands/alter.c   |   2 +-
 src/backend/commands/extension.c   |   2 +-
 src/backend/commands/foreigncmds.c |  57 +--
 src/backend/commands/schemacmds.c  |  30 +-
 src/backend/commands/tablecmds.c   |   4 +-
 src/backend/commands/tablespace.c  |   2 +-
 src/backend/commands/user.c|  86 +
 src/backend/nodes/copyfuncs.c  |  37 +---
 src/backend/nodes/equalfuncs.c |  35 ---
 src/backend/parser/gram.y  | 190 +++--
 src/backend/parser/parse_utilcmd.c |   4 +-
 src/backend/utils/adt/acl.c|  34 +++
 src/include/commands/user.h|   2 +-
 src/include/nodes/nodes.h  |   1 +
 src/include/nodes/parsenodes.h |  48 +++---
 src/include/utils/acl.h|   2 +-
 17 files changed, 385 insertions(+), 181 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index d30612c..24811c6 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -430,13 +430,16 @@ ExecuteGrantStmt(GrantStmt *stmt)
 	foreach(cell, stmt-grantees)
 	{
 		PrivGrantee *grantee = (PrivGrantee *) lfirst(cell);
+		Oid grantee_uid = ACL_ID_PUBLIC;
 
-		if (grantee-rolname == NULL)
-			istmt.grantees = lappend_oid(istmt.grantees, ACL_ID_PUBLIC);
-		else
-			istmt.grantees =
-lappend_oid(istmt.grantees,
-			get_role_oid(grantee-rolname, false));
+		/* public is mapped to ACL_ID_PUBLIC */
+		if (grantee-role-roltype != ROLESPEC_PUBLIC)
+		{
+			grantee_uid = get_rolespec_oid(grantee-role, false);
+			if (!OidIsValid(grantee_uid))
+grantee_uid = ACL_ID_PUBLIC;
+		}
+		istmt.grantees = lappend_oid(istmt.grantees, grantee_uid);
 	}
 
 	/*
@@ -913,13 +916,16 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt)
 	foreach(cell, action-grantees)
 	{
 		PrivGrantee *grantee = (PrivGrantee *) lfirst(cell);
+		Oid grantee_uid = ACL_ID_PUBLIC;
 
-		if (grantee-rolname == NULL)
-			iacls.grantees = lappend_oid(iacls.grantees, ACL_ID_PUBLIC);
-		else
-			iacls.grantees =
-lappend_oid(iacls.grantees,
-			get_role_oid(grantee-rolname, false));
+		/* public is mapped to ACL_ID_PUBLIC */
+		if (grantee-role-roltype != ROLESPEC_PUBLIC)
+		{
+			grantee_uid = get_rolespec_oid(grantee-role, false);
+			if (!OidIsValid(grantee_uid))
+grantee_uid = ACL_ID_PUBLIC;
+		}
+		iacls.grantees = lappend_oid(iacls.grantees, grantee_uid);
 	}
 
 	/*
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index c9a9baf..c53d4e5 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -678,7 +678,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
 Oid
 ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
 {
-	Oid			newowner = get_role_oid(stmt-newowner, false);
+	Oid

Re: [HACKERS] alter user/role CURRENT_USER

2014-11-13 Thread Alvaro Herrera

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

2014-11-10 Thread Kyotaro HORIGUCHI

Hello, This is the new version. (WIP v2)

The first attachment is the patch and the second is test sql
script.

- Behavior changing

Almost all syntax taking role accepts CURRENT_USER and
SESSION_USER and they are distinguished from current_user and
session_user. The exceptions are follows.

 - CREATE USER/GROUP roleid
 - ALTER ROLE/GROUP/USER roleid RENAME TO newname
 
 These syntax still do not accept the keywords like CURRENT_USER
 and special names like public at all, but accepts
 current_user. The error message is changed as follows.

| postgres=# create user current_user;
| ERROR:  role name should not be a keyword nor reserved name.
| LINE 1: create user current_user;
| ^

# Some other messages may changed...

USER and CURRENT_ROLE haven't been extended to other syntax. The
former still can be used only in CREATE/ALTER/DROP USER MAPPING
and the latter cannot be used out of function expressions.

- Storage for new information

The new struct NameId stores an identifier which telling what it
logically is using the new enum NameIdTypes.

This is still be a bit suffered by the difference between
CURRENT_USER and PUBLIC but now it makes distinction between
current_user and current_user. User oid does not have the room
for representing the difference among PUBLIC, NONE and 'not
found' as the result of get_nameid_oid(), so members of NameId is
exposed in foreigncmds.c and it gets a bit uglier.

-  Changes of related structs and grammar.

Type of role member is changed to NameId in some of parser
structs. AlterTableCmd.name has many other usage so I added new
member NameId *newowner for exclusive usage.

Type of OWNER clause of CREATE TABLESPACE is changed to RoleId. I
suppose there's no particular reason that the non-terminal was
name.

Usage of public and none had been blocked for CREATE/RENAME
USER in user.c but now it is blocked in gram.y


- New function to resolve NameId

New function get_nameid_oid() is added. It is an alternative of
get_role_oid which can handle current_user and current_user
properly. get_role_oid() still be used in many places having no
direct relation to syntax.

- Others

No doc provided for now.


regards,


  Adam Brightwell adam.brightw...@crunchydatasolutions.com writes:
   FWIW, I found the following in the archives:
  
   http://www.postgresql.org/message-id/15516.1038718...@sss.pgh.pa.us
  
   Now this is from 2002 and it appears it wasn't necessary to change at the
   time, but I haven't yet found anything else related (it's a big archive).
   Though, as I understand it, PUBLIC is now non-reserved as of SQL:2011 
   which
   might make a compelling argument to leave it as is?
  
  The current spec does list PUBLIC as a non-reserved keyword, but it also
  says (5.4 Names and identifiers syntax rules)
  
   20) No authorization identifier shall specify PUBLIC.
  
  which, oddly enough, seems to license us to handle PUBLIC the way
  we are doing.  OTOH, it lists CURRENT_USER as a reserved word, suggesting
  that they don't think the same type of hack should be used for that.
  
  I'd be inclined to leave the grammar as such alone (ie CURRENT_USER is
  a keyword, PUBLIC isn't).  Changing that has more downside than upside,
  and we do have justification in the spec for treating the two cases
  differently.  However, I agree that we should fix the subsequent
  processing so that current_user is not confused with CURRENT_USER.
 
 Sure, maybe.
 
  - PUBLIC should be left as it is, as an supecial identifier
which cannot be used even if quoted under some syntax.
 
  - CURRENT_USER should be a kayword as it is, but we shouldn't
inhibit it from being used as an identifier if
quoted. SESSION_USER and USER should be treated in the same way.
 
We don't want to use something other than string (prefixed by
zero-byte) as a matter of course:). And resolving the name to
roleid instantly in gram.y is not allowed for the reason shown
upthread.
 
So it is necessary to add a new member for the struct, say
int special_role;:... Let me have more sane name for it :(
 
  - USER and CURRENT_ROLE are not needed for the syntax other than
them which already uses them.
 
 I will work on this way. Let me know if something is not acceptable.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From f18d078d5e6e4005e803ecc954e59c899dbfd557 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp
Date: Mon, 10 Nov 2014 19:08:42 +0900
Subject: [PATCH] ALTER USER CURRENT_USER v2

---
 src/backend/catalog/aclchk.c   |  13 ++-
 src/backend/commands/alter.c   |   2 +-
 src/backend/commands/foreigncmds.c |  39 -
 src/backend/commands/schemacmds.c  |  26 +-
 src/backend/commands/tablecmds.c   |   2 +-
 src/backend/commands/tablespace.c  |   2 +-
 src/backend/commands/user.c|  70 +++
 src/backend/nodes/copyfuncs.c  |  37 +---
 src/backend/nodes/equalfuncs.c |  35 +---

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-31 Thread Adam Brightwell

All,


 I agree that we should probably seperate the concerns here.  Personally,
 I like the idea of being able to say CURRENT_USER in utility commands
 to refer to the current user where a role would normally be expected, as
 I could see it simplifying things for some applications, but that's a
 new feature and independent of making role-vs-user cases more
 consistent.


So, I've been doing a little digging and it would appear that the ALTER
ROLE/USER consistency was brought up earlier in the year.

http://www.postgresql.org/message-id/cadyruxmv-tvsbv7mttcs+qedny6xj1+krtzfowvuhdjc5mg...@mail.gmail.com

It was returned with feedback in Commitfest 2014-06 and apparently lost
steam:

https://commitfest.postgresql.org/action/patch_view?id=1408

Tom put forward a suggestion for how to fix it:

http://www.postgresql.org/message-id/21570.1408832...@sss.pgh.pa.us

I have taken that patch and updated the documentation (attached) and ran it
through some cursory testing.

At any rate, this is probably a good starting point for those changes.

-Adam

-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/doc/src/sgml/ref/alter_user.sgml b/doc/src/sgml/ref/alter_user.sgml
new file mode 100644
index 58ae1da..ac05682
*** a/doc/src/sgml/ref/alter_user.sgml
--- b/doc/src/sgml/ref/alter_user.sgml
*** ALTER USER replaceable class=PARAMETER
*** 38,47 
  
  ALTER USER replaceable class=PARAMETERname/replaceable RENAME TO replaceablenew_name/replaceable
  
! ALTER USER replaceable class=PARAMETERname/replaceable SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT }
! ALTER USER replaceable class=PARAMETERname/replaceable SET replaceableconfiguration_parameter/replaceable FROM CURRENT
! ALTER USER replaceable class=PARAMETERname/replaceable RESET replaceableconfiguration_parameter/replaceable
! ALTER USER replaceable class=PARAMETERname/replaceable RESET ALL
  /synopsis
   /refsynopsisdiv
  
--- 38,47 
  
  ALTER USER replaceable class=PARAMETERname/replaceable RENAME TO replaceablenew_name/replaceable
  
! ALTER USER replaceable class=PARAMETERname/replaceable [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT }
! ALTER USER { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable FROM CURRENT
! ALTER USER { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET replaceableconfiguration_parameter/replaceable
! ALTER USER { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET ALL
  /synopsis
   /refsynopsisdiv
  
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
new file mode 100644
index 0de9584..d7c0586
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*** static Node *makeRecursiveViewSelect(cha
*** 230,236 
  		AlterFdwStmt AlterForeignServerStmt AlterGroupStmt
  		AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterSystemStmt AlterTableStmt
  		AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt AlterForeignTableStmt
! 		AlterCompositeTypeStmt AlterUserStmt AlterUserMappingStmt AlterUserSetStmt
  		AlterRoleStmt AlterRoleSetStmt AlterPolicyStmt
  		AlterDefaultPrivilegesStmt DefACLAction
  		AnalyzeStmt ClosePortalStmt ClusterStmt CommentStmt
--- 230,236 
  		AlterFdwStmt AlterForeignServerStmt AlterGroupStmt
  		AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterSystemStmt AlterTableStmt
  		AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt AlterForeignTableStmt
! 		AlterCompositeTypeStmt AlterUserMappingStmt
  		AlterRoleStmt AlterRoleSetStmt AlterPolicyStmt
  		AlterDefaultPrivilegesStmt DefACLAction
  		AnalyzeStmt ClosePortalStmt ClusterStmt CommentStmt
*** static Node *makeRecursiveViewSelect(cha
*** 520,525 
--- 520,527 
  %type str		opt_existing_window_name
  %type boolean opt_if_not_exists
  
+ %type str		role_or_user
+ 
  /*
   * Non-keyword token types.  These are hard-wired into the flex lexer.
   * They must be listed first so that their numeric codes do not depend on
*** stmt :
*** 756,763 
  			| AlterTSConfigurationStmt
  			| AlterTSDictionaryStmt
  			| AlterUserMappingStmt
- 			| AlterUserSetStmt
- 			| AlterUserStmt
  			| AnalyzeStmt
  			| CheckPointStmt
  			| ClosePortalStmt
--- 758,763 
*** CreateUserStmt:
*** 1033,1042 
   *
   * Alter a postgresql DBMS role
   *
   */
  
  AlterRoleStmt:
! 			ALTER ROLE RoleId opt_with AlterOptRoleList
   {
  	AlterRoleStmt *n =

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-30 Thread Kyotaro HORIGUCHI

Hello,

At Tue, 28 Oct 2014 09:05:20 -0400, Stephen Frost sfr...@snowman.net wrote in 
20141028130520.gl28...@tamriel.snowman.net
  As well, the
  originally proposed RoleId_or_curruser suffers from the same issue.  I'm
  going to go out on a limb here, but is it not possible to consider
  current_user, etc. reserved in the same sense that we do with PUBLIC and
  NONE and disallow users/roles from being created as them?
 
 Maybe we could in some future release, but we can't for back-branches so
 I'm afraid we're going to have to figure out how to fix this to work
 regardless.

Zero-length identifiers are rejected in scanner so RoleId cannot
be a valid pointer to a zero-length string. (NULL is used as
PUBLIC in auth_ident)

| postgres=# create role ;
| ERROR:  zero-length delimited identifier at or near 
| postgres=# create role U\00;
| ERROR:  invalid Unicode escape value at or near \00

As a dirty and quick hack we can use some integer values prfixed
by single zero byte to represent special roles such as
CURRENT_USER.

| RoleId_or_curruser: RoleId{ $$ = $1; }
|   | CURRENT_USER  { $$ = \x00\x01;};

| Oid ResolveRoleId(const char *name, bool missing_ok)
| {
|   Oid roleid;
| 
|   if (name[0] == 0  name[1] == 1)
|   roleid = GetUserId();

This is ugly but needs no additional struct member or special
logics. (Macros could make them look better.)

  Taking a step back, I'm still not sure I understand the need for this
  feature or the use case.  It seems to have started as a potential fix to an
  inconsistency between ALTER USER and ALTER ROLE syntax (which I think I
  could see some value in).  However, I think it has been taken beyond just
  resolving the inconsistency and started to cross over into feature creep.
  Is the intent simply to resolve inconsistencies between what is now an
  alias of another command?  Or is it to add new functionality?  I think the
  original proposal needs to be revisited and more time needs to be spent
  defining the scope and purpose of this patch.
 
 I agree that we should probably seperate the concerns here.  Personally,
 I like the idea of being able to say CURRENT_USER in utility commands
 to refer to the current user where a role would normally be expected, as
 I could see it simplifying things for some applications, but that's a
 new feature and independent of making role-vs-user cases more
 consistent.

I agree that role-vs-user compatibility is another problem.

In a sense, the CURRENT_USER problem is also a separate problem
but simplly spreading current current_user mechanism (which
cannot allow using the words literally even if double-quoted) out
to other commands is a kind of pandemic so it should be fixed
before making current_user usable in other commands.

From a view of comptibility (specification stability), fixing
this problem could break someone's application if he/she uses
current_user in the meaning of CURRENT_USER intentionally but I
think it is least likely.


As another problem, in the defenition of grantee, there is the
following comment.

|   /* This hack lets us avoid reserving PUBLIC as a keyword*/
|   if (strcmp($1, public) == 0)

Please let me know the reason to avoid registering new keyword
making the word unusable as an literal identifier, if any?

PUBLIC and NONE ought to can be identifiers but in reality
unusable because they are handled as keywords in literal
form. Thses are fixed by making them real keywords.

So if there's no particular reason, I will register new keywords
PUBLIC and NONE as another patch.

# However, I don't think that considerable number of people want
# to do that..

On the other hand, pg_* as shcmea names and operators are cases
simply of special names, which cannot be identifiers from the
first so it should be as it is, I think.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-30 Thread Adam Brightwell

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

2014-10-30 Thread Stephen Frost

* 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

2014-10-30 Thread Tom Lane

Stephen Frost sfr...@snowman.net writes:
 The other idea which comes to mind is- could we try to actually resolve
 what the 'right' answer is here, instead of setting a special value and
 then having to detect and fix it later?

No, absolutely not.  Read the NOTES at the head of gram.y.  Or if you
need it spelled out: when we run the bison parser, we may not be inside a
transaction at all, and even if we are, we aren't necessarily going to
be seeing the same current user that will apply when the parsetree is
ultimately executed.  (Consider a query inside a plpgsql function, which
might be called under multiple userids over the course of a session.)

I think Alvaro's suggestion is a perfectly appropriate solution.

regards, tom lane


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

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-30 Thread Tom Lane

Adam Brightwell adam.brightw...@crunchydatasolutions.com writes:
 FWIW, I found the following in the archives:

 http://www.postgresql.org/message-id/15516.1038718...@sss.pgh.pa.us

 Now this is from 2002 and it appears it wasn't necessary to change at the
 time, but I haven't yet found anything else related (it's a big archive).
 Though, as I understand it, PUBLIC is now non-reserved as of SQL:2011 which
 might make a compelling argument to leave it as is?

The current spec does list PUBLIC as a non-reserved keyword, but it also
says (5.4 Names and identifiers syntax rules)

 20) No authorization identifier shall specify PUBLIC.

which, oddly enough, seems to license us to handle PUBLIC the way
we are doing.  OTOH, it lists CURRENT_USER as a reserved word, suggesting
that they don't think the same type of hack should be used for that.

I'd be inclined to leave the grammar as such alone (ie CURRENT_USER is
a keyword, PUBLIC isn't).  Changing that has more downside than upside,
and we do have justification in the spec for treating the two cases
differently.  However, I agree that we should fix the subsequent
processing so that current_user is not confused with CURRENT_USER.

regards, tom lane


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

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-29 Thread Kyotaro HORIGUCHI

Hello, thank you all for many comments.

At the first, I removed changes for role-vs-user consistency and
remove all added role named other than current_user.

The followings are one-by-one answer for the comments so far,
please let me know if I missed anything.

- The necessity of the new function ResolveRoleId()

 It is very brief function but used in many place where the role
 name should be treated in the same way, so I think encapsulation
 is needed in some extent and in any form. It could be merged
 with get_role_oid.

- About changes in foreigncmds.c

 I removed the refactoring using ResolveRoleId() in this patch.

- RoleId_or_curruser separate from RoleId.

 There seems to be places where 'current_user' and like is not
 appropraite to occur such as CREATE USER. I don't mind to remove
 the non-terminal if it is needless consideration.

- GRANT is not modified.

 I thought GRANT is not appropriate but it seems appropriate
 seeing your example. And grantee takes public. I changed
 GRANT/REVOKE to take current_user in this patch.

- (not a comment) CREATE SCHEMA needed additonal aid

 Schema name can be omitted in CREATE SCHEMA and role name is
 used for it, so CREATE SCHEMA AUTHORIZATION current_user
 crates the schema current_user in the previous patch. This
 should be the real name of current_user.

This patch is for reviewing at a glance for food of discussion
and tested very briefly (and what is worse, it might even not be
applicable). I'll repost more refined version in this way and
other portions.

At Tue, 28 Oct 2014 12:16:13 -0400, Stephen Frost sfr...@snowman.net wrote in 
20141028161613.gt28...@tamriel.snowman.net
 * Kevin Grittner (kgri...@ymail.com) wrote:
  It is very important that a quoted identifier not be treated as a
  keyword.  I would be very interested in seeing that list, and in
  ensuring that it doesn't get any longer.
 
 It's object specific and not handled through the grammar, so that gets
 pretty annoying. :/
 
 The ones I could find by a quick look through backend/commands are:
 
 roles
   public
   none
 
 schemas
   pg_*
 
 operator
   = (throws a warning at least)
 
 There may be other cases that my quick review didn't find, of course.

Hmm... This seems to be another issue, though. I'll be careful
not to make it worse..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From ebe2d8b5549eddbd073b65aabe15af9299b948f6 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp
Date: Wed, 29 Oct 2014 17:38:46 +0900
Subject: [PATCH] CURRENT_USER_WIP_v1

---
 src/backend/commands/alter.c  |  2 +-
 src/backend/commands/schemacmds.c | 20 ++-
 src/backend/commands/tablecmds.c  |  2 +-
 src/backend/commands/user.c   | 48 ++
 src/backend/parser/gram.y | 71 ---
 src/include/commands/dbcommands.h |  1 +
 src/include/commands/user.h   |  1 +
 7 files changed, 93 insertions(+), 52 deletions(-)

diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index c9a9baf..1f598f6 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -678,7 +678,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
 Oid
 ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
 {
-	Oid			newowner = get_role_oid(stmt-newowner, false);
+	Oid			newowner = ResolveRoleId(stmt-newowner, false);
 
 	switch (stmt-objectType)
 	{
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index 03f5514..d67789a 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -23,8 +23,10 @@
 #include catalog/namespace.h
 #include catalog/objectaccess.h
 #include catalog/pg_namespace.h
+#include catalog/pg_authid.h
 #include commands/dbcommands.h
 #include commands/schemacmds.h
+#include commands/user.h
 #include miscadmin.h
 #include parser/parse_utilcmd.h
 #include tcop/utility.h
@@ -59,10 +61,26 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString)
 	 * Who is supposed to own the new schema?
 	 */
 	if (authId)
-		owner_uid = get_role_oid(authId, false);
+		owner_uid = ResolveRoleId(authId, false);
 	else
 		owner_uid = saved_uid;
 
+	/* Use the name for the owner_uid as the schema name if it is omitted */
+	if (!schemaName)
+	{
+		HeapTuple	tuple;
+
+		tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(owner_uid));
+		if (!HeapTupleIsValid(tuple))
+			ereport(ERROR,
+	(errcode(ERRCODE_UNDEFINED_OBJECT),
+	 errmsg(roleid %d does not exist, owner_uid)));
+		
+		schemaName =
+			strdup(((Form_pg_authid) GETSTRUCT(tuple))-rolname.data);
+		ReleaseSysCache(tuple);
+	}
+
 	/*
 	 * To create a schema, must have schema-create privilege on the current
 	 * database and must be able to become the target role (this does not
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ecdff1e..0b82765 100644
--- a/src/backend/commands/tablecmds.c
+++

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-28 Thread Adam Brightwell


 +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

2014-10-28 Thread Stephen Frost

* 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

2014-10-28 Thread Kevin Grittner

Stephen Frost sfr...@snowman.net wrote:

 You can still double-quote reserved words and use them in general.  What
 we're talking about here are cases where a word can't be used even if
 it's double-quoted, and we try really hard to keep those cases at an
 absolute minimum.  We should also really be keeping a list of those
 cases somewhere, now that I think about it..

It is very important that a quoted identifier not be treated as a
keyword.  I would be very interested in seeing that list, and in
ensuring that it doesn't get any longer.

 I agree that we should probably seperate the concerns here.  Personally,
 I like the idea of being able to say CURRENT_USER in utility commands
 to refer to the current user where a role would normally be expected, as
 I could see it simplifying things for some applications, but that's a
 new feature and independent of making role-vs-user cases more
 consistent.

Yeah, let's not mix those in the same patch.

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


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

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-28 Thread Robert Haas

On Tue, Oct 28, 2014 at 2:40 AM, Adam Brightwell
adam.brightw...@crunchydatasolutions.com wrote:
 Taking a step back, I'm still not sure I understand the need for this
 feature or the use case.  It seems to have started as a potential fix to an
 inconsistency between ALTER USER and ALTER ROLE syntax (which I think I
 could see some value in).  However, I think it has been taken beyond just
 resolving the inconsistency and started to cross over into feature creep.
 Is the intent simply to resolve inconsistencies between what is now an alias
 of another command?  Or is it to add new functionality?  I think the
 original proposal needs to be revisited and more time needs to be spent
 defining the scope and purpose of this patch.

+1.  I've been reading this thread with some bemusement, but couldn't
find a way articulate what you just said nearly as well as you just
said it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-28 Thread Stephen Frost

* Kevin Grittner (kgri...@ymail.com) wrote:
 Stephen Frost sfr...@snowman.net wrote:
 
  You can still double-quote reserved words and use them in general.  What
  we're talking about here are cases where a word can't be used even if
  it's double-quoted, and we try really hard to keep those cases at an
  absolute minimum.  We should also really be keeping a list of those
  cases somewhere, now that I think about it..
 
 It is very important that a quoted identifier not be treated as a
 keyword.  I would be very interested in seeing that list, and in
 ensuring that it doesn't get any longer.

It's object specific and not handled through the grammar, so that gets
pretty annoying. :/

The ones I could find by a quick look through backend/commands are:

roles
  public
  none

schemas
  pg_*

operator
  = (throws a warning at least)

There may be other cases that my quick review didn't find, of course.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-28 Thread Alvaro Herrera

Marti Raudsepp wrote:
 On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
 horiguchi.kyot...@lab.ntt.co.jp wrote:
   - 0001-ALTER-ROLE-CURRENT_USER_v2.patch  - the patch.
 
 +RoleId:CURRENT_USER{ $$ = 
 current_user;}
 +   | USER  { $$ = current_user;}
 +   | CURRENT_ROLE  { $$ = current_user;}
 +   | SESSION_USER  { $$ = session_user;}
 
 This is kind of ugly, and it means you can't distinguish between a
 CURRENT_USER keyword and a quoted user name current_user. It's a
 legitimate user name, so the behavior of the following now changes:
 
 CREATE ROLE current_user;
 ALTER ROLE current_user SET work_mem='10MB';
 
 There ought to be a better way to represent this than using magic string 
 values.

Agreed.  Since the current_user disease has already infected the USER
MAPPING stuff, I think we need to solve that problem -- how about having
this production return a new node which has either a string name or
flags for the various acceptable keywords?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-27 Thread Adam Brightwell

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

2014-10-27 Thread Marti Raudsepp

On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
  - 0001-ALTER-ROLE-CURRENT_USER_v2.patch  - the patch.

+RoleId:CURRENT_USER{ $$ = current_user;}
+   | USER  { $$ = current_user;}
+   | CURRENT_ROLE  { $$ = current_user;}
+   | SESSION_USER  { $$ = session_user;}

This is kind of ugly, and it means you can't distinguish between a
CURRENT_USER keyword and a quoted user name current_user. It's a
legitimate user name, so the behavior of the following now changes:

CREATE ROLE current_user;
ALTER ROLE current_user SET work_mem='10MB';

There ought to be a better way to represent this than using magic string values.


It accepts CURRENT_USER/USER/CURRENT_ROLE/SESSION_USER.

On a stylistic note, do we really want to support the alternative
spellings of CURRENT_USER, like CURRENT_ROLE and USER? The SQL
standard is well-hated for inventing more keywords than necessary.
There is no precedent for using/allowing these aliases in PostgreSQL
DDL commands, and ALTER USER  ROLE aren't SQL standard anyway. So
maybe we should stick with just accepting one canonical syntax
instead.

I think the word USER may reasonably arise from an editing mistake,
ALTER USER USER does not seem like sane syntax that should be
accepted.


From your original email:

 - Modify syntax of ALTER USER so as to be an alias of ALTER ROLE.
   - Added ALL syntax as user name to ALTER USER.

But should ALTER USER ALL and ALTER ROLE ALL really do the same thing?
A user is a role with the LOGIN option. Every user is a role, but not
every role is a user. I suspect that ambiguity was why ALTER USER ALL
wasn't originally implemented.

Regards,
Marti


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

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-27 Thread David G Johnston

Marti Raudsepp wrote
On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
lt;

horiguchi.kyotaro@.co

gt; wrote:

But should ALTER USER ALL and ALTER ROLE ALL really do the same thing?
A user is a role with the LOGIN option. Every user is a role, but not
every role is a user. I suspect that ambiguity was why ALTER USER ALL
wasn't originally implemented.

There is no system level distinction here - only semantic and only during
the issuance of a CREATE without specifying an explicit value for
LOGIN/NOLGIN.

The documentation states user and group are aliases for ROLE, not subsets
that require or disallow login privileges.

I am OK with not making them true aliases in order to minimize user
confusion w.r.t. the semantics implied by user and group but then I'd submit
we simply note those two forms as deprecated in favor of role and that all
new role-based functionality will only be attached to role-based commands.

Since all of user, current_user and session_user have special syntactic
consideration in SQL [1] I'd be generally in favor of trying to keep that
dynamic intact while implementing this feature. And for the same reason I
would not allow current_role as a keyword. We didn't add a current_role
function but instead chose to rely on the standard keywords even when we
introduced ROLE.

I'm not clear on the keyword confusion since while current_user is a valid
literal it requires quotation while the token current_user does not.

1. http://www.postgresql.org/docs/9.4/static/functions-info.html

David J.

--
View this message in context:
http://postgresql.1045698.n5.nabble.com/alter-user-role-CURRENT-USER-tp5822520p5824528.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

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

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-26 Thread Rushabh Lathia

Thanks Kyotaro,

I just did quickly looked at the patch and it does cover more syntax then
earlier patch. But still if doesn't not cover the all the part of syntax
where
we can use CURRENT_USER/CURRENT_ROLE/USER/SESSION_USER. For example:

-- Not working
alter default privileges for role current_user grant SELECT ON TABLES TO
current_user ;

-- Not working
grant user1 TO current_user;

Their might few more syntax like this.

I understand that patch is  sightly getting bigger and complex then what it
was
originally proposed. Before going back to more review on latest patch I
would
like to understand the requirement of this new feature. Also would like
others
to comment on where/how we should restrict this feature ?

On Fri, Oct 24, 2014 at 1:59 PM, Kyotaro HORIGUCHI 
horiguchi.kyot...@lab.ntt.co.jp wrote:

 Hi, here is the revised patch.

 Attached files are the followings

  - 0001-ALTER-ROLE-CURRENT_USER_v2.patch  - the patch.

  - testset.tar.bz2 - test set. Run by typing 'make check' as a
superuser of the running postgreSQL server. It creates testdb
and some roles.

 Documents are not edited this time.

 

 Considering your comments, I found more points to modify.

  - CREATE SCHEMA (IF NOT EXISTS) .. AUTHORIZATION role ...

  - ALTER AGGREAGE/COLLATION/etc... OWNER TO role

  - CREATE/ALTER/DROP USER MAPPING FOR role SERVER ..

 GRANT/REVOKE also takes role as an arguemnt but CURRENT_USER and
 the similar keywords seem to be useless for them.

 Finally, the new patch modifies the following points.

 In gram.y,

  - RoleId's are replaced with RoleId_or_curruser in more places.
It accepts CURRENT_USER/USER/CURRENT_ROLE/SESSION_USER.

  - ALTER USER ALL syntax is added. (not changed from the previous patch)

  - The non-terminal auth_ident now uses RoleId_or_curruser
instead of RoleId. This affects CREATE/ALTER/DROP USER MAPPING

 In user.c, new function ResolveRoleId() is added and used for all
 role ID resolutions that correspond to the syntax changes in
 parser. It is AlterRole() in user.c.

 In foreigncmds.c, GetUserOidFromMapping() is removed and
 ResolveRoleId is used instead.

 In alter.c and tablecmds.c, all calls to get_role_oid()
 correspond the the grammer change were replaced with
 ResolveRoleId().

 The modifications are a bit complicated so I provided a
 comprehensive test set.


 regards,

 --
 Kyotaro Horiguchi
 NTT Open Source Software Center




-- 
Rushabh Lathia

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-24 Thread Kyotaro HORIGUCHI

Hi, here is the revised patch.

Attached files are the followings

 - 0001-ALTER-ROLE-CURRENT_USER_v2.patch  - the patch.

 - testset.tar.bz2 - test set. Run by typing 'make check' as a
   superuser of the running postgreSQL server. It creates testdb
   and some roles.

Documents are not edited this time.

 

Considering your comments, I found more points to modify.

 - CREATE SCHEMA (IF NOT EXISTS) .. AUTHORIZATION role ...

 - ALTER AGGREAGE/COLLATION/etc... OWNER TO role

 - CREATE/ALTER/DROP USER MAPPING FOR role SERVER ..

GRANT/REVOKE also takes role as an arguemnt but CURRENT_USER and
the similar keywords seem to be useless for them.

Finally, the new patch modifies the following points.

In gram.y,

 - RoleId's are replaced with RoleId_or_curruser in more places.
   It accepts CURRENT_USER/USER/CURRENT_ROLE/SESSION_USER.

 - ALTER USER ALL syntax is added. (not changed from the previous patch)

 - The non-terminal auth_ident now uses RoleId_or_curruser
   instead of RoleId. This affects CREATE/ALTER/DROP USER MAPPING

In user.c, new function ResolveRoleId() is added and used for all
role ID resolutions that correspond to the syntax changes in
parser. It is AlterRole() in user.c.

In foreigncmds.c, GetUserOidFromMapping() is removed and
ResolveRoleId is used instead.

In alter.c and tablecmds.c, all calls to get_role_oid()
correspond the the grammer change were replaced with
ResolveRoleId().

The modifications are a bit complicated so I provided a
comprehensive test set.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From f55494a49b6d4c7eb32665ea9cc63634f5000c99 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi horiguchi.kyot...@lab.ntt.co.jp
Date: Tue, 9 Sep 2014 19:26:24 +0900
Subject: [PATCH] ALTER ROLE CURRENT_USER

---
 src/backend/commands/alter.c   |  2 +-
 src/backend/commands/foreigncmds.c | 25 ++--
 src/backend/commands/schemacmds.c  |  3 +-
 src/backend/commands/tablecmds.c   |  2 +-
 src/backend/commands/user.c| 56 ++-
 src/backend/parser/gram.y  | 78 ++
 src/include/commands/dbcommands.h  |  1 +
 src/include/commands/user.h|  2 +
 8 files changed, 96 insertions(+), 73 deletions(-)

diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index c9a9baf..15f254e 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -678,7 +678,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
 Oid
 ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
 {
-	Oid			newowner = get_role_oid(stmt-newowner, false);
+	Oid			newowner = ResolveRoleId(stmt-newowner, false, NULL);
 
 	switch (stmt-objectType)
 	{
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index ab4ed6c..9878fca 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -27,6 +27,7 @@
 #include catalog/pg_type.h
 #include catalog/pg_user_mapping.h
 #include commands/defrem.h
+#include commands/user.h
 #include foreign/fdwapi.h
 #include foreign/foreign.h
 #include miscadmin.h
@@ -198,24 +199,6 @@ transformGenericOptions(Oid catalogId,
 
 
 /*
- * Convert the user mapping user name to OID
- */
-static Oid
-GetUserOidFromMapping(const char *username, bool missing_ok)
-{
-	if (!username)
-		/* PUBLIC user mapping */
-		return InvalidOid;
-
-	if (strcmp(username, current_user) == 0)
-		/* map to the owner */
-		return GetUserId();
-
-	/* map to provided user */
-	return get_role_oid(username, missing_ok);
-}
-
-/*
  * Internal workhorse for changing a data wrapper's owner.
  *
  * Allow this only for superusers; also the new owner must be a
@@ -1099,7 +1082,7 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
 
 	rel = heap_open(UserMappingRelationId, RowExclusiveLock);
 
-	useId = GetUserOidFromMapping(stmt-username, false);
+	useId = ResolveRoleId(stmt-username, false, NULL);
 
 	/* Check that the server exists. */
 	srv = GetForeignServerByName(stmt-servername, false);
@@ -1194,7 +1177,7 @@ AlterUserMapping(AlterUserMappingStmt *stmt)
 
 	rel = heap_open(UserMappingRelationId, RowExclusiveLock);
 
-	useId = GetUserOidFromMapping(stmt-username, false);
+	useId = ResolveRoleId(stmt-username, false, NULL);
 	srv = GetForeignServerByName(stmt-servername, false);
 
 	umId = GetSysCacheOid2(USERMAPPINGUSERSERVER,
@@ -1276,7 +1259,7 @@ RemoveUserMapping(DropUserMappingStmt *stmt)
 	Oid			umId;
 	ForeignServer *srv;
 
-	useId = GetUserOidFromMapping(stmt-username, stmt-missing_ok);
+	useId = ResolveRoleId(stmt-username, stmt-missing_ok, NULL);
 	srv = GetForeignServerByName(stmt-servername, true);
 
 	if (stmt-username  !OidIsValid(useId))
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index 03f5514..4f97f23 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -25,6 +25,7 @@
 #include catalog/pg_namespace.h
 #include commands/dbcommands.h
 #include

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-21 Thread Kyotaro HORIGUCHI

Thank you for reviewing,

 2014 13:10:57 +0530, Rushabh Lathia rushabh.lat...@gmail.com wrote in 
cagpqqf0kdfajizx0vca_-wazwu+xj5mdnl-hgg1sez9aw3c...@mail.gmail.com
 I gone through patch and here is the review for this patch:
 
 
 .) patch go applied on master branch with patch -p1 command
(git apply failed)
 .) regression make check run fine
 .) testcase coverage is missing in the patch
 .) Over all coding/patch looks good.
 
 Few comments:
 
 1) Any particular reason for not adding SESSION_USER/USER also made usable
 for this command ?

No particular reson. This patch was the first step and if this is
the adequate way and adding them is desirable, I will add them.

 2) I think RoleId_or_curruser can be used for following role:
 
 /* ALTER TABLE name OWNER TO RoleId */
 | OWNER TO RoleId

Good catch. I'll try it.

 3) In the documentation patch, added for CURRENT_USER but CURRENT_ROLE is
 missing.

Mmm.. I didn't added CURRENT_ROLE in the previous patch.

I suppose CURRENT_ROLE is an implicit alias of CURRENT_USER
because it is not explained in the page below in spite of
existing in syntax.

http://www.postgresql.org/docs/9.4/static/functions-info.html

and it is currently usable only in expressions, so the following
SQL failed and all syntax using auth_ident will fail.

postgres=# CREATE USER MAPPING FOR CURRENT_ROLE SERVER s1;
ERROR:  syntax error at or near current_role

share/doc/html/sql-createusermapping.html

| Synopsis
| 
| CREATE USER MAPPING FOR { user_name | USER | CURRENT_USER | PUBLIC }

I don't know why the 'USER' is added here, but anyway I can add
'CURRENT_ROLE' there in gram.y but it seems not necessary to add
it to document.


Ok, I'll modify this patch so that,

  - Make CURRENT_USER/ROLE usable in TABLE OWNER TO.

and since adding CURRENT_ROLE is the another thing, I'll do the
following things as additional patch.

 - Add USER, CURRENT_ROLE and SESSION_* for the place where
   CURRENT_USER is usable now.  auth_ident and
   RoleId_or_curruser.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-21 Thread Kyotaro HORIGUCHI

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

2014-10-20 Thread Rushabh Lathia

I gone through patch and here is the review for this patch:


.) patch go applied on master branch with patch -p1 command
   (git apply failed)
.) regression make check run fine
.) testcase coverage is missing in the patch
.) Over all coding/patch looks good.

Few comments:

1) Any particular reason for not adding SESSION_USER/USER also made usable
for this command ?

2) I think RoleId_or_curruser can be used for following role:

/* ALTER TABLE name OWNER TO RoleId */
| OWNER TO RoleId

3) In the documentation patch, added for CURRENT_USER but CURRENT_ROLE is
missing.


On Fri, Oct 10, 2014 at 1:57 PM, Kyotaro HORIGUCHI 
horiguchi.kyot...@lab.ntt.co.jp wrote:

 Hello, on the way considering alter role set .., I found that
 ALTER ROLE/USER cannot take CURRENT_USER as the role name.

 In addition to that, documents of ALTER ROLE / USER are
 inconsistent with each other in spite of ALTER USER is said to be
 an alias for ALTER ROLE. Plus, ALTER USER cannot take ALL as user
 name although ALTER ROLE can.

 This patch does following things,

  - ALTER USER/ROLE now takes CURRENT_USER as user name.

  - Rewrite sysnopsis of the documents for ALTER USER and ALTER
ROLE so as to they have exactly same syntax.

  - Modify syntax of ALTER USER so as to be an alias of ALTER ROLE.

- Added CURRENT_USER/CURRENT_ROLE syntax to both.
- Added ALL syntax as user name to ALTER USER.
- Added IN DATABASE syntax to ALTER USER.

x Integrating ALTER ROLE/USER syntax could not be done because of
  shift/reduce error of bison.

  x This patch contains no additional regressions. Is it needed?

 SESSION_USER/USER also can be made usable for this command, but
 this patch doesn't so (yet).

 regards,

 --
 Kyotaro Horiguchi
 NTT Open Source Software Center


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




-- 
Rushabh Lathia

Re: [HACKERS] alter user/role CURRENT_USER

2014-10-20 Thread Brightwell, Adam

Kyotaro,

Food for thought.  Couldn't you reduce the following block:

+ if (strcmp(stmt-role, current_user) == 0)
+ {
+ roleid = GetUserId();
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg(roleid %d does not exist, roleid)));
+ }
+ else
+ {
+ tuple = SearchSysCache1(AUTHNAME, PointerGetDatum(stmt-role));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg(role \%s\ does not exist, stmt-role)));

To:

if (strcmp(stmt-role, current_user) == 0)
roleid = GetUserId();
else
roleid = get_role_oid(stmt-role, false);

tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));

if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
 errmsg(roleid %d does not exist, roleid)));

I think this makes it a bit cleaner.  It also reuses existing code as
'get_role_oid()' already does a valid role name check and will raise the
proper error.

-Adam


On Mon, Oct 20, 2014 at 3:40 AM, Rushabh Lathia rushabh.lat...@gmail.com
wrote:

 I gone through patch and here is the review for this patch:


 .) patch go applied on master branch with patch -p1 command
(git apply failed)
 .) regression make check run fine
 .) testcase coverage is missing in the patch
 .) Over all coding/patch looks good.

 Few comments:

 1) Any particular reason for not adding SESSION_USER/USER also made usable
 for this command ?

 2) I think RoleId_or_curruser can be used for following role:

 /* ALTER TABLE name OWNER TO RoleId */
 | OWNER TO RoleId

 3) In the documentation patch, added for CURRENT_USER but CURRENT_ROLE is
 missing.


 On Fri, Oct 10, 2014 at 1:57 PM, Kyotaro HORIGUCHI 
 horiguchi.kyot...@lab.ntt.co.jp wrote:

 Hello, on the way considering alter role set .., I found that
 ALTER ROLE/USER cannot take CURRENT_USER as the role name.

 In addition to that, documents of ALTER ROLE / USER are
 inconsistent with each other in spite of ALTER USER is said to be
 an alias for ALTER ROLE. Plus, ALTER USER cannot take ALL as user
 name although ALTER ROLE can.

 This patch does following things,

  - ALTER USER/ROLE now takes CURRENT_USER as user name.

  - Rewrite sysnopsis of the documents for ALTER USER and ALTER
ROLE so as to they have exactly same syntax.

  - Modify syntax of ALTER USER so as to be an alias of ALTER ROLE.

- Added CURRENT_USER/CURRENT_ROLE syntax to both.
- Added ALL syntax as user name to ALTER USER.
- Added IN DATABASE syntax to ALTER USER.

x Integrating ALTER ROLE/USER syntax could not be done because of
  shift/reduce error of bison.

  x This patch contains no additional regressions. Is it needed?

 SESSION_USER/USER also can be made usable for this command, but
 this patch doesn't so (yet).

 regards,

 --
 Kyotaro Horiguchi
 NTT Open Source Software Center


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




 --
 Rushabh Lathia




-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

[HACKERS] alter user/role CURRENT_USER

2014-10-10 Thread Kyotaro HORIGUCHI

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 horiguchi.kyot...@lab.ntt.co.jp
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
 
  refsynopsisdiv
 synopsis
-ALTER ROLE replaceable class=PARAMETERname/replaceable [ [ WITH ] replaceable class=PARAMETERoption/replaceable [ ... ] ]
+ALTER ROLE { replaceable class=parametername/replaceable | CURRENT_USER } [ [ WITH ] replaceable class=PARAMETERoption/replaceable [ ... ] ]
 
 phrasewhere replaceable class=PARAMETERoption/replaceable can be:/phrase
 
@@ -37,12 +37,12 @@ ALTER ROLE replaceable class=PARAMETERname/replaceable [ [ WITH ] replace
 | [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'replaceable class=PARAMETERpassword/replaceable'
 | VALID UNTIL 'replaceable class=PARAMETERtimestamp/replaceable'
 
-ALTER ROLE replaceable class=PARAMETERname/replaceable RENAME TO replaceablenew_name/replaceable
+ALTER ROLE replaceable class=parametername/replaceable RENAME TO replaceablenew_name/replaceable
 
-ALTER ROLE replaceable class=PARAMETERname/replaceable [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT }
-ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable FROM CURRENT
-ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET replaceableconfiguration_parameter/replaceable
-ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET ALL
+ALTER ROLE { replaceable class=parametername/replaceable | CURRENT_USER | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable { TO | = } { replaceablevalue/replaceable | DEFAULT }
+ALTER ROLE { replaceable class=PARAMETERname/replaceable | CURRENT_USER | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] SET replaceableconfiguration_parameter/replaceable FROM CURRENT
+ALTER ROLE { replaceable class=PARAMETERname/replaceable | CURRENT_USER | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET replaceableconfiguration_parameter/replaceable
+ALTER ROLE { replaceable class=PARAMETERname/replaceable | CURRENT_USER | ALL } [ IN DATABASE replaceable class=PARAMETERdatabase_name/replaceable ] RESET ALL
 /synopsis
  /refsynopsisdiv
 
@@ -123,7 +123,8 @@ ALTER ROLE { replaceable class=PARAMETERname/replaceable | ALL } [ IN DATA
   termreplaceable class=PARAMETERname/replaceable/term
   listitem
para
-The name of the role whose attributes are to be altered.
+The name of the role whose attributes are to be
+altered. literalCURRENT_USER/ matches the name of the current user.
/para
   /listitem
  /varlistentry
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
 
  refsynopsisdiv
 synopsis
-ALTER USER replaceable class=PARAMETERname/replaceable [ [ WITH ] replaceable class=PARAMETERoption/replaceable [ ... ] ]
+ALTER USER {

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

Re: [HACKERS] alter user/role CURRENT_USER

[HACKERS] alter user/role CURRENT_USER

36 matches

Site Navigation

Mail list logo

Footer information