Merge branch 'main' into mtocker-burnfree-instant-ddl

pkg/migration/runner.go

@@ -209,6 +209,15 @@ func (r *Runner) Run(originalCtx context.Context) error {
 		return err
 	}
 
+	// Force enable the checksum if it's an ADD UNIQUE INDEX operation
+	// https://github.com/cashapp/spirit/issues/266
+	if !r.migration.Checksum {
+		if err := utils.AlterContainsAddUnique("ALTER TABLE unused " + r.migration.Alter); err != nil {
+			r.logger.Warnf("force enabling checksum: %v", err)
+			r.migration.Checksum = true
+		}
+	}
+
 	// Run post-setup checks
 	if err := r.runChecks(ctx, check.ScopePostSetup); err != nil {
 		return err
@@ -769,7 +778,7 @@ func (r *Runner) checksum(ctx context.Context) error {
 			// then the checksum will fail. This is entirely expected, and not considered a bug. We should
 			// do our best-case to differentiate that we believe this ALTER statement is lossy, and
 			// customize the returned error based on it.
-			if err := utils.AlterIsAddUnique("ALTER TABLE unused " + r.migration.Alter); err != nil {
+			if err := utils.AlterContainsAddUnique("ALTER TABLE unused " + r.migration.Alter); err != nil {
 				return fmt.Errorf("checksum failed after 3 attempts. Check that the ALTER statement is not adding a UNIQUE INDEX to non-unique data")
 			}
 			return fmt.Errorf("checksum failed after 3 attempts. This likely indicates either a bug in Spirit, or a manual modification to the _new table outside of Spirit. Please report @ github.com/cashapp/spirit")

pkg/migration/runner_test.go

@@ -575,7 +575,8 @@ func TestChangeDatatypeLossyFailEarly(t *testing.T) {
 
 // TestAddUniqueIndex is a really interesting test *because* resuming from checkpoint
 // will cause duplicate key errors. It's not straight-forward to differentiate between
-// duplicate errors from a resume, and a constraint violation. So what we do is
+// duplicate errors from a resume, and a constraint violation. So what we do is:
+// 0) *FORCE* checksum to be enabled if ALTER contains add unique index.
 // 1) *FORCE* checksum to be enabled on resume from checkpoint
 // 2) If checksum is not enabled, duplicate key errors are elevated to errors.
 func TestAddUniqueIndexChecksumEnabled(t *testing.T) {
@@ -602,11 +603,14 @@ func TestAddUniqueIndexChecksumEnabled(t *testing.T) {
 		Threads:  16,
 		Table:    "uniqmytable",
 		Alter:    "ADD UNIQUE INDEX b (b)",
+		Checksum: false,
 	})
 	assert.NoError(t, err)
+	assert.False(t, m.migration.Checksum)
 	err = m.Run(context.Background())
-	assert.Error(t, err)         // not unique
-	assert.NoError(t, m.Close()) // need to close now otherwise we'll get an error on re-opening it.
+	assert.Error(t, err)                 // not unique
+	assert.NoError(t, m.Close())         // need to close now otherwise we'll get an error on re-opening it.
+	assert.True(t, m.migration.Checksum) // it force enables a checksum
 
 	testutils.RunSQL(t, "DELETE FROM uniqmytable WHERE b = REPEAT('a', 200) LIMIT 1") // make unique
 	m, err = NewRunner(&Migration{

pkg/utils/utils.go

@@ -136,9 +136,9 @@ func AlterContainsUnsupportedClause(sql string) error {
 	return nil
 }
 
-// AlterIsAddUnique checks to see if any clauses of an ALTER contains add UNIQUE index.
+// AlterContainsAddUnique checks to see if any clauses of an ALTER contains add UNIQUE index.
 // We use this to customize the error returned from checksum fails.
-func AlterIsAddUnique(sql string) error {
+func AlterContainsAddUnique(sql string) error {
 	p := parser.New()
 	stmtNodes, _, err := p.Parse(sql, "", "")
 	if err != nil {

pkg/utils/utils_test.go

@@ -70,16 +70,16 @@ func TestAlterIsAddUnique(t *testing.T) {
 	var alter = func(stmt string) string {
 		return "ALTER TABLE `test`.`t1` " + stmt
 	}
-	assert.NoError(t, AlterIsAddUnique(alter("drop index `a`")))
-	assert.NoError(t, AlterIsAddUnique(alter("rename index `a` to `b`")))
-	assert.NoError(t, AlterIsAddUnique(alter("drop index `a`, drop index `b`")))
-	assert.NoError(t, AlterIsAddUnique(alter("drop index `a`, rename index `b` to c")))
+	assert.NoError(t, AlterContainsAddUnique(alter("drop index `a`")))
+	assert.NoError(t, AlterContainsAddUnique(alter("rename index `a` to `b`")))
+	assert.NoError(t, AlterContainsAddUnique(alter("drop index `a`, drop index `b`")))
+	assert.NoError(t, AlterContainsAddUnique(alter("drop index `a`, rename index `b` to c")))
 
-	assert.NoError(t, AlterIsAddUnique(alter("ADD COLUMN `a` INT")))
-	assert.NoError(t, AlterIsAddUnique(alter("ADD index (a)")))
-	assert.NoError(t, AlterIsAddUnique(alter("drop index `a`, add index `b` (`b`)")))
-	assert.NoError(t, AlterIsAddUnique(alter("engine=innodb")))
-	assert.Error(t, AlterIsAddUnique(alter("add unique(b)"))) // this is potentially lossy.
+	assert.NoError(t, AlterContainsAddUnique(alter("ADD COLUMN `a` INT")))
+	assert.NoError(t, AlterContainsAddUnique(alter("ADD index (a)")))
+	assert.NoError(t, AlterContainsAddUnique(alter("drop index `a`, add index `b` (`b`)")))
+	assert.NoError(t, AlterContainsAddUnique(alter("engine=innodb")))
+	assert.Error(t, AlterContainsAddUnique(alter("add unique(b)"))) // this is potentially lossy.
 }
 
 func TestTrimAlter(t *testing.T) {