OnlineDDL: -skip-topo is always 'true'

go/test/endtoend/onlineddl/ghost/onlineddl_ghost_test.go

@@ -311,13 +311,13 @@ func TestSchemaChange(t *testing.T) {
 		onlineddl.CheckRetryMigration(t, &vtParams, shards, uuid, true)
 	})
 	t.Run("Online CREATE no PK table, vtgate", func(t *testing.T) {
-		uuid := testOnlineDDLStatement(t, noPKCreateTableStatement, "gh-ost --skip-topo", "vtgate", "online_ddl_create_col")
+		uuid := testOnlineDDLStatement(t, noPKCreateTableStatement, "gh-ost", "vtgate", "online_ddl_create_col")
 		onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
 		onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, false)
 		onlineddl.CheckRetryMigration(t, &vtParams, shards, uuid, false)
 	})
 	t.Run("Fail ALTER for no PK table, vtgate", func(t *testing.T) {
-		uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost --skip-topo", "vtgate", "")
+		uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost", "vtgate", "")
 		onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusFailed)
 
 		rs := onlineddl.ReadMigrations(t, &vtParams, uuid)

go/test/endtoend/onlineddl/vrepl_stress/onlineddl_vrepl_mini_stress_test.go

@@ -88,7 +88,7 @@ var (
 
 	opOrder               int64
 	opOrderMutex          sync.Mutex
-	onlineDDLStrategy     = "online -vreplication-test-suite -skip-topo"
+	onlineDDLStrategy     = "online -vreplication-test-suite"
 	hostname              = "localhost"
 	keyspaceName          = "ks"
 	shards                []cluster.Shard

go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go

@@ -46,7 +46,7 @@ var (
 	clusterInstance         *cluster.LocalProcessCluster
 	vtParams                mysql.ConnParams
 	evaluatedMysqlParams    *mysql.ConnParams
-	ddlStrategy             = "online -skip-topo -vreplication-test-suite"
+	ddlStrategy             = "online -vreplication-test-suite"
 	waitForMigrationTimeout = 20 * time.Second
 
 	hostname              = "localhost"

go/vt/schema/ddl_strategy.go

@@ -155,13 +155,10 @@ func (setting *DDLStrategySetting) RuntimeOptions() []string {
 
 // IsSkipTopo suggests that DDL should apply to tables bypassing global topo request
 func (setting *DDLStrategySetting) IsSkipTopo() bool {
-	switch {
-	case setting.IsSingleton(), setting.IsSingletonContext():
-		return true
-	case setting.hasFlag(skipTopoFlag):
-		return true
-	}
-	return false
+	// Vitess 11 introduced the flag -skip-topo. starting Vitess 12 the flag is _always_ considered 'true'.
+	// Ideally the flag should be gone, but for backwards compatibility we allow users to still specify it
+	// (and we stil ignore the value, it's always set to true)
+	return true
 }
 
 // ToString returns a simple string representation of this instance

go/vt/schema/online_ddl.go

@@ -372,6 +372,32 @@ func (onlineDDL *OnlineDDL) ToJSON() ([]byte, error) {
 	return json.Marshal(onlineDDL)
 }
 
+// sqlWithoutComments returns the SQL statement without comment directives. Useful for tests
+func (onlineDDL *OnlineDDL) sqlWithoutComments() (sql string, err error) {
+	sql = onlineDDL.SQL
+	stmt, err := sqlparser.Parse(sql)
+	if err != nil {
+		// query validation and rebuilding
+		if _, err := legacyParseRevertUUID(sql); err == nil {
+			// This is a revert statement of the form "revert <uuid>". We allow this for now. Future work will
+			// make sure the statement is a valid, parseable "revert vitess_migration '<uuid>'", but we must
+			// be backwards compatible for now.
+			return sql, nil
+		}
+		// otherwise the statement should have been parseable!
+		return "", err
+	}
+
+	switch stmt := stmt.(type) {
+	case sqlparser.DDLStatement:
+		stmt.SetComments(nil)
+	case *sqlparser.RevertMigration:
+		stmt.SetComments(nil)
+	}
+	sql = sqlparser.String(stmt)
+	return sql, nil
+}
+
 // GetAction extracts the DDL action type from the online DDL statement
 func (onlineDDL *OnlineDDL) GetAction() (action sqlparser.DDLAction, err error) {
 	if _, err := onlineDDL.GetRevertUUID(); err == nil {

go/vt/schema/online_ddl_test.go

@@ -210,8 +210,8 @@ func TestNewOnlineDDL(t *testing.T) {
 		NewDDLStrategySetting(DDLStrategyOnline, ""),
 		NewDDLStrategySetting(DDLStrategyOnline, "-singleton"),
 	}
-	require.False(t, strategies[0].IsSkipTopo())
-	require.False(t, strategies[1].IsSkipTopo())
+	require.True(t, strategies[0].IsSkipTopo())
+	require.True(t, strategies[1].IsSkipTopo())
 	require.True(t, strategies[2].IsSkipTopo())
 
 	for _, ts := range tt {
@@ -224,16 +224,11 @@ func TestNewOnlineDDL(t *testing.T) {
 						return
 					}
 					assert.NoError(t, err)
-					if stgy.IsSkipTopo() {
-						// onlineDDL.SQL enriched with /*vt+ ... */ comment
-						assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(onlineDDL.UUID)))
-						assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(migrationContext)))
-						assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(string(stgy.Strategy))))
-					} else {
-						assert.NotContains(t, onlineDDL.SQL, hex.EncodeToString([]byte(onlineDDL.UUID)))
-						assert.NotContains(t, onlineDDL.SQL, hex.EncodeToString([]byte(migrationContext)))
-						assert.NotContains(t, onlineDDL.SQL, hex.EncodeToString([]byte(string(stgy.Strategy))))
-					}
+					require.True(t, stgy.IsSkipTopo(), "IsSkipTopo() should always be true")
+					// onlineDDL.SQL enriched with /*vt+ ... */ comment
+					assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(onlineDDL.UUID)))
+					assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(migrationContext)))
+					assert.Contains(t, onlineDDL.SQL, hex.EncodeToString([]byte(string(stgy.Strategy))))
 				})
 			}
 		})
@@ -297,7 +292,8 @@ func TestNewOnlineDDLs(t *testing.T) {
 
 			sqls := []string{}
 			for _, onlineDDL := range onlineDDLs {
-				sql := onlineDDL.SQL
+				sql, err := onlineDDL.sqlWithoutComments()
+				assert.NoError(t, err)
 				sql = strings.ReplaceAll(sql, "\n", "")
 				sql = strings.ReplaceAll(sql, "\t", "")
 				sqls = append(sqls, sql)