diff --git a/go/cmd/vtctldclient/command/schema.go b/go/cmd/vtctldclient/command/schema.go index 8abe8bd0b94..a2d7843756d 100644 --- 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 ] [--uuid ...] [--migration-context ] [--wait-replicas-timeout ] [--skip-preflight] [--caller-id ] {--sql-file | --sql } ", + Use: "ApplySchema [--allow-long-unavailability] [--ddl-strategy ] [--uuid ...] [--migration-context ] [--wait-replicas-timeout ] [--caller-id ] {--sql-file | --sql } ", 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. @@ -141,7 +141,7 @@ func commandApplySchema(cmd *cobra.Command, args []string) error { AllowLongUnavailability: applySchemaOptions.AllowLongUnavailability, DdlStrategy: applySchemaOptions.DDLStrategy, Sql: parts, - SkipPreflight: applySchemaOptions.SkipPreflight, + SkipPreflight: true, UuidList: applySchemaOptions.UUIDList, MigrationContext: applySchemaOptions.MigrationContext, WaitReplicasTimeout: protoutil.DurationToProto(applySchemaOptions.WaitReplicasTimeout), @@ -286,12 +286,12 @@ func commandReloadSchemaShard(cmd *cobra.Command, args []string) error { } func init() { + 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.") ApplySchema.Flags().DurationVar(&applySchemaOptions.WaitReplicasTimeout, "wait-replicas-timeout", wrangler.DefaultWaitReplicasTimeout, "Amount of time to wait for replicas to receive the schema change via replication.") - ApplySchema.Flags().BoolVar(&applySchemaOptions.SkipPreflight, "skip-preflight", false, "Skip pre-apply schema checks, and directly forward schema change query to shards.") ApplySchema.Flags().StringVar(&applySchemaOptions.CallerID, "caller-id", "", "Effective caller ID used for the operation and should map to an ACL name which grants this identity the necessary permissions to perform the operation (this is only necessary when strict table ACLs are used).") ApplySchema.Flags().StringArrayVar(&applySchemaOptions.SQL, "sql", nil, "Semicolon-delimited, repeatable SQL commands to apply. Exactly one of --sql|--sql-file is required.") ApplySchema.Flags().StringVar(&applySchemaOptions.SQLFile, "sql-file", "", "Path to a file containing semicolon-delimited SQL commands to apply. Exactly one of --sql|--sql-file is required.") diff --git a/go/test/endtoend/cluster/vtctlclient_process.go b/go/test/endtoend/cluster/vtctlclient_process.go index 369ffcb09c7..3c4941da2c2 100644 --- a/go/test/endtoend/cluster/vtctlclient_process.go +++ b/go/test/endtoend/cluster/vtctlclient_process.go @@ -42,7 +42,6 @@ type VtctlClientProcess struct { type VtctlClientParams struct { DDLStrategy string MigrationContext string - SkipPreflight bool UUIDList string CallerID string } @@ -88,9 +87,6 @@ func (vtctlclient *VtctlClientProcess) ApplySchemaWithOutput(Keyspace string, SQ if params.UUIDList != "" { args = append(args, "--uuid_list", params.UUIDList) } - if params.SkipPreflight { - args = append(args, "--skip_preflight") - } if params.CallerID != "" { args = append(args, "--caller_id", params.CallerID) @@ -101,7 +97,7 @@ func (vtctlclient *VtctlClientProcess) ApplySchemaWithOutput(Keyspace string, SQ // ApplySchema applies SQL schema to the keyspace func (vtctlclient *VtctlClientProcess) ApplySchema(Keyspace string, SQL string) error { - message, err := vtctlclient.ApplySchemaWithOutput(Keyspace, SQL, VtctlClientParams{DDLStrategy: "direct -allow-zero-in-date", SkipPreflight: true}) + message, err := vtctlclient.ApplySchemaWithOutput(Keyspace, SQL, VtctlClientParams{DDLStrategy: "direct -allow-zero-in-date"}) return vterrors.Wrap(err, message) } diff --git a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go b/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go index 875ac646d2c..7bed08c71ca 100644 --- a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go +++ b/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go @@ -771,7 +771,7 @@ func testScheduler(t *testing.T) { t.Run("Idempotent submission, retry failed migration", func(t *testing.T) { uuid := "00000000_1111_2222_3333_444444444444" - overrideVtctlParams = &cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true, UUIDList: uuid, MigrationContext: "idempotent:1111-2222-3333"} + overrideVtctlParams = &cluster.VtctlClientParams{DDLStrategy: ddlStrategy, UUIDList: uuid, MigrationContext: "idempotent:1111-2222-3333"} defer func() { overrideVtctlParams = nil }() // create a migration and cancel it. We don't let it complete. We want it in "failed" state t.Run("start and fail migration", func(t *testing.T) { @@ -807,7 +807,7 @@ func testScheduler(t *testing.T) { t.Run("Idempotent submission, retry failed migration in singleton context", func(t *testing.T) { uuid := "00000000_1111_3333_3333_444444444444" ddlStrategy := ddlStrategy + " --singleton-context" - overrideVtctlParams = &cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true, UUIDList: uuid, MigrationContext: "idempotent:1111-3333-3333"} + overrideVtctlParams = &cluster.VtctlClientParams{DDLStrategy: ddlStrategy, UUIDList: uuid, MigrationContext: "idempotent:1111-3333-3333"} defer func() { overrideVtctlParams = nil }() // create a migration and cancel it. We don't let it complete. We want it in "failed" state t.Run("start and fail migration", func(t *testing.T) { @@ -2075,7 +2075,7 @@ func testForeignKeys(t *testing.T) { continue } statement := fmt.Sprintf("DROP TABLE IF EXISTS %s", artifact) - _, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, statement, cluster.VtctlClientParams{DDLStrategy: "direct", SkipPreflight: true}) + _, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, statement, cluster.VtctlClientParams{DDLStrategy: "direct"}) if err == nil { droppedTables[artifact] = true } @@ -2107,7 +2107,7 @@ func testOnlineDDLStatement(t *testing.T, params *testOnlineDDLStatementParams) } } } else { - vtctlParams := &cluster.VtctlClientParams{DDLStrategy: params.ddlStrategy, MigrationContext: params.migrationContext, SkipPreflight: true} + vtctlParams := &cluster.VtctlClientParams{DDLStrategy: params.ddlStrategy, MigrationContext: params.migrationContext} if overrideVtctlParams != nil { vtctlParams = overrideVtctlParams } @@ -2156,7 +2156,7 @@ func testRevertMigration(t *testing.T, params *testRevertMigrationParams) (uuid } } } else { - output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: params.ddlStrategy, MigrationContext: params.migrationContext, SkipPreflight: true}) + output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: params.ddlStrategy, MigrationContext: params.migrationContext}) if params.expectError == "" { assert.NoError(t, err) uuid = output diff --git a/go/test/endtoend/onlineddl/vtctlutil.go b/go/test/endtoend/onlineddl/vtctlutil.go index 70aa3b6e3ec..a5d3293547e 100644 --- a/go/test/endtoend/onlineddl/vtctlutil.go +++ b/go/test/endtoend/onlineddl/vtctlutil.go @@ -29,7 +29,7 @@ import ( func CheckCancelAllMigrationsViaVtctl(t *testing.T, vtctlclient *cluster.VtctlClientProcess, keyspace string) { cancelQuery := "alter vitess_migration cancel all" - _, err := vtctlclient.ApplySchemaWithOutput(keyspace, cancelQuery, cluster.VtctlClientParams{SkipPreflight: true}) + _, err := vtctlclient.ApplySchemaWithOutput(keyspace, cancelQuery, cluster.VtctlClientParams{}) assert.NoError(t, err) } diff --git a/go/test/endtoend/vreplication/resharding_workflows_v2_test.go b/go/test/endtoend/vreplication/resharding_workflows_v2_test.go index d095477fc7f..fada3b621c2 100644 --- a/go/test/endtoend/vreplication/resharding_workflows_v2_test.go +++ b/go/test/endtoend/vreplication/resharding_workflows_v2_test.go @@ -779,7 +779,7 @@ func createAdditionalCustomerShards(t *testing.T, shards string) { } func tstApplySchemaOnlineDDL(t *testing.T, sql string, keyspace string) { - err := vc.VtctlClient.ExecuteCommand("ApplySchema", "--", "--skip_preflight", "--ddl_strategy=online", + err := vc.VtctlClient.ExecuteCommand("ApplySchema", "--", "--ddl_strategy=online", "--sql", sql, keyspace) require.NoError(t, err, fmt.Sprintf("ApplySchema Error: %s", err)) } diff --git a/go/test/endtoend/vtgate/schema/schema_test.go b/go/test/endtoend/vtgate/schema/schema_test.go index 38cc74514df..456c5589f37 100644 --- a/go/test/endtoend/vtgate/schema/schema_test.go +++ b/go/test/endtoend/vtgate/schema/schema_test.go @@ -105,7 +105,6 @@ func TestSchemaChange(t *testing.T) { testWithAlterSchema(t) testWithAlterDatabase(t) testWithDropCreateSchema(t) - testSchemaChangePreflightErrorPartially(t) testDropNonExistentTables(t) testCreateInvalidView(t) testCopySchemaShards(t, clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].VttabletProcess.TabletPath, 2) @@ -195,17 +194,6 @@ func matchSchema(t *testing.T, firstTablet string, secondTablet string) { assert.Equal(t, firstShardSchema, secondShardSchema) } -// testSchemaChangePreflightErrorPartially applying same schema + new schema should throw error for existing one -// Tests that some SQL statements fail properly during PreflightSchema. -func testSchemaChangePreflightErrorPartially(t *testing.T) { - createNewTable := fmt.Sprintf(createTable, fmt.Sprintf("vt_select_test_%02d", 5)) + fmt.Sprintf(createTable, fmt.Sprintf("vt_select_test_%02d", 2)) - output, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ApplySchema", "--", "--sql", createNewTable, keyspaceName) - require.Error(t, err) - assert.True(t, strings.Contains(output, "already exists")) - - checkTables(t, totalTableCount) -} - // testDropNonExistentTables applying same schema + new schema should throw error for existing one and also add the new schema // If a table does not exist, DROP TABLE should error during preflight // because the statement does not change the schema as there is @@ -230,7 +218,7 @@ func testDropNonExistentTables(t *testing.T) { func testCreateInvalidView(t *testing.T) { for _, ddlStrategy := range []string{"direct", "direct -allow-zero-in-date"} { createInvalidView := "CREATE OR REPLACE VIEW invalid_view AS SELECT * FROM nonexistent_table;" - output, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ApplySchema", "--", "--skip_preflight", "--ddl_strategy", ddlStrategy, "--sql", createInvalidView, keyspaceName) + output, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ApplySchema", "--", "--ddl_strategy", ddlStrategy, "--sql", createInvalidView, keyspaceName) require.Error(t, err) assert.Contains(t, output, "doesn't exist (errno 1146)") } diff --git a/go/vt/schemamanager/tablet_executor.go b/go/vt/schemamanager/tablet_executor.go index 9609adeedb2..3ca154b77b4 100644 --- a/go/vt/schemamanager/tablet_executor.go +++ b/go/vt/schemamanager/tablet_executor.go @@ -51,7 +51,6 @@ type TabletExecutor struct { waitReplicasTimeout time.Duration ddlStrategySetting *schema.DDLStrategySetting uuids []string - skipPreflight bool } // NewTabletExecutor creates a new TabletExecutor instance @@ -110,11 +109,6 @@ func (exec *TabletExecutor) hasProvidedUUIDs() bool { return len(exec.uuids) != 0 } -// SkipPreflight disables preflight checks -func (exec *TabletExecutor) SkipPreflight() { - exec.skipPreflight = true -} - // Open opens a connection to the primary for every shard. func (exec *TabletExecutor) Open(ctx context.Context, keyspace string) error { if !exec.isClosed { @@ -270,14 +264,6 @@ func (exec *TabletExecutor) detectBigSchemaChanges(ctx context.Context, parsedDD return false, nil } -func (exec *TabletExecutor) preflightSchemaChanges(ctx context.Context, sqls []string) error { - if exec.skipPreflight { - return nil - } - _, err := exec.tmc.PreflightSchema(ctx, exec.tablets[0], sqls) - return err -} - // 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) { @@ -349,12 +335,6 @@ func (exec *TabletExecutor) Execute(ctx context.Context, sqls []string) *Execute } }() - // Make sure the schema changes introduce a table definition change. - if err := exec.preflightSchemaChanges(ctx, sqls); err != nil { - execResult.ExecutorErr = err.Error() - return &execResult - } - if exec.hasProvidedUUIDs() && len(exec.uuids) != len(sqls) { execResult.ExecutorErr = fmt.Sprintf("provided %v UUIDs do not match number of DDLs %v", len(exec.uuids), len(sqls)) return &execResult diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index 2ab12ec8c9c..8f0b01fcb94 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -222,7 +222,6 @@ func (s *VtctldServer) ApplySchema(ctx context.Context, req *vtctldatapb.ApplySc defer panicHandler(&err) span.Annotate("keyspace", req.Keyspace) - span.Annotate("skip_preflight", req.SkipPreflight) span.Annotate("ddl_strategy", req.DdlStrategy) if len(req.Sql) == 0 { @@ -268,9 +267,6 @@ func (s *VtctldServer) ApplySchema(ctx context.Context, req *vtctldatapb.ApplySc if req.AllowLongUnavailability { executor.AllowBigSchemaChange() } - if req.SkipPreflight { - executor.SkipPreflight() - } 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 index 513ef26fc42..5b2957bd68c 100644 --- a/go/vt/vtctl/vtctl.go +++ b/go/vt/vtctl/vtctl.go @@ -614,8 +614,8 @@ var commands = []commandGroup{ { name: "ApplySchema", method: commandApplySchema, - params: "[--allow_long_unavailability] [--wait_replicas_timeout=10s] [--ddl_strategy=] [--uuid_list=] [--migration_context=] [--skip_preflight] {--sql= || --sql-file=} ", - 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: "[--allow_long_unavailability] [--wait_replicas_timeout=10s] [--ddl_strategy=] [--uuid_list=] [--migration_context=] {--sql= || --sql-file=} ", + 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.", }, { name: "CopySchemaShard", @@ -2877,7 +2877,7 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *pf migrationContext := subFlags.String("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") requestContext := subFlags.String("request_context", "", "synonym for --migration_context") waitReplicasTimeout := subFlags.Duration("wait_replicas_timeout", wrangler.DefaultWaitReplicasTimeout, "The amount of time to wait for replicas to receive the schema change via replication.") - skipPreflight := subFlags.Bool("skip_preflight", false, "Skip pre-apply schema checks, and directly forward schema change query to shards") + skipPreflight := subFlags.Bool("skip_preflight", false, "Deprecated. Always assumed to be 'true'") callerID := subFlags.String("caller_id", "", "This is the effective caller ID used for the operation and should map to an ACL name which grants this identity the necessary permissions to perform the operation (this is only necessary when strict table ACLs are used)") if err := subFlags.Parse(args); err != nil { @@ -2886,6 +2886,9 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *pf if subFlags.NArg() != 1 { return fmt.Errorf("the argument is required for the commandApplySchema command") } + if *skipPreflight { + log.Warningf("--skip_preflight flag is deprecated. Always assumed to be 'true'") + } keyspace := subFlags.Arg(0) change, err := getFileParam(*sql, *sqlFile, "sql") @@ -2917,7 +2920,7 @@ func commandApplySchema(ctx context.Context, wr *wrangler.Wrangler, subFlags *pf AllowLongUnavailability: *allowLongUnavailability, DdlStrategy: *ddlStrategy, Sql: parts, - SkipPreflight: *skipPreflight, + SkipPreflight: true, UuidList: textutil.SplitDelimitedList(*uuidList), MigrationContext: *migrationContext, WaitReplicasTimeout: protoutil.DurationToProto(*waitReplicasTimeout),