ApplySchema: deprecate '--allow_long_unavailability' flag

go/cmd/vtctldclient/command/schema.go

@@ -138,7 +138,7 @@ func commandApplySchema(cmd *cobra.Command, args []string) error {
 
 	resp, err := client.ApplySchema(commandCtx, &vtctldatapb.ApplySchemaRequest{
 		Keyspace:                ks,
-		AllowLongUnavailability: applySchemaOptions.AllowLongUnavailability,
+		AllowLongUnavailability: false,
 		DdlStrategy:             applySchemaOptions.DDLStrategy,
 		Sql:                     parts,
 		SkipPreflight:           applySchemaOptions.SkipPreflight,

go/vt/schemamanager/tablet_executor.go

@@ -26,7 +26,6 @@ import (
 	"vitess.io/vitess/go/timer"
 	"vitess.io/vitess/go/vt/logutil"
 	querypb "vitess.io/vitess/go/vt/proto/query"
-	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
 	"vitess.io/vitess/go/vt/schema"
 	"vitess.io/vitess/go/vt/sqlparser"
@@ -38,45 +37,31 @@ import (
 
 // TabletExecutor applies schema changes to all tablets.
 type TabletExecutor struct {
-	migrationContext     string
-	ts                   *topo.Server
-	tmc                  tmclient.TabletManagerClient
-	logger               logutil.Logger
-	tablets              []*topodatapb.Tablet
-	isClosed             bool
-	allowBigSchemaChange bool
-	keyspace             string
-	waitReplicasTimeout  time.Duration
-	ddlStrategySetting   *schema.DDLStrategySetting
-	uuids                []string
-	skipPreflight        bool
+	migrationContext    string
+	ts                  *topo.Server
+	tmc                 tmclient.TabletManagerClient
+	logger              logutil.Logger
+	tablets             []*topodatapb.Tablet
+	isClosed            bool
+	keyspace            string
+	waitReplicasTimeout time.Duration
+	ddlStrategySetting  *schema.DDLStrategySetting
+	uuids               []string
+	skipPreflight       bool
 }
 
 // NewTabletExecutor creates a new TabletExecutor instance
 func NewTabletExecutor(migrationContext string, ts *topo.Server, tmc tmclient.TabletManagerClient, logger logutil.Logger, waitReplicasTimeout time.Duration) *TabletExecutor {
 	return &TabletExecutor{
-		ts:                   ts,
-		tmc:                  tmc,
-		logger:               logger,
-		isClosed:             true,
-		allowBigSchemaChange: false,
-		waitReplicasTimeout:  waitReplicasTimeout,
-		migrationContext:     migrationContext,
+		ts:                  ts,
+		tmc:                 tmc,
+		logger:              logger,
+		isClosed:            true,
+		waitReplicasTimeout: waitReplicasTimeout,
+		migrationContext:    migrationContext,
 	}
 }
 
-// AllowBigSchemaChange changes TabletExecutor such that big schema changes
-// will no longer be rejected.
-func (exec *TabletExecutor) AllowBigSchemaChange() {
-	exec.allowBigSchemaChange = true
-}
-
-// DisallowBigSchemaChange enables the check for big schema changes such that
-// TabletExecutor will reject these.
-func (exec *TabletExecutor) DisallowBigSchemaChange() {
-	exec.allowBigSchemaChange = false
-}
-
 // SetDDLStrategy applies ddl_strategy from command line flags
 func (exec *TabletExecutor) SetDDLStrategy(ddlStrategy string) error {
 	ddlStrategySetting, err := schema.ParseDDLStrategy(ddlStrategy)
@@ -151,48 +136,31 @@ func (exec *TabletExecutor) Validate(ctx context.Context, sqls []string) error {
 	if exec.isClosed {
 		return fmt.Errorf("executor is closed")
 	}
-
-	// We ignore DATABASE-level DDLs here because detectBigSchemaChanges doesn't
-	// look at them anyway.
-	parsedDDLs, _, _, _, err := exec.parseDDLs(sqls)
-	if err != nil {
+	if err := exec.parseDDLs(sqls); err != nil {
 		return err
 	}
 
-	bigSchemaChange, err := exec.detectBigSchemaChanges(ctx, parsedDDLs)
-	if bigSchemaChange && exec.allowBigSchemaChange {
-		exec.logger.Warningf("Processing big schema change. This may cause visible MySQL downtime.")
-		return nil
-	}
-	return err
+	return nil
 }
 
-func (exec *TabletExecutor) parseDDLs(sqls []string) ([]sqlparser.DDLStatement, []sqlparser.DBDDLStatement, [](*sqlparser.RevertMigration), [](*sqlparser.AlterMigration), error) {
-	parsedDDLs := make([]sqlparser.DDLStatement, 0)
-	parsedDBDDLs := make([]sqlparser.DBDDLStatement, 0)
-	revertStatements := make([](*sqlparser.RevertMigration), 0)
-	alterMigrationStatements := make([](*sqlparser.AlterMigration), 0)
+func (exec *TabletExecutor) parseDDLs(sqls []string) error {
 	for _, sql := range sqls {
 		stmt, err := sqlparser.Parse(sql)
 		if err != nil {
-			return nil, nil, nil, nil, fmt.Errorf("failed to parse sql: %s, got error: %v", sql, err)
+			return fmt.Errorf("failed to parse sql: %s, got error: %v", sql, err)
 		}
-		switch stmt := stmt.(type) {
+		switch stmt.(type) {
 		case sqlparser.DDLStatement:
-			parsedDDLs = append(parsedDDLs, stmt)
 		case sqlparser.DBDDLStatement:
-			parsedDBDDLs = append(parsedDBDDLs, stmt)
 		case *sqlparser.RevertMigration:
-			revertStatements = append(revertStatements, stmt)
 		case *sqlparser.AlterMigration:
-			alterMigrationStatements = append(alterMigrationStatements, stmt)
 		default:
 			if len(exec.tablets) != 1 {
-				return nil, nil, nil, nil, fmt.Errorf("non-ddl statements can only be executed for single shard keyspaces: %s", sql)
+				return fmt.Errorf("non-ddl statements can only be executed for single shard keyspaces: %s", sql)
 			}
 		}
 	}
-	return parsedDDLs, parsedDBDDLs, revertStatements, alterMigrationStatements, nil
+	return nil
 }
 
 // IsOnlineSchemaDDL returns true if we expect to run a online schema change DDL
@@ -215,59 +183,6 @@ func (exec *TabletExecutor) isOnlineSchemaDDL(stmt sqlparser.Statement) (isOnlin
 	return false
 }
 
-// a schema change that satisfies any following condition is considered
-// to be a big schema change and will be rejected.
-//   1. Alter more than 100,000 rows.
-//   2. Change a table with more than 2,000,000 rows (Drops are fine).
-func (exec *TabletExecutor) detectBigSchemaChanges(ctx context.Context, parsedDDLs []sqlparser.DDLStatement) (bool, error) {
-	// We want to avoid any overhead if possible. If all DDLs are online schema changes, then we want to
-	// skip GetSchema altogether.
-	foundAnyNonOnlineDDL := false
-	for _, ddl := range parsedDDLs {
-		if !exec.isOnlineSchemaDDL(ddl) {
-			foundAnyNonOnlineDDL = true
-		}
-	}
-	if !foundAnyNonOnlineDDL {
-		return false, nil
-	}
-	// exec.tablets is guaranteed to have at least one element;
-	// Otherwise, Open should fail and executor should fail.
-	primaryTabletInfo := exec.tablets[0]
-	// get database schema, excluding views.
-	req := &tabletmanagerdatapb.GetSchemaRequest{Tables: []string{}, ExcludeTables: []string{}, TableSchemaOnly: true}
-	dbSchema, err := exec.tmc.GetSchema(ctx, primaryTabletInfo, req)
-	if err != nil {
-		return false, fmt.Errorf("unable to get database schema, error: %v", err)
-	}
-	tableWithCount := make(map[string]uint64, len(dbSchema.TableDefinitions))
-	for _, tableSchema := range dbSchema.TableDefinitions {
-		tableWithCount[tableSchema.Name] = tableSchema.RowCount
-	}
-	for _, ddl := range parsedDDLs {
-		if exec.isOnlineSchemaDDL(ddl) {
-			// Since this is an online schema change, there is no need to worry about big changes
-			continue
-		}
-		switch ddl.GetAction() {
-		case sqlparser.DropDDLAction, sqlparser.CreateDDLAction, sqlparser.TruncateDDLAction, sqlparser.RenameDDLAction:
-			continue
-		}
-		tableName := ddl.GetTable().Name.String()
-		if rowCount, ok := tableWithCount[tableName]; ok {
-			if rowCount > 100000 && ddl.GetAction() == sqlparser.AlterDDLAction {
-				return true, fmt.Errorf(
-					"big schema change detected. Disable check with -allow_long_unavailability. ddl: %s alters a table with more than 100 thousand rows", sqlparser.String(ddl))
-			}
-			if rowCount > 2000000 {
-				return true, fmt.Errorf(
-					"big schema change detected. Disable check with -allow_long_unavailability. ddl: %s changes a table with more than 2 million rows", sqlparser.String(ddl))
-			}
-		}
-	}
-	return false, nil
-}
-
 func (exec *TabletExecutor) preflightSchemaChanges(ctx context.Context, sqls []string) error {
 	if exec.skipPreflight {
 		return nil

go/vt/schemamanager/tablet_executor_test.go

@@ -134,8 +134,8 @@ func TestTabletExecutorValidate(t *testing.T) {
 	// alter a table with more than 100,000 rows
 	if err := executor.Validate(ctx, []string{
 		"ALTER TABLE test_table_03 ADD COLUMN new_id bigint(20)",
-	}); err == nil {
-		t.Fatalf("executor.Validate should fail, alter a table more than 100,000 rows")
+	}); err != nil {
+		t.Fatalf("executor.Validate should not fail, even for a table with more than 100,000 rows")
 	}
 
 	if err := executor.Validate(ctx, []string{
@@ -149,21 +149,6 @@ func TestTabletExecutorValidate(t *testing.T) {
 	}); err != nil {
 		t.Fatalf("executor.Validate should succeed, drop a table with more than 2,000,000 rows is allowed")
 	}
-
-	executor.AllowBigSchemaChange()
-	// alter a table with more than 100,000 rows
-	if err := executor.Validate(ctx, []string{
-		"ALTER TABLE test_table_03 ADD COLUMN new_id bigint(20)",
-	}); err != nil {
-		t.Fatalf("executor.Validate should succeed, big schema change is disabled")
-	}
-
-	executor.DisallowBigSchemaChange()
-	if err := executor.Validate(ctx, []string{
-		"ALTER TABLE test_table_03 ADD COLUMN new_id bigint(20)",
-	}); err == nil {
-		t.Fatalf("executor.Validate should fail, alter a table more than 100,000 rows")
-	}
 }
 
 func TestTabletExecutorDML(t *testing.T) {

go/vt/vtctl/grpcvtctldserver/server.go

@@ -212,9 +212,6 @@ func (s *VtctldServer) ApplySchema(ctx context.Context, req *vtctldatapb.ApplySc
 	})
 
 	executor := schemamanager.NewTabletExecutor(migrationContext, s.ts, s.tmc, logger, waitReplicasTimeout)
-	if req.AllowLongUnavailability {
-		executor.AllowBigSchemaChange()
-	}
 	if req.SkipPreflight {
 		executor.SkipPreflight()
 	}

go/vt/vtctl/vtctl.go

@@ -611,8 +611,8 @@ var commands = []commandGroup{
 			{
 				name:   "ApplySchema",
 				method: commandApplySchema,
-				params: "[--allow_long_unavailability] [--wait_replicas_timeout=10s] [--ddl_strategy=<ddl_strategy>] [--uuid_list=<comma_separated_uuids>] [--migration_context=<unique-request-context>] [--skip_preflight] {--sql=<sql> || --sql-file=<filename>} <keyspace>",
-				help:   "Applies the schema change to the specified keyspace on every primary, running in parallel on all shards. The changes are then propagated to replicas via replication. If --allow_long_unavailability is set, schema changes affecting a large number of rows (and possibly incurring a longer period of unavailability) will not be rejected. -ddl_strategy is used to instruct migrations via vreplication, gh-ost or pt-osc with optional parameters. -migration_context allows the user to specify a custom request context for online DDL migrations. If -skip_preflight, SQL goes directly to shards without going through sanity checks.",
+				params: "[--wait_replicas_timeout=10s] [--ddl_strategy=<ddl_strategy>] [--uuid_list=<comma_separated_uuids>] [--migration_context=<unique-request-context>] [--skip_preflight] {--sql=<sql> || --sql-file=<filename>} <keyspace>",
+				help:   "Applies the schema change to the specified keyspace on every primary, running in parallel on all shards. The changes are then propagated to replicas via replication. -ddl_strategy is used to instruct migrations via vreplication, gh-ost or pt-osc with optional parameters. -migration_context allows the user to specify a custom request context for online DDL migrations. If -skip_preflight, SQL goes directly to shards without going through sanity checks.",
 			},
 			{
 				name:   "CopySchemaShard",
@@ -3053,7 +3053,7 @@ func commandValidateSchemaKeyspace(ctx context.Context, wr *wrangler.Wrangler, s
 }
 
 func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *flag.FlagSet, args []string) error {
-	allowLongUnavailability := subFlags.Bool("allow_long_unavailability", false, "Allow large schema changes which incur a longer unavailability of the database.")
+	allowLongUnavailability := subFlags.Bool("allow_long_unavailability", false, "Deprecated. We recommend using Online DDL for schema changes, big or small.")
 	sql := subFlags.String("sql", "", "A list of semicolon-delimited SQL commands")
 	sqlFile := subFlags.String("sql-file", "", "Identifies the file that contains the SQL commands")
 	ddlStrategy := subFlags.String("ddl_strategy", string(schema.DDLStrategyDirect), "Online DDL strategy, compatible with @@ddl_strategy session variable (examples: 'gh-ost', 'pt-osc', 'gh-ost --max-load=Threads_running=100'")
@@ -3070,6 +3070,9 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *fl
 	if subFlags.NArg() != 1 {
 		return fmt.Errorf("the <keyspace> argument is required for the commandApplySchema command")
 	}
+	if *allowLongUnavailability {
+		log.Warningf("--allow_long_unavailability flag is deprecated. We recomment using Online DDL for schema changes, big or small.")
+	}
 
 	// v14 deprecates `--skip-topo` flag. This check will be removed in v15
 	if settings, _ := schema.ParseDDLStrategy(*ddlStrategy); settings != nil && settings.IsSkipTopoFlag() {
@@ -3104,7 +3107,7 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *fl
 
 	resp, err := wr.VtctldServer().ApplySchema(ctx, &vtctldatapb.ApplySchemaRequest{
 		Keyspace:                keyspace,
-		AllowLongUnavailability: *allowLongUnavailability,
+		AllowLongUnavailability: false,
 		DdlStrategy:             *ddlStrategy,
 		Sql:                     parts,
 		SkipPreflight:           *skipPreflight,