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 <[email protected]>
cockroachdb · Apr 1, 2020 · de30021 · de30021
1 parent 52653e6
commit de30021
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 36 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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.