sql: support {GRANT|REVOKE} PRIVILEGES ... ON {SEQUENCE | ALL SEQUENCES IN SCHEMA}

[ZhouXing19]

Add support for {GRANT|REVOKE} PRIVILEGES ... ON {SEQUENCE | ALL SEQUENCES IN SCHEMA} syntax.

Fixes #74780
Fixes #69584

Release Note(sql): Add support for {GRANT|REVOKE} PRIVILEGES ... ON {SEQUENCE | ALL SEQUENCES IN SCHEMA} syntax.
The privileges allowed for sequences will be those in
Postgres14
(ALL, USAGE, SELECT, UPDATE) in addition to the existing CRBD
table privileges
(ALL, CREATE, DROP, GRANT, SELECT, INSERT, DELETE, UPDATE, ZONECONFIG).
This is because before 22.2 we allow granting table privileges to sequences
via ALTER DEFAULT PRIVILEGES ... ON SEQUENCE.
Note that now sequences are still treated as a subset of tables,
which means {GRANT|REVOKE} ... ON TABLE sequence_name works,
and GRANT ... ON ALL TABLES IN SCHEMA
also grant privileges on all sequences in the schema to the target user.

[cockroach-teamcity]

This change is 

[rafiss]

this looks quite nice! i just had a question

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @shermanCRL and @ZhouXing19)

---

pkg/sql/privilege/privilege.go, line 86 at r3 (raw file):

	SchemaPrivileges   = List{ALL, GRANT, CREATE, USAGE}
	TypePrivileges     = List{ALL, GRANT, USAGE}
	SequencePrivileges = List{ALL, USAGE, SELECT, UPDATE}

should we include all the TablePrivileges in the list for sequences? (since we are treating sequences as a special type of table)

otherwise, i'm a bit worried about the catpb.IsValidPrivilegesForObjectType and privilege.ValidatePrivileges functions

---

pkg/sql/logictest/testdata/logic_test/grant_sequence, line 59 at r3 (raw file):

statement ok
GRANT ALL ON ALL SEQUENCES IN SCHEMA test.public TO readwrite;

could you add another test to see that GRANT USAGE ON SEQUENCE works? I'm a bit worried that since we still treat sequences the same as tables, then something somewhere will end up looking at TablePrivileges, which doesn't include USAGE

if USAGE doesn't work then i think it means we need to update the descriptor validation logic to be aware of sequence privileges.

[ZhouXing19]

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

---

pkg/sql/privilege/privilege.go, line 86 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

should we include all the TablePrivileges in the list for sequences? (since we are treating sequences as a special type of table)

otherwise, i'm a bit worried about the catpb.IsValidPrivilegesForObjectType and privilege.ValidatePrivileges functions

I don't think we need to, because both of these 2 funcs call privilge.GetValidPrivilegesForObject(objectType) to return the allowed priv list here, and we refactored that function too, so that it always return this SequencePrivileges list when objectType = priviledge.Sequence.

---

pkg/sql/logictest/testdata/logic_test/grant_sequence, line 59 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

could you add another test to see that GRANT USAGE ON SEQUENCE works? I'm a bit worried that since we still treat sequences the same as tables, then something somewhere will end up looking at TablePrivileges, which doesn't include USAGE

if USAGE doesn't work then i think it means we need to update the descriptor validation logic to be aware of sequence privileges.

Yes good point, it didn't work because the USAGE privilege was not stored in the descriptor (UserPrivileges.Privileges). I changed how we call catprivilege.MaybeFixPrivileges in pkg/sql/catalog/tabledesc/table_desc_builder.go and it's working now. See added tests above.

[rafiss]

some small nits

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

---

pkg/sql/sequence.go, line 102 at r9 (raw file):

	ctx context.Context, p *planner, descriptor catalog.TableDescriptor,
) (int64, error) {
	var err error

nit: don't think we need the error declared here

---

pkg/sql/sequence.go, line 105 at r9 (raw file):

	requiredPrivileges := []privilege.Kind{privilege.USAGE, privilege.UPDATE}
	HasRequiredPriviledge := false

nit: should be lowercase h

---

pkg/sql/sequence.go, line 115 at r9 (raw file):

	}
	if !HasRequiredPriviledge {
		return 0, errors.AssertionFailedf(

nit: assertion error should only be used for programming errors. they cause reports to be sent to us and automatically create github issues when it happens

so the error should match closer to what the CheckPrivilege function was returning:

	return pgerror.Newf(pgcode.InsufficientPrivilege,
		"user %s does not have %s privilege on %s %s",
		user, privilege, descriptor.DescriptorType(), descriptor.GetName())

---

pkg/sql/catalog/tabledesc/table_desc_builder.go, line 246 at r9 (raw file):

	}

	fixedPrivileges := catprivilege.MaybeFixPrivileges(

i think the call to MaybeFixPrivileges will cause us to clear out any privileges that are now invalid for sequences (e.g. DELETE). i'm pretty sure that's fine, but just wanted to confirm. do you think we should call runPostDeserializationChangesOnAllDescriptors during the upgrade migration to v22.2? perhaps the schema team has thoughts

also, right now the comment on MaybeFixPrivileges says it can be removed after 21.2, but it seems like we should change that to say 22.2 now.

[ZhouXing19]

From the CI, it seems that the current changes are breaking default privilege tests. Looking into it.

[ZhouXing19]

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

---

pkg/sql/sequence.go line 102 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: don't think we need the error declared here

Done.

---

pkg/sql/sequence.go line 105 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: should be lowercase h

Done.

---

pkg/sql/sequence.go line 115 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: assertion error should only be used for programming errors. they cause reports to be sent to us and automatically create github issues when it happens

so the error should match closer to what the CheckPrivilege function was returning:

	return pgerror.Newf(pgcode.InsufficientPrivilege,
		"user %s does not have %s privilege on %s %s",
		user, privilege, descriptor.DescriptorType(), descriptor.GetName())

Done, thanks!

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 246 at r9 (raw file):

clear out any privileges that are now invalid for sequences (e.g. DELETE)

Yes that's the expected behavior.

do you think we should call runPostDeserializationChangesOnAllDescriptors during the upgrade migration to v22.2?

Ah in this case I agree that we may want to include all the TablePrivileges in the list for sequences, so that we can avoid making existing privileges unavailable during upgrade migration.
But I'm not sure we need to call runPostDeserializationChangesOnAllDescriptors since we're just expanding the set of allowed privileges for sequences.

also, right now the comment on MaybeFixPrivileges says it can be removed after 21.2, but it seems like we should change that to say 22.2 now.

Made the changes.

[rafiss]

Ah in this case I agree that we may want to include all the TablePrivileges in the list for sequences, so that we can avoid making existing privileges unavailable during upgrade migration.

Could you say more? I thought you were saying that the privileges that were being removed are not used for sequences anywhere?

[ZhouXing19]

I thought you were saying that the privileges that were being removed are not used for sequences anywhere?

They (e.g. CREATE) can be used for sequences via ALTER DEFAULT PRIVILEGES, e.g.

cockroach/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_with_grant_option

Lines 554 to 560 in c676689

	ALTER DEFAULT PRIVILEGES GRANT CREATE ON SEQUENCES TO testuser2 WITH GRANT OPTION
	statement ok
	ALTER DEFAULT PRIVILEGES REVOKE GRANT OPTION FOR ALL PRIVILEGES ON SEQUENCES FROM testuser
	statement ok
	CREATE SEQUENCE seq1

[ZhouXing19]

Added notice if the granted privilege on sequence is not in PG14's list (i.e. (ALL, USAGE, UPDATE, SELECT).

[ZhouXing19]

@rafiss re-pinging for review. Thanks!

[rafiss]

all the code lgtm! i just have one comment about the notice.

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

---

pkg/sql/grant_revoke.go line 256 at r13 (raw file):

					errors.WithHint(
						pgnotice.Newf("%s", sequencePrivilegesNotInPg.SortedNames()),
						"the above privileges may be incompatible with PostgreSQL",

i think we can use a bit of a softer message, so user's don't worry too much. also, i think we should only show a notice for the privileges that are essentially no-ops in CRDB. for example, even though DROP is not allowed in PG for sequences, it still is useful in CRDB. so i think the only privileges we need to show the notice for are: CREATE, INSERT, DELETE, and ZONECONFIG (but perhaps you could confirm that these all have no effect for sequences)

also, nit: no need to use WithHint on this one.

so, to summarize the above, i think we can use something like:

params.p.BufferClientNotice(
  ctx,
  pgnotice.Newf("some privileges have no effect on sequences: %s", sequencePrivilegesNotInPg.SortedNames(),
)

List{ALL, USAGE, SELECT, UPDATE, CREATE, DROP, GRANT, INSERT, DELETE, ZONECONFIG}

[ZhouXing19]

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

---

pkg/sql/grant_revoke.go line 256 at r13 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think we can use a bit of a softer message, so user's don't worry too much. also, i think we should only show a notice for the privileges that are essentially no-ops in CRDB. for example, even though DROP is not allowed in PG for sequences, it still is useful in CRDB. so i think the only privileges we need to show the notice for are: CREATE, INSERT, DELETE, and ZONECONFIG (but perhaps you could confirm that these all have no effect for sequences)

also, nit: no need to use WithHint on this one.

so, to summarize the above, i think we can use something like:

params.p.BufferClientNotice(
  ctx,
  pgnotice.Newf("some privileges have no effect on sequences: %s", sequencePrivilegesNotInPg.SortedNames(),
)

List{ALL, USAGE, SELECT, UPDATE, CREATE, DROP, GRANT, INSERT, DELETE, ZONECONFIG}

Thanks! This makes lots of sense.
I renamed the list to sequencePrivilegesNoOp, and note that this excludes DROP and GRANT, which are not in PG's privileges for sequences but correspond to actual operations in CRDB. Test cases for DROP and GRANT on sequences are also added in pkg/sql/logictest/testdata/logic_test/grant_sequence.

[rafiss]

this looks great! feel free to merge after fixing the comment

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

---

pkg/sql/privilege/privilege.go line 89 at r16 (raw file):

	// before v22.2 we treated Sequences the same as Tables. This is to avoid making
	// certain privileges unavailable after upgrade migration.
	// Note that "UPDATE, CREATE, GRANT, "

nit: unfinished comment

[ZhouXing19]

pkg/sql/grant_revoke.go line 256 at r13 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Thanks! This makes lots of sense.
I renamed the list to sequencePrivilegesNoOp, and note that this excludes DROP and GRANT, which are not in PG's privileges for sequences but correspond to actual operations in CRDB. Test cases for DROP and GRANT on sequences are also added in pkg/sql/logictest/testdata/logic_test/grant_sequence.

And yes I've confirmed thatCREATE, INSERT, DELETE, ZONECONFIG on sequences are no-ops.

[ZhouXing19]

TFTR!
The failing test in bazelCI is irrelevant.
bors r=rafiss

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)