alter user/role CURRENT_USER

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

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

This patch does following things,

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

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

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

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

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

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

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

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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(at)postgresql(dot)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(dot)lathia(at)gmail(dot)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(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>>
>
>
> --
> Rushabh Lathia
>

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

Thank you for reviewing,

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

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

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(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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

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

> horiguchi(dot)kyotaro(at)(dot)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.

>
> +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(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

Marti Raudsepp wrote:
> On Fri, Oct 24, 2014 at 11:29 AM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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

* Adam Brightwell (adam(dot)brightwell(at)crunchydatasolutions(dot)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(at)snowman(dot)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

On Tue, Oct 28, 2014 at 2:40 AM, Adam Brightwell
<adam(dot)brightwell(at)crunchydatasolutions(dot)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

* Kevin Grittner (kgrittn(at)ymail(dot)com) wrote:
> Stephen Frost <sfrost(at)snowman(dot)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(at)snowman(dot)net> wrote in <20141028161613(dot)GT28859(at)tamriel(dot)snowman(dot)net>
> * Kevin Grittner (kgrittn(at)ymail(dot)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(at)snowman(dot)net> wrote in <20141028130520(dot)GL28859(at)tamriel(dot)snowman(dot)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

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.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(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

* Adam Brightwell (adam(dot)brightwell(at)crunchydatasolutions(dot)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

Stephen Frost <sfrost(at)snowman(dot)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

Adam Brightwell <adam(dot)brightwell(at)crunchydatasolutions(dot)com> writes:
> FWIW, I found the following in the archives:

> http://www.postgresql.org/message-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?

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

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+KRtzfowVuHdJC5mGjfg@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.1408832605@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(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

Hello,

> Adam Brightwell <adam(dot)brightwell(at)crunchydatasolutions(dot)com> writes:
> > FWIW, I found the following in the archives:
>
> > http://www.postgresql.org/message-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?
>
> 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.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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(dot)brightwell(at)crunchydatasolutions(dot)com> writes:
> > > FWIW, I found the following in the archives:
> >
> > > http://www.postgresql.org/message-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?
> >
> > 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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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 => ?

On Fri, Nov 14, 2014 at 5:39 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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

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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera wrote:
> Kyotaro HORIGUCHI wrote:

> Thanks for doing the fiddly work here. Attached is a new version of
> this patch. I simplified some things, including removing those rules
> you added to RoleId. It seems to me that this problem:
>
> > RoleId in the patch still has rule components for CURRENT_USER,
> > SESSION_USER, and CURRENT_ROLE. Without them, the parser prints
> > an error ununderstandable to users.
> >
> > | =# alter role current_user rename to "PuBlic";
> > | ERROR: syntax error at or near "rename"
> > | LINE 1: alter role current_user rename to "PuBlic";
> > | ^
>
> can be fixed without complicating the rest of the stuff simply by using
> RoleSpec instead of RoleId and doing the error checks at the RenameStmt
> production.

I tried that but it's way too messy, so I readded them.

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

I later noticed that you had already submitted a test.sql file, so I
adopted it as rolenames.sql and added it to the schedule files. I still
have to read through the results and make sure they make sense, so the
expected file is not in this patch.

I made some more changes to the code; unless the tests uncover something
ugly, the code in this patch is what will be committed.

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

That's done in the attached.

Documentation is still missing. Are you submitting doc changes soon? I
would like to get this committed.

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

Actually this is better -- I added token location tracking, and changed
RoleId to use RoleSpec which means it can throw errors with locations
when "public" or "none" are specified. I think the checks for
public/none in CreateRole and AlterRole are dead code now.

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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 <alvherre(at)2ndquadrant(dot)com> wrote in <20150309185032(dot)GQ3291(at)alvh(dot)no-ip(dot)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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello,

At Thu, 30 Apr 2015 17:12:25 -0300, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote in <20150430201225(dot)GV4369(at)alvh(dot)no-ip(dot)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