Nice looks good overall.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jackcwu and @rafiss)

docs/RFCS/00000000_grant_option.md, line 13 at r1 (raw file):

is an issue for two reasons:
- There is no way to limit a user's ability to continue granting a privilege without
completely revoking the privilege from the user itself

nit, here and below: finish lines (sentences) with a period.

docs/RFCS/00000000_grant_option.md, line 25 at r1 (raw file):

currently stored. The new release will be accompanied by a long-running migration to give all previous users
with the "GRANT" privilege granting ability on their privileges to promote backwards compatibility. The "GRANT" privilege will effectively
be replaced by this change and eventually deprecated

"and become deprecated"

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 39 at r1 (raw file):

This change will incorporate an optional flag to indicate when to apply the grant option logic on grant ("WITH GRANT OPTION")
and revoke ("GRANT OPTION FOR") statements. Postgres has extensive documentation on [granting](https://www.postgresql.org/docs/9.0/sql-grant.html) 
and [revoking](https://www.postgresql.org/docs/14/sql-revoke.html) on various object types

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 41 at r1 (raw file):

and [revoking](https://www.postgresql.org/docs/14/sql-revoke.html) on various object types

Ex. Grant on Tables

"For example, grant on tables:"

docs/RFCS/00000000_grant_option.md, line 50 at r1 (raw file):

Ex. Revoke on Tables

ditto

docs/RFCS/00000000_grant_option.md, line 63 at r1 (raw file):

### Core Logic
Similar to how privileges are currently tracked for a user, the grant options will be stored as a uint32, with each bit representing
a different privilege. Each user will have its grant option bits tracked in parallel to its privilege bits

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 67 at r1 (raw file):

The grant and revoke functions will be changed to accept a boolean that will be true if the flag is added to the SQL
statement and false otherwise. If true, then the same actions applied to the privilege bits will be applied to the
grant option bits as well (bitwise OR for grant, bitwise AND NOT for revoke)

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 70 at r1 (raw file):

A validation check will be run whenever a grant statement is executed; it will iterate through the privileges in the statement
and check if the granter possesses that privilege in its grant option bits. If it does not, then an error will be 

can you provide an example of which error is returned? Which SQLSTATE is being used?

(I recommend the accompanying test suite to also assert the SQLSTATE is the same that pg reports)

docs/RFCS/00000000_grant_option.md, line 71 at r1 (raw file):

A validation check will be run whenever a grant statement is executed; it will iterate through the privileges in the statement
and check if the granter possesses that privilege in its grant option bits. If it does not, then an error will be 
returned since this is not a valid grant statement

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 78 at r1 (raw file):

privileges holds those privileges in its grant bits will not be run. This is because the node(s) containing the old
version will have all of its users with empty grant option bits. It is only until all nodes are 22.1 or greater
that the check is run, meaning this validation logic will be contained with a version gate

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 84 at r1 (raw file):

| 21.2 & Mixed Cluster  | 22.1                                           | 
| ------------------------------------------------------------------| ----------------------------------------------------------------- | 
| Any user can grant privileges that they possess given that they have the “GRANT” privilege (ignore grant option privileges) | When granting privileges, run a validation check to make sure the granter has the grant option on those privileges               |                                                | 

nit: period at end of sentences.

docs/RFCS/00000000_grant_option.md, line 101 at r1 (raw file):

After migration is performed, we will deprecate GRANT by removing it from the list of privileges in addition to removing
the validation check that it must be present for a grant to succeed

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 105 at r1 (raw file):

The drawbacks primarily revolve around added complexity in regards to backwards compatibility. There are
risks of invalid grants occurring in mixed version clusters if the version gate fails to activate properly

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 105 at r1 (raw file):

The drawbacks primarily revolve around added complexity in regards to backwards compatibility. There are
risks of invalid grants occurring in mixed version clusters if the version gate fails to activate properly

I think if version gates "do not activate properly" the privilege issue will be the least of our concerns 😂

I'm not sure this is really a drawback.

docs/RFCS/00000000_grant_option.md, line 111 at r1 (raw file):

This design is a good choice because it is intuitive and makes use of existing components/abstractions. Not performing this change
means the compatibility gap in privileges between CockroachDB and Postgres would continue

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 120 at r1 (raw file):

# Explain it to folk outside of your team

If I wanted to grant privileges to user A, I could specify the "WITH GRANT OPTION" flag in the grant statement. If I do so,

tip: when creating user archetypes in an explanation, give them human names: Alice, Bob, Cecil, David, Elinor etc

docs/RFCS/00000000_grant_option.md, line 122 at r1 (raw file):

If I wanted to grant privileges to user A, I could specify the "WITH GRANT OPTION" flag in the grant statement. If I do so,
then user A possesses and can grant the privileges it just received from me to other users; if I do not add that flag, then user A 
still obtains those privileges but cannot grant them to other users.

tip: interleave the eample SQL statements with the text to illustrate.

docs/RFCS/00000000_grant_option.md, line 126 at r1 (raw file):

If I wanted to revoke privileges from user A, I could specify the "GRANT OPTION FOR" flag in the revoke statement. If I do so,
then user A loses the ability to grant the privileges in that statement to other users but its own privileges remain unchanged; if
I do not add that flag, then user A loses both those privileges and the ability to grant those privileges

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 131 at r1 (raw file):

those privileges - if it does not, then the command fails and returns an error message.

Ex. Granting

can you explain with more words what the example does?

docs/RFCS/00000000_grant_option.md, line 197 at r1 (raw file):

Altering default privileges will also support the "GRANT OPTION" flags above in the grant/revoke statements

nit: period at end of sentence.

docs/RFCS/00000000_grant_option.md, line 203 at r1 (raw file):

- What to output when a user attempts to grant a privilege it does possess the grant option for? Postgres does not apply
the change but it does not error out either, which a user could mistake for the action being valid if they do not explicity 
check the privileges afterwards. Should CockroachDB return an error or should it mimic the behavior of Postgres?

That's a very good question. The pg behavior seems surprising at first, but then with pg decisions often the decision is well-motivated in the pg source code.

Have you looked at the source code for this inside pg? What does it say about why it is being done this way?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jackcwu, @knz, @rafiss, and @RichardJCai)

docs/RFCS/00000000_grant_option.md, line 12 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 13 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 25 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 39 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 41 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 50 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 63 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 67 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 71 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 75 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 78 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 84 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 100 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 101 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 105 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 105 at r1 (raw file):

fixed - changed to only real drawbacks are complexity that could lead to developer error

docs/RFCS/00000000_grant_option.md, line 105 at r1 (raw file):

fixed - changed to only real drawbacks are complexity that could lead to developer error

docs/RFCS/00000000_grant_option.md, line 111 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 120 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 122 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 126 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 131 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 197 at r1 (raw file):

fixed

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @rafiss, and @RichardJCai)

docs/RFCS/00000000_grant_option.md, line 70 at r1 (raw file):

Added

docs/RFCS/00000000_grant_option.md, line 203 at r1 (raw file):

Thanks for the suggestion, I will do that

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @RichardJCai)

docs/RFCS/00000000_grant_option.md, line 203 at r1 (raw file):

Note that Postgres does in fact show a WARNING (using the "notice" protocol)

postgres=> grant select on t to new_user;
WARNING:  no privileges were granted for "t"
GRANT

this RFC is looking good to merge! there are a few minor details that can be cleaned up though; and maybe useful to mention the has_*_privilege functions that will be updated.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @RichardJCai)

docs/RFCS/00000000_grant_option.md, line 100 at r1 (raw file):

to clarify: "deprecating" just means that it is marked as deprecated in comments, and a notice will be shown to users who try to use it. so it will be deprecated in 22.1, and it will be completely removed in 22.2

docs/RFCS/00000000_grant_option.md, line 203 at r1 (raw file):

here is the code in postgres https://github.com/postgres/postgres/blob/5a2832465fd8984d089e8c44c094e6900d987fcd/src/backend/catalog/aclchk.c#L286-L307

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @RichardJCai)

docs/RFCS/00000000_grant_option.md, line 203 at r1 (raw file):
here's current CRDB behavior

> grant insert on t to newuser;
ERROR: user newuser does not have GRANT privilege on relation t
SQLSTATE: 42501

i think since CRDB is already returning an error in this case, we should have it keep returning an error if the user does not have GRANT OPTION

one final detail to add: how will we show grant options in SHOW GRANTS ?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @RichardJCai)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @RichardJCai)

docs/RFCS/00000000_grant_option.md, line 100 at r1 (raw file):

fixed

docs/RFCS/00000000_grant_option.md, line 203 at r1 (raw file):

Sounds good! I think this was a bit outdated since I took another look at the behavior afterwards.

If you try to grant multiple things in a single query and you have grant options or privileges on only one, the command will go through but only apply the change that you have. That's why I thought it was misleading because it doesn't tell you what was granted and what wasn't (it just says WARNING: not all privileges were granted for

CockroachDB stops the entire statement as soon as it sees an invalid privilege and tells which privilege is missing which I think is more intuitive and also what I based the grant option validation on

lgtm with some minor fixes!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jackcwu, @knz, and @RichardJCai)

-- commits, line 4 at r5:
nit: change to Release note: None

docs/RFCS/00000000_grant_option.md, line 12 at r1 (raw file):

nit: misspelling of "ee"

thanks!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @rafiss, and @RichardJCai)

-- commits, line 4 at r5:

fixed

docs/RFCS/00000000_grant_option.md, line 12 at r1 (raw file):

fixed to "we"