sql/parser: Make ROLE a non-reserved keyword

[bdarnell]

WORK became a reserved keyword in #20274, but it doesn't look like this is necessary. We should move it to the unreserved list to minimize disruption to applications that may have tables or columns with this name.

VIRTUAL and ROLE were also made reserved in 2.0, and we should see if we can move them too.

[jordanlewis]

VIRTUAL and WORK are fixed now. ROLE is harder, because of the new SHOW GRANTS syntax that we added. I think we should consider removing or restricting some of that syntax, considering the pain it will inevitably cause (and has already started to cause - see cockroachdb/sqlalchemy-cockroachdb#50).

The problem is caused by the rules:

SHOW GRANTS ON ROLE opt_name_list for_grantee_clause

SHOW GRANTS on_privilege_target_clause for_grantee_clause

These are problematic rules because they're ambiguous. on_privilege_target_clause is either empty or ON targets. targets can be a list of table names. Table names could previously include the word role.

Therefore, it's impossible for the grammar to distinguish the syntax:

SHOW GRANTS ON ROLE

between parsing as the first rule and parsing as the second. That's why ROLE was added to the reserved keywords list - that resolves the problem, since the grammar now knows that ROLE can't be used as an unquoted table/target name.

This all being said, we need to figure out a way to move ROLE back to the unreserved keyword list. Unfortunately this is all but certain to require a backward-incompatible grammar change between 2.0 and the release we fix this in.

Some options are:

• Change the syntax for SHOW GRANTS ON <table list> to be SHOW GRANTS FOR <table list>.
• Force the table names in SHOW GRANTS ON <table list> to be quoted no matter what

[bdarnell]

Grammatically, I think I'd keep ON for SHOW GRANTS ON <tables> and use FOR for SHOW GRANTS FOR <role>. This also keeps the incompatible change to the new-in-2.0-enterprise syntax while preserving compatibility with the older SHOW GRANTS ON <tables>.

FWIW, we also reserve STORED, VIEW, LATERAL, NOTHING, and INDEX, while these are not reserved in postgres.

[jordanlewis]

LATERAL and STORED are easily fixable; the others aren't. I'll send another PR for that.

@mberhault do you have any thoughts on this issue?

[knz]

👍 on using FOR, for the same reasons as Ben's.

Regarding the other keywords, I'd propose trying to un-reserve them if possible. I recently came to the understanding that the move to reserve new keywords by default was cargo-culting previous grammar changes, not done principally, and I suspect several of the new keywords do not need to be reserved.

[knz]

May I also suggest using this discussion as renewed motive to lock down grammar changes behind an additional level of scrutiny. Last time I suggested to do so, my proposal was shot down (for some good reasons, but one bad: I was also saying we'd run into this class of problems, and nobody believed me. Now there's proof) but I'd be willing to champion such an initiative again.

[mberhault]

The problem with SHOW GRANTS is that in both cases (privileges and roles) the target (table or database or role) and user are both optional, so we need to way to distinguish between showing privileges and showing role memberships.

Now, we have a couple of options:

• the postgres way: just don't have SHOW GRANTS. that kind of sucks, requiring special cli commands just to read this stuff is annoying.
• the mysql way: SHOW GRANTS shows everything, with the FOR and ON clauses used to filter targets. Depending on how this is implemented, ROLE may or may not be used as a keyword.

In this specific case, ROLE wasn't moved to reserved willy-nilly, this was the only way to make this statement work. The fact that postgres does not do this is, in my opinion, a lack on their part.

[knz]

In this specific case, ROLE wasn't moved to reserved willy-nilly, this was the only way to make this statement work.

Dissenting opinion: there was no good reason to use ON in ON ROLE.

[mberhault]

Then the syntax is even weirder than usual SHOW GRANT ROLE <some role> FOR <some user>?

At some point, we either move the target logic into code (to determine if it's a db or table or role), we don't do role memberships, we keep role reserved, or we make show grants trivial (eg: no filtering of target, just the user. After checking the exact mysql grammar, turns out this is exactly what it does).

[bdarnell]

Or we could do something like SHOW ROLE GRANTS..., or use virtual tables instead of custom non-standard syntax. The grammar of SHOW statements should be kept simple and non-intrusive.

These contortions may be awkward, but they're worthwhile. Adding new reserved words breaks applications, so we should be very cautious about doing this. role is a fairly common name for columns, which is why this is coming up now while no one complained when we unnecessarily reserved STORED and LATERAL.

[knz]

The real question IMHO is what sense does it make to call "show grants" the statement that tests "which user has which role" -- there's no idea of grant in there.

(Incidentally, I think it was a mistake to treat roles as permissions and "grant" them as permissions. They are really groups and we have role membership and group-level permissions. I'm not advocating changing the other statements accordingly, but it would be good to keep this reality in mind when evaluating syntax proposals.)

To me the following make sense:

• SHOW USERS FROM <some role> (currently: SHOW GRANTS ON <some role>)
• SHOW ROLES FOR <some user> (currently: SHOW GRANTS FOR <some user>)

[mberhault]

The reason for this is that 1) roles are granted just like privileges (GRANT <role> TO <user>) and 2) mysql's introspection for role memberships is SHOW GRANTS. It was merely an attempt to limit surprises and to inject some semblance of logic in sql (ah!).

That said, SHOW ROLE MEMBERS (or similar) would work. I'm hesitant about SHOW USERES FROM <role>) as both roles and users are members of roles.

[knz]

SHOW ROLE GRANTS and SHOW ROLE MEMBERS would work for me. I agree that SHOW USERS FROM is perhaps a bit too far-fetched.

[knz]

FWIW @benesch just explained to me an alternate idea which would un-break the code for the 2.0 release without require a change in docs. I'll try to expand his idea.

[knz]

Folk the problem is really much worse than we thought: not only does this issue prevent new apps from working that want to use the reserved keywords as tables.

It also breaks existing databases -- any existing views that refer to tables named after new reserved keywords will now generate errors.

May I also suggest using this discussion as renewed motive to lock down grammar changes behind an additional level of scrutiny.

... including verification that the addition of any new reserved keyword is accompanied by a db migration that rewrites all views.

[petermattis]

@knz Is locking down the grammar necessary? From my reading of this discussion, it is the addition of new reserved keywords that is the problem.

PS Are you going to share the alternate idea that @benesch explained to you? I'm waiting eagerly to hear what it is.

[knz]

The idea (if I understand correctly) is to disambiguate in the action part ({...}). Both nikhil and I have separately started to work on a PoC. Stay tuned. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[petermattis]

I'm curious to see how that will work. SHOW GRANTS ON ROLE foo vs SHOW GRANTS ON role foo appear to be identical grammatically. I suppose you could make role special so that you can only do SHOW GRANTS ON "role". I wonder if you could achieve this is via something similar to the WITH_LA approach. That is, change the grammar to look for SHOW GRANTS ON_ROLE_LA where ON_ROLE_LA is a special token that looks for ON followed by an unquoted ROLE identifier.

[knz]

Update: both Peter and I have a PR out to unreserve the keyword without changing the syntax (except for the very special case of SHOW GRANTS ON with a table named "role", where the table name now needs to be quoted).

We might pick one of these approaches to go to 2.0.1 (to unbreak apps immediately), but then I'd like to also immediately start the process to migrate/evolve the syntax to something different, for example using the FOR keyword instead, so as to remove the code and complexity that either of our PRs is adding and reduce tech debt.