diff --git a/pkg/bench/rttanalysis/testdata/benchmark_expectations b/pkg/bench/rttanalysis/testdata/benchmark_expectations index c2e2cb88afa4..c2a06561fe88 100644 --- a/pkg/bench/rttanalysis/testdata/benchmark_expectations +++ b/pkg/bench/rttanalysis/testdata/benchmark_expectations @@ -56,9 +56,9 @@ exp,benchmark 5,GenerateObjects/generate_10_tables 16,GenerateObjects/generate_10x10_schemas_and_tables_in_existing_db 5,GenerateObjects/generate_50000_tables -15,Grant/grant_all_on_1_table -19,Grant/grant_all_on_2_tables -23,Grant/grant_all_on_3_tables +10,Grant/grant_all_on_1_table +10,Grant/grant_all_on_2_tables +10,Grant/grant_all_on_3_tables 19,GrantRole/grant_1_role 25,GrantRole/grant_2_roles 6,Jobs/cancel_job @@ -104,9 +104,9 @@ exp,benchmark 4,ORMQueries/pg_type 133,ORMQueries/prisma_column_descriptions 3,ORMQueries/prisma_column_descriptions_updated -15,Revoke/revoke_all_on_1_table -19,Revoke/revoke_all_on_2_tables -23,Revoke/revoke_all_on_3_tables +10,Revoke/revoke_all_on_1_table +10,Revoke/revoke_all_on_2_tables +10,Revoke/revoke_all_on_3_tables 17,RevokeRole/revoke_1_role 21,RevokeRole/revoke_2_roles 14,ShowGrants/grant_2_roles diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go index d6b73842234e..27e0f65d6f78 100644 --- a/pkg/sql/grant_revoke.go +++ b/pkg/sql/grant_revoke.go @@ -393,15 +393,6 @@ func (n *changeDescriptorBackedPrivilegesNode) startExec(params runParams) error } case *tabledesc.Mutable: - // TODO (lucy): This should probably have a single consolidated job like - // DROP DATABASE. - if err := p.createOrUpdateSchemaChangeJob( - ctx, d, - fmt.Sprintf("updating privileges for table %d", d.ID), - descpb.InvalidMutationID, - ); err != nil { - return err - } if !d.Dropped() { if err := p.writeSchemaChangeToBatch(ctx, d, b); err != nil { return err @@ -419,7 +410,7 @@ func (n *changeDescriptorBackedPrivilegesNode) startExec(params runParams) error }) } case *typedesc.Mutable: - err := p.writeTypeSchemaChange(ctx, d, fmt.Sprintf("updating privileges for type %d", d.ID)) + err := p.writeDescToBatch(ctx, d, b) if err != nil { return err } diff --git a/pkg/sql/logictest/testdata/logic_test/grant_type b/pkg/sql/logictest/testdata/logic_test/grant_type index 0d04ec25253d..f7117c402b73 100644 --- a/pkg/sql/logictest/testdata/logic_test/grant_type +++ b/pkg/sql/logictest/testdata/logic_test/grant_type @@ -126,3 +126,26 @@ test public owner_grant_option other_owner ALL test public owner_grant_option owner_grant_option_child USAGE false test public owner_grant_option public USAGE false test public owner_grant_option root ALL true + +statement ok +CREATE USER roach; +CREATE TYPE custom_type1 AS ENUM ('roach1', 'roach2', 'roach3'); +CREATE TYPE custom_type2 AS ENUM ('roachA', 'roachB', 'roachC'); +CREATE TYPE custom_type3 AS ENUM ('roachI', 'roachII', 'roachIII'); +BEGIN; +GRANT ALL ON TYPE custom_type1 TO roach; +GRANT ALL ON TYPE custom_type2 TO roach; +GRANT ALL ON TYPE custom_type3 TO roach; +COMMIT + +query T +select description from [show jobs] where description LIKE '%type%' +---- + +query TTTTTTB colnames,rowsort +SHOW GRANTS FOR roach +---- +database_name schema_name object_name object_type grantee privilege_type is_grantable +test public custom_type1 type roach ALL false +test public custom_type2 type roach ALL false +test public custom_type3 type roach ALL false diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go index c4e115250b9c..fe6307d95133 100644 --- a/pkg/sql/schema_changer_test.go +++ b/pkg/sql/schema_changer_test.go @@ -7821,3 +7821,58 @@ func TestMemoryMonitorErrorsDuringBackfillAreRetried(t *testing.T) { require.Equalf(t, shouldFail.Load(), int64(2), "not all failure conditions were hit %d", shouldFail.Load()) }) } + +// TestGrantAndSelectsWithinTransaction tests two concurrent +// transactions on tables, where one transaction is a grant. +// We verify that grants on tables no longer creates a job(s). +// We also verify that the second transaction does wait for +// the first transaction to commit. +func TestGrantAndSelectsWithinTransaction(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + ctx := context.Background() + + s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(ctx) + + sqlRunner := sqlutils.MakeSQLRunner(sqlDB) + + sqlRunner.Exec(t, `CREATE USER ROACHMIN;`) + sqlRunner.Exec(t, `GRANT ADMIN TO ROACHMIN;`) + sqlRunner.Exec(t, `CREATE TABLE PROMO_CODES (my_int INT);`) + sqlRunner.Exec(t, `CREATE TABLE RIDES (my_int INT);`) + + txn1 := sqlRunner.Begin(t) + txn1.Exec(`SELECT * FROM PROMO_CODES;`) + + resBefore := sqlRunner.QueryStr(t, `select count(*) from [show jobs];`) + + txn2 := sqlRunner.Begin(t) + txn2.Exec(`GRANT ALL ON TABLE PROMO_CODES TO ROACHMIN;`) + txn2.Exec(`GRANT ALL ON TABLE RIDES TO ROACHMIN;`) + + var txn2Committed syncutil.AtomicBool + group := ctxgroup.WithContext(ctx) + + group.GoCtx(func(ctx context.Context) error { + err := txn2.Commit() + txn2Committed.Set(true) + return err + }) + + group.GoCtx(func(ctx context.Context) error { + if txn2Committed.Get() { + return errors.New("transaction 2 committed before transaction 1") + } + err := txn1.Commit() + return err + }) + + err := group.Wait() + require.NoError(t, err) + + resAfter := sqlRunner.QueryStr(t, `select count(*) from [show jobs];`) + // No new jobs should have been created between resBefore to resAfter. + require.True(t, resBefore[0][0] == resAfter[0][0]) + +} diff --git a/pkg/sql/stats/automatic_stats_test.go b/pkg/sql/stats/automatic_stats_test.go index 4ee58a26f4b3..b47de2e0be5e 100644 --- a/pkg/sql/stats/automatic_stats_test.go +++ b/pkg/sql/stats/automatic_stats_test.go @@ -309,6 +309,85 @@ func BenchmarkEnsureAllTables(b *testing.B) { } } +func BenchmarkGrantTables(b *testing.B) { + defer leaktest.AfterTest(b)() + defer log.Scope(b).Close(b) + ctx := context.Background() + + for _, numTables := range []int{10, 100, 1000} { + b.Run(fmt.Sprintf("numTables=%d", numTables), func(b *testing.B) { + srv, sqlDB, _ := serverutils.StartServer(b, base.TestServerArgs{}) + defer srv.Stopper().Stop(ctx) + + sqlRun := sqlutils.MakeSQLRunner(sqlDB) + sqlRun.Exec(b, `CREATE DATABASE t;`) + sqlRun.Exec(b, `USE t;`) + + sqlRun.Exec(b, `CREATE USER ROACH;`) + + for i := 0; i < numTables; i++ { + sqlRun.Exec(b, fmt.Sprintf(`CREATE TABLE t.a%d (k INT PRIMARY KEY);`, i)) + } + + b.ResetTimer() + for i := 0; i < b.N; i++ { + sqlRun.Exec(b, `GRANT ALL ON * TO ROACH;`) + sqlRun.Exec(b, `REVOKE ALL ON * FROM ROACH;`) + } + }) + } +} + +func BenchmarkGrantTypes(b *testing.B) { + defer leaktest.AfterTest(b)() + defer log.Scope(b).Close(b) + ctx := context.Background() + + for _, numTypes := range []int{10, 100, 1000} { + b.Run(fmt.Sprintf("numTypes=%d", numTypes), func(b *testing.B) { + srv, sqlDB, _ := serverutils.StartServer(b, base.TestServerArgs{}) + defer srv.Stopper().Stop(ctx) + + sqlRun := sqlutils.MakeSQLRunner(sqlDB) + sqlRun.Exec(b, `CREATE DATABASE t;`) + sqlRun.Exec(b, `USE t;`) + + sqlRun.Exec(b, `CREATE USER ROACH;`) + + for i := 0; i < numTypes; i++ { + sqlRun.Exec(b, fmt.Sprintf(`CREATE TYPE a%d AS ENUM ('roach1', 'roach2', 'roach3');`, i)) + } + + b.ResetTimer() + for i := 0; i < b.N; i++ { + txn := sqlRun.Begin(b) + for i := 0; i < numTypes; i++ { + _, err := txn.Exec(fmt.Sprintf(`GRANT ALL ON TYPE a%d TO ROACH;`, i)) + if err != nil { + return + } + } + err := txn.Commit() + if err != nil { + return + } + + txn = sqlRun.Begin(b) + for i := 0; i < numTypes; i++ { + _, err = txn.Exec(fmt.Sprintf(`REVOKE ALL ON TYPE a%d FROM ROACH;`, i)) + if err != nil { + return + } + } + err = txn.Commit() + if err != nil { + return + } + } + }) + } +} + func checkAllTablesCount(ctx context.Context, systemTables bool, expected int, r *Refresher) error { const collectionDelay = time.Microsecond oldAutoStatsClusterMode := AutomaticStatisticsClusterMode.Get(&r.st.SV) diff --git a/pkg/sql/type_change.go b/pkg/sql/type_change.go index a6a19907cee9..621810818ffe 100644 --- a/pkg/sql/type_change.go +++ b/pkg/sql/type_change.go @@ -165,6 +165,14 @@ func (p *planner) writeTypeDesc(ctx context.Context, typeDesc *typedesc.Mutable) return p.txn.Run(ctx, b) } +func (p *planner) writeDescToBatch( + ctx context.Context, typeDesc catalog.MutableDescriptor, b *kv.Batch, +) error { + return p.Descriptors().WriteDescToBatch( + ctx, p.extendedEvalCtx.Tracing.KVTracingEnabled(), typeDesc, b, + ) +} + // typeSchemaChanger is the struct that actually runs the type schema change. type typeSchemaChanger struct { typeID descpb.ID