From de30021bf4b1b447fab56a03ffa97dfbc1635f30 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 1 Apr 2020 10:02:29 +0200 Subject: [PATCH] sqlmigrations: prevent schema change noise upon cluster creation The system.comments is now created without write permission to role public. No need to re-do that change on every new cluster. Release note: None Co-authored-by: Lucy Zhang --- pkg/sql/drop_test.go | 37 ++++++++----------- .../testdata/logic_test/information_schema | 2 +- pkg/sql/schema_changer_test.go | 21 +++++------ pkg/sqlmigrations/migrations.go | 5 ++- 4 files changed, 29 insertions(+), 36 deletions(-) diff --git a/pkg/sql/drop_test.go b/pkg/sql/drop_test.go index 4194df0f792c..d3776bad248b 100644 --- a/pkg/sql/drop_test.go +++ b/pkg/sql/drop_test.go @@ -231,11 +231,10 @@ INSERT INTO t.kv VALUES ('c', 'e'), ('a', 'c'), ('b', 'd'); } // Job still running, waiting for GC. - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. sqlRun := sqlutils.MakeSQLRunner(sqlDB) - if err := jobutils.VerifySystemJob(t, sqlRun, 4, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ + if err := jobutils.VerifySystemJob(t, sqlRun, 0, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, Description: "DROP DATABASE t CASCADE", DescriptorIDs: sqlbase.IDs{ @@ -382,10 +381,9 @@ INSERT INTO t.kv2 VALUES ('c', 'd'), ('a', 'b'), ('e', 'a'); tests.CheckKeyCount(t, kvDB, tableSpan, 6) tests.CheckKeyCount(t, kvDB, table2Span, 6) - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - const migrationJobOffset = 4 + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. + const migrationJobOffset = 0 sqlRun := sqlutils.MakeSQLRunner(sqlDB) if err := jobutils.VerifySystemJob(t, sqlRun, migrationJobOffset, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, @@ -559,10 +557,9 @@ func TestDropIndex(t *testing.T) { tests.CheckKeyCount(t, kvDB, indexSpan, numRows) tests.CheckKeyCount(t, kvDB, tableDesc.TableSpan(), 3*numRows) - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - const migrationJobOffset = 4 + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. + const migrationJobOffset = 0 sqlRun := sqlutils.MakeSQLRunner(sqlDB) if err := jobutils.VerifySystemJob(t, sqlRun, migrationJobOffset+1, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, @@ -776,7 +773,7 @@ func TestDropTable(t *testing.T) { // Job still running, waiting for GC. sqlRun := sqlutils.MakeSQLRunner(sqlDB) - if err := jobutils.VerifySystemJob(t, sqlRun, 5, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ + if err := jobutils.VerifySystemJob(t, sqlRun, 1, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, Description: `DROP TABLE t.public.kv`, DescriptorIDs: sqlbase.IDs{ @@ -848,10 +845,9 @@ func TestDropTableDeleteData(t *testing.T) { } } - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - const migrationJobOffset = 4 + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. + const migrationJobOffset = 0 // Data hasn't been GC-ed. sqlRun := sqlutils.MakeSQLRunner(sqlDB) @@ -1133,12 +1129,11 @@ func TestDropDatabaseAfterDropTable(t *testing.T) { } // Job still running, waiting for draining names. - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. sqlRun := sqlutils.MakeSQLRunner(sqlDB) if err := jobutils.VerifySystemJob( - t, sqlRun, 5, jobspb.TypeSchemaChange, jobs.StatusSucceeded, + t, sqlRun, 1, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, Description: "DROP TABLE t.public.kv", diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index 37f945111978..707be8adb169 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -625,7 +625,7 @@ system public web_sessions BASE TABLE system public table_statistics BASE TABLE YES 1 system public locations BASE TABLE YES 1 system public role_members BASE TABLE YES 1 -system public comments BASE TABLE YES 5 +system public comments BASE TABLE YES 1 system public replication_constraint_stats BASE TABLE YES 1 system public replication_critical_localities BASE TABLE YES 1 system public replication_stats BASE TABLE YES 1 diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go index 31a9a1ceda93..2091383a5f3a 100644 --- a/pkg/sql/schema_changer_test.go +++ b/pkg/sql/schema_changer_test.go @@ -1843,10 +1843,9 @@ CREATE TABLE t.test (k INT PRIMARY KEY, v INT8); // State of jobs table t.Skip("TODO(pbardea): The following fails due to causes seemingly unrelated to GC") runner := sqlutils.SQLRunner{DB: sqlDB} - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - const migrationJobOffset = 4 + // TODO (lucy): This test API should use an offset starting from the + // most recent job instead. + const migrationJobOffset = 0 for i, tc := range testCases { status := jobs.StatusSucceeded if tc.errString != "" { @@ -3989,10 +3988,9 @@ CREATE TABLE t.test (k INT PRIMARY KEY, v INT, pi DECIMAL REFERENCES t.pi (d) DE // Ensure that the job is marked as succeeded. sqlRun := sqlutils.MakeSQLRunner(sqlDB) - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - schemaChangeJobOffset := 4 + // TODO (lucy): This test API should use an offset starting from the + // most recent job instead. + schemaChangeJobOffset := 0 if err := jobutils.VerifySystemJob(t, sqlRun, schemaChangeJobOffset+2, jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{ Username: security.RootUser, Description: "TRUNCATE TABLE t.public.test", @@ -5542,10 +5540,9 @@ INSERT INTO t.test (k, v) VALUES (1, 99), (2, 100); sqlRun := sqlutils.MakeSQLRunner(sqlDB) runBeforeConstraintValidation = func() error { - // TODO (lucy): The offset of +4 accounts for unrelated startup migrations. - // Maybe this test API should use an offset starting from the most recent job - // instead. - return jobutils.VerifyRunningSystemJob(t, sqlRun, 4, jobspb.TypeSchemaChange, sql.RunningStatusValidation, jobs.Record{ + // TODO (lucy): Maybe this test API should use an offset starting + // from the most recent job instead. + return jobutils.VerifyRunningSystemJob(t, sqlRun, 0, jobspb.TypeSchemaChange, sql.RunningStatusValidation, jobs.Record{ Username: security.RootUser, Description: "ALTER TABLE t.public.test ADD COLUMN a INT8 AS (v - 1) STORED, ADD CHECK ((a < v) AND (a IS NOT NULL))", DescriptorIDs: sqlbase.IDs{ diff --git a/pkg/sqlmigrations/migrations.go b/pkg/sqlmigrations/migrations.go index 5acaaacc5550..2ae53abec03f 100644 --- a/pkg/sqlmigrations/migrations.go +++ b/pkg/sqlmigrations/migrations.go @@ -285,8 +285,9 @@ var backwardCompatibleMigrations = []migrationDescriptor{ }, { // Introduced in v20.1. - name: "remove public permissions on system.comments", - workFn: depublicizeSystemComments, + name: "remove public permissions on system.comments", + includedInBootstrap: clusterversion.VersionByKey(clusterversion.VersionSchemaChangeJob), + workFn: depublicizeSystemComments, }, { // Introduced in v20.1.