ApplySchema: deprecate '--allow_long_unavailability' flag (#10717)

* ApplySchema: deprecate '--allow_long_unavailability' flag Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * simplify parseDDLs(), use in Validate Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * warn if --allow_long_unavailability is provided Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * remove --allow_long_unavailability from usage message Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * ALTER should not FAIL in test Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * formally deprecate flag; fix typo; fix usage Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * fix merge Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * removed allow_long_unavailability from proto Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * removed allow_long_unavailability from proto Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * remove AllowLongUnavailability field Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * remove test testing deprecated code Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * remove deprecation message Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * use vterrors.Errorf Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * mark flag as deprecated Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
vitessio · Jul 13, 2023 · a7d8b52 · a7d8b52
1 parent 535f013
commit a7d8b52
Show file tree

Hide file tree

Showing 12 changed files with 1,155 additions and 1,470 deletions.
diff --git a/go/cmd/vtctldclient/command/schema.go b/go/cmd/vtctldclient/command/schema.go
@@ -40,7 +40,7 @@ import (
 var (
 	// ApplySchema makes an ApplySchema gRPC call to a vtctld.
 	ApplySchema = &cobra.Command{
-		Use:   "ApplySchema [--allow-long-unavailability] [--ddl-strategy <strategy>] [--uuid <uuid> ...] [--migration-context <context>] [--wait-replicas-timeout <duration>] [--caller-id <caller_id>] {--sql-file <file> | --sql <sql>} <keyspace>",
+		Use:   "ApplySchema [--ddl-strategy <strategy>] [--uuid <uuid> ...] [--migration-context <context>] [--wait-replicas-timeout <duration>] [--caller-id <caller_id>] {--sql-file <file> | --sql <sql>} <keyspace>",
 		Short: "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.",
 		Long: `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.
 
@@ -137,15 +137,14 @@ func commandApplySchema(cmd *cobra.Command, args []string) error {
 	ks := cmd.Flags().Arg(0)
 
 	resp, err := client.ApplySchema(commandCtx, &vtctldatapb.ApplySchemaRequest{
-		Keyspace:                ks,
-		AllowLongUnavailability: applySchemaOptions.AllowLongUnavailability,
-		DdlStrategy:             applySchemaOptions.DDLStrategy,
-		Sql:                     parts,
-		SkipPreflight:           true,
-		UuidList:                applySchemaOptions.UUIDList,
-		MigrationContext:        applySchemaOptions.MigrationContext,
-		WaitReplicasTimeout:     protoutil.DurationToProto(applySchemaOptions.WaitReplicasTimeout),
-		CallerId:                cid,
+		Keyspace:            ks,
+		DdlStrategy:         applySchemaOptions.DDLStrategy,
+		Sql:                 parts,
+		SkipPreflight:       true,
+		UuidList:            applySchemaOptions.UUIDList,
+		MigrationContext:    applySchemaOptions.MigrationContext,
+		WaitReplicasTimeout: protoutil.DurationToProto(applySchemaOptions.WaitReplicasTimeout),
+		CallerId:            cid,
 	})
 	if err != nil {
 		return err
@@ -286,8 +285,8 @@ func commandReloadSchemaShard(cmd *cobra.Command, args []string) error {
 }
 
 func init() {
+	ApplySchema.Flags().MarkDeprecated("--allow-long-unavailability", "")
 	ApplySchema.Flags().MarkDeprecated("--skip-preflight", "Deprecated. Assumed to be always 'true'")
-	ApplySchema.Flags().BoolVar(&applySchemaOptions.AllowLongUnavailability, "allow-long-unavailability", false, "Allow large schema changes which incur a longer unavailability of the database.")
 	ApplySchema.Flags().StringVar(&applySchemaOptions.DDLStrategy, "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'.")
 	ApplySchema.Flags().StringSliceVar(&applySchemaOptions.UUIDList, "uuid", nil, "Optional, comma-delimited, repeatable, explicit UUIDs for migration. If given, must match number of DDL changes.")
 	ApplySchema.Flags().StringVar(&applySchemaOptions.MigrationContext, "migration-context", "", "For Online DDL, optionally supply a custom unique string used as context for the migration(s) in this command. By default a unique context is auto-generated by Vitess.")

diff --git a/go/vt/proto/vtctldata/vtctldata.pb.go b/go/vt/proto/vtctldata/vtctldata.pb.go
diff --git a/go/vt/proto/vtctldata/vtctldata_vtproto.pb.go b/go/vt/proto/vtctldata/vtctldata_vtproto.pb.go
diff --git a/go/vt/schemamanager/schemamanager_test.go b/go/vt/schemamanager/schemamanager_test.go
@@ -101,18 +101,6 @@ func TestSchemaManagerExecutorOpenFail(t *testing.T) {
 	}
 }
 
-func TestSchemaManagerExecutorExecuteFail(t *testing.T) {
-	controller := newFakeController(
-		[]string{"create table test_table (pk int);"}, false, false, false)
-	executor := NewTabletExecutor("TestSchemaManagerExecutorExecuteFail", newFakeTopo(t), newFakeTabletManagerClient(), logutil.NewConsoleLogger(), testWaitReplicasTimeout)
-	ctx := context.Background()
-
-	_, err := Run(ctx, controller, executor)
-	if err == nil || !strings.Contains(err.Error(), "unknown database: vt_test_keyspace") {
-		t.Fatalf("run schema change should fail due to executor.Execute fail, but got: %v", err)
-	}
-}
-
 func TestSchemaManagerRun(t *testing.T) {
 	sql := "create table test_table (pk int)"
 	controller := newFakeController(

diff --git a/go/vt/schemamanager/tablet_executor.go b/go/vt/schemamanager/tablet_executor.go
@@ -36,48 +36,35 @@ import (
 	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/proto/vtrpc"
 )
 
 // 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
+	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
 }
 
 // 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)
@@ -147,48 +134,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 vterrors.Errorf(vtrpc.Code_INVALID_ARGUMENT, "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 vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "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
@@ -211,59 +181,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
-}
-
 // executeSQL executes a single SQL statement either as online DDL or synchronously on all tablets.
 // In online DDL case, the query may be exploded into multiple queries during
 func (exec *TabletExecutor) executeSQL(ctx context.Context, sql string, providedUUID string, execResult *ExecuteResult) (executedAsynchronously bool, err error) {

diff --git a/go/vt/schemamanager/tablet_executor_test.go b/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) {

diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go
@@ -264,9 +264,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 err = executor.SetDDLStrategy(req.DdlStrategy); err != nil {
 		err = vterrors.Wrapf(err, "invalid DdlStrategy: %s", req.DdlStrategy)

diff --git a/go/vt/vtctl/vtctl.go b/go/vt/vtctl/vtctl.go
@@ -587,8 +587,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>] {--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.",
+				params: "[--wait_replicas_timeout=10s] [--ddl_strategy=<ddl_strategy>] [--uuid_list=<comma_separated_uuids>] [--migration_context=<unique-request-context>] {--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.",
 			},
 			{
 				name:   "CopySchemaShard",
@@ -2863,7 +2863,7 @@ func commandValidateSchemaKeyspace(ctx context.Context, wr *wrangler.Wrangler, s
 }
 
 func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *pflag.FlagSet, args []string) error {
-	allowLongUnavailability := subFlags.Bool("allow_long_unavailability", false, "Allow large schema changes which incur a longer unavailability of the database.")
+	subFlags.MarkDeprecated("allow_long_unavailability", "")
 	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'")
@@ -2910,15 +2910,14 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *pf
 	log.Info("Calling ApplySchema on VtctldServer")
 
 	resp, err := wr.VtctldServer().ApplySchema(ctx, &vtctldatapb.ApplySchemaRequest{
-		Keyspace:                keyspace,
-		AllowLongUnavailability: *allowLongUnavailability,
-		DdlStrategy:             *ddlStrategy,
-		Sql:                     parts,
-		SkipPreflight:           true,
-		UuidList:                textutil.SplitDelimitedList(*uuidList),
-		MigrationContext:        *migrationContext,
-		WaitReplicasTimeout:     protoutil.DurationToProto(*waitReplicasTimeout),
-		CallerId:                cID,
+		Keyspace:            keyspace,
+		DdlStrategy:         *ddlStrategy,
+		Sql:                 parts,
+		SkipPreflight:       true,
+		UuidList:            textutil.SplitDelimitedList(*uuidList),
+		MigrationContext:    *migrationContext,
+		WaitReplicasTimeout: protoutil.DurationToProto(*waitReplicasTimeout),
+		CallerId:            cID,
 	})
 
 	if err != nil {