alter user/role CURRENT_USER

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

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

Few comments:

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

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

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

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

On Fri, Oct 10, 2014 at 1:57 PM, Kyotaro HORIGUCHI <
horiguchi.kyotaro@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

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.lathia@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.kyotaro@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.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Thank you for reviewing,

2014 13:10:57 +0530, Rushabh Lathia <rushabh.lathia@gmail.com> wrote in <CAGPqQf0kDFAJiZx0vCA_-wAZwU+Xj5MDNL-HGg1SEz9AW3ck7w@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

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

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

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.kyotaro@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

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.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@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

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

+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.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Marti Raudsepp wrote:

On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@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 Herrera http://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

* Adam Brightwell (adam.brightwell@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

Stephen Frost <sfrost@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

On Tue, Oct 28, 2014 at 2:40 AM, Adam Brightwell
<adam.brightwell@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

* Kevin Grittner (kgrittn@ymail.com) wrote:

Stephen Frost <sfrost@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

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 <sfrost@snowman.net> wrote in <20141028161613.GT28859@tamriel.snowman.net>

* Kevin Grittner (kgrittn@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

Hello,

At Tue, 28 Oct 2014 09:05:20 -0400, Stephen Frost <sfrost@snowman.net> wrote in <20141028130520.GL28859@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

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:

/messages/by-id/15516.1038718413@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.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

* Adam Brightwell (adam.brightwell@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