Merge cockroachdb#108056

108056: sql: delete TestSchemaChangeReverseMutations r=chengxiong-ruan a=chengxiong-ruan

Informs: cockroachdb#51796

The logic this test wanted to test are mostly covered by logic tests and other schema changer tests which verify job status. Also the the failed schema change purging behavior is covered by TestSchemaChangePurgeFailure which is unskipped in cockroachdb#108053.

Release note: None
Release justification: test only change

Co-authored-by: Chengxiong Ruan <[email protected]>

Author: craig[bot]

pkg/sql/schema_changer_test.go

@@ -1810,267 +1810,6 @@ CREATE TABLE t.test (k INT PRIMARY KEY, v INT);
 	}
 }
 
-// TestSchemaChangeReverseMutations tests that schema changes get reversed
-// correctly when one of them violates a constraint.
-func TestSchemaChangeReverseMutations(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-	// TODO (lucy): This test needs more complicated schema changer knobs than
-	// currently implemented. What this test should be doing is starting a schema
-	// change, and, before it hits a violation that requires a rollback, starting
-	// another schema change that depends on the successful completion of the
-	// first. At the end, all the dependent schema change jobs should have been
-	// rolled back. Right now we're just testing that schema changes fail
-	// correctly when run sequentially, which is not as interesting. The previous
-	// comments are kept around to describe the intended results of the test, but
-	// they don't reflect what's happening now.
-	params, _ := createTestServerParams()
-	const chunkSize = 200
-	// Disable synchronous schema change processing so that the mutations get
-	// processed asynchronously.
-	var enableAsyncSchemaChanges uint32
-	params.Knobs = base.TestingKnobs{
-		SQLSchemaChanger: &sql.SchemaChangerTestingKnobs{
-			BackfillChunkSize: chunkSize,
-		},
-	}
-	s, sqlDB, kvDB := serverutils.StartServer(t, params)
-	defer s.Stopper().Stop(context.Background())
-	codec := s.ApplicationLayer().Codec()
-
-	// Disable strict GC TTL enforcement because we're going to shove a zero-value
-	// TTL into the system with AddImmediateGCZoneConfig.
-	defer sqltestutils.DisableGCTTLStrictEnforcement(t, sqlDB)()
-
-	// Create a k-v table.
-	if _, err := sqlDB.Exec(`
-CREATE DATABASE t;
-CREATE TABLE t.test (k INT PRIMARY KEY, v INT8);
-`); err != nil {
-		t.Fatal(err)
-	}
-
-	// Add some data
-	const maxValue = chunkSize + 1
-	if err := sqltestutils.BulkInsertIntoTable(sqlDB, maxValue); err != nil {
-		t.Fatal(err)
-	}
-
-	tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "test")
-
-	testCases := []struct {
-		sql       string
-		errString string
-	}{
-		// Create a column that is not NULL. This schema change doesn't return an
-		// error only because we've turned off the synchronous execution path; it
-		// will eventually fail when run by the asynchronous path.
-		{`ALTER TABLE t.public.test ADD COLUMN a INT8 UNIQUE DEFAULT 0, ADD COLUMN c INT8`,
-			"violates unique constraint"},
-		// Add an index over a column that will be purged. This index will
-		// eventually not get added. The column aa will also be dropped as
-		// a result.
-		{`ALTER TABLE t.public.test ADD COLUMN aa INT8, ADD CONSTRAINT foo UNIQUE (a)`,
-			"column \"a\" does not exist"},
-
-		// The purge of column 'a' doesn't influence these schema changes.
-
-		// Drop column 'v' moves along just fine.
-		{`ALTER TABLE t.public.test DROP COLUMN v`,
-			""},
-		// Add unique column 'b' moves along creating column b and the index on
-		// it.
-		{`ALTER TABLE t.public.test ADD COLUMN b INT8 UNIQUE`,
-			""},
-		// #27033: Add a column followed by an index on the column.
-		{`ALTER TABLE t.public.test ADD COLUMN d STRING NOT NULL DEFAULT 'something'`,
-			""},
-
-		{`CREATE INDEX ON t.public.test (d)`,
-			""},
-
-		// Add an index over a column 'c' that will be purged. This index will
-		// eventually not get added. The column bb will also be dropped as
-		// a result.
-		{`ALTER TABLE t.public.test ADD COLUMN bb INT8, ADD CONSTRAINT bar UNIQUE (never_existed)`,
-			"column \"never_existed\" does not exist"},
-		// Cascading of purges. column 'c' -> column 'bb' -> constraint 'idx_bb'.
-		{`ALTER TABLE t.public.test ADD CONSTRAINT idx_bb UNIQUE (bb)`,
-			"column \"bb\" does not exist"},
-	}
-
-	for _, tc := range testCases {
-		_, err := sqlDB.Exec(tc.sql)
-		if tc.errString == "" {
-			if err != nil {
-				t.Fatalf("%s: %v", tc.sql, err)
-			}
-		} else {
-			if err == nil {
-				t.Fatalf("%s: expected error", tc.sql)
-			}
-			if !strings.Contains(err.Error(), tc.errString) {
-				t.Fatalf("%s: %v", tc.sql, err)
-			}
-		}
-	}
-
-	// Enable async schema change processing for purged schema changes.
-	atomic.StoreUint32(&enableAsyncSchemaChanges, 1)
-
-	expectedCols := []string{"k", "b", "d"}
-	// Wait until all the mutations have been processed.
-	testutils.SucceedsSoon(t, func() error {
-		tableDesc = desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "test")
-		if len(tableDesc.AllMutations()) > 0 {
-			return errors.Errorf("%d mutations remaining", len(tableDesc.AllMutations()))
-		}
-		return nil
-	})
-
-	// Verify that t.public.test has the expected data. Read the table data while
-	// ensuring that the correct table lease is in use.
-	rows, err := sqlDB.Query(`SELECT * from t.test`)
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer rows.Close()
-	cols, err := rows.Columns()
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// Ensure that sql is using the correct table lease.
-	if len(cols) != len(expectedCols) {
-		t.Fatalf("incorrect columns: %v, expected: %v", cols, expectedCols)
-	}
-	if cols[0] != expectedCols[0] || cols[1] != expectedCols[1] {
-		t.Fatalf("incorrect columns: %v", cols)
-	}
-
-	// rows contains the data; verify that it's the right data.
-	vals := make([]interface{}, len(expectedCols))
-	for i := range vals {
-		vals[i] = new(interface{})
-	}
-	var count int64
-	for ; rows.Next(); count++ {
-		if err := rows.Scan(vals...); err != nil {
-			t.Errorf("row %d scan failed: %s", count, err)
-			continue
-		}
-		for j, v := range vals {
-			switch j {
-			case 0:
-				if val := *v.(*interface{}); val != nil {
-					switch k := val.(type) {
-					case int64:
-						if count != k {
-							t.Errorf("k = %d, expected %d", k, count)
-						}
-
-					default:
-						t.Errorf("error input of type %T", k)
-					}
-				} else {
-					t.Error("received NULL value for column 'k'")
-				}
-
-			case 1:
-				if val := *v.(*interface{}); val != nil {
-					t.Error("received non NULL value for column 'b'")
-				}
-
-			case 2:
-				if val := *v.(*interface{}); val == nil {
-					t.Error("received NULL value for column 'd'")
-				}
-			}
-		}
-	}
-	if err := rows.Err(); err != nil {
-		t.Fatal(err)
-	}
-	if eCount := int64(maxValue + 1); eCount != count {
-		t.Fatalf("read the wrong number of rows: e = %d, v = %d", eCount, count)
-	}
-
-	// Check that the index on b eventually goes live even though a schema
-	// change in front of it in the queue got purged.
-	rows, err = sqlDB.Query(`SELECT * from t.test@test_b_key`)
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer rows.Close()
-	count = 0
-	for ; rows.Next(); count++ {
-	}
-	if err := rows.Err(); err != nil {
-		t.Fatal(err)
-	}
-	if eCount := int64(maxValue + 1); eCount != count {
-		t.Fatalf("read the wrong number of rows: e = %d, v = %d", eCount, count)
-	}
-
-	// Check that the index on c gets purged.
-	if _, err := sqlDB.Query(`SELECT * from t.test@foo`); err == nil {
-		t.Fatal("SELECT over index 'foo' works")
-	}
-
-	if err := sqlutils.RunScrub(sqlDB, "t", "test"); err != nil {
-		t.Fatal(err)
-	}
-
-	// Add immediate GC TTL to allow index creation purge to complete.
-	if _, err := sqltestutils.AddImmediateGCZoneConfig(sqlDB, tableDesc.GetID()); err != nil {
-		t.Fatal(err)
-	}
-
-	ctx := context.Background()
-
-	testutils.SucceedsSoon(t, func() error {
-		// Check that the number of k-v pairs is accurate.
-		return sqltestutils.CheckTableKeyCount(ctx, kvDB, codec, 3, maxValue)
-	})
-
-	// State of jobs table
-	skip.WithIssue(t, 51796, "TODO(pbardea): The following fails due to causes seemingly unrelated to GC")
-	runner := sqlutils.SQLRunner{DB: sqlDB}
-	// 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 != "" {
-			status = jobs.StatusFailed
-		}
-		if err := jobutils.VerifySystemJob(t, &runner, migrationJobOffset+i, jobspb.TypeSchemaChange, status, jobs.Record{
-			Username:    username.RootUserName(),
-			Description: tc.sql,
-			DescriptorIDs: descpb.IDs{
-				tableDesc.GetID(),
-			},
-		}); err != nil {
-			t.Fatal(err)
-		}
-	}
-
-	jobRolledBack := 0
-	jobID := jobutils.GetJobID(t, &runner, jobRolledBack)
-
-	// Roll back job.
-	if err := jobutils.VerifySystemJob(t, &runner, len(testCases), jobspb.TypeSchemaChange, jobs.StatusSucceeded, jobs.Record{
-		Username:    username.RootUserName(),
-		Description: fmt.Sprintf("ROLL BACK JOB %d: %s", jobID, testCases[jobRolledBack].sql),
-		DescriptorIDs: descpb.IDs{
-			tableDesc.GetID(),
-		},
-	}); err != nil {
-		t.Fatal(err)
-	}
-
-}
-
 // This test checks backward compatibility with old data that contains
 // sentinel kv pairs at the start of each table row. Cockroachdb used
 // to write table rows with sentinel values in the past. When a new column