VReplication: Remove auto_increment clauses for MoveTables to a sharded keyspace

go/cmd/vtctldclient/command/vreplication/movetables/movetables.go

@@ -49,6 +49,7 @@ func registerCommands(root *cobra.Command) {
 	create.Flags().BoolVar(&createOptions.AtomicCopy, "atomic-copy", false, "(EXPERIMENTAL) A single copy phase is run for all tables from the source. Use this, for example, if your source keyspace has tables which use foreign key constraints.")
 	create.Flags().StringVar(&createOptions.WorkflowOptions.TenantId, "tenant-id", "", "(EXPERIMENTAL) The tenant ID to use for the MoveTables workflow into a multi-tenant keyspace.")
 	create.Flags().StringVar(&createOptions.WorkflowOptions.SourceKeyspaceAlias, "source-keyspace-alias", "", "(EXPERIMENTAL) Used currently only for multi-tenant migrations. This value will be used instead of the source keyspace name in the keyspace routing rules.")
+	create.Flags().BoolVar(&createOptions.WorkflowOptions.StripShardedAutoIncrement, "remove-sharded-auto-increment", true, "If moving the table(s) to a sharded keyspace, remove any auto_increment clauses when copying the schema to the target as sharded keyspaces should rely on either user/application generated values or Vitess sequences to ensure uniqueness.")
 	base.AddCommand(create)
 
 	opts := &common.SubCommandsOpts{

go/test/endtoend/vreplication/config_test.go

@@ -20,9 +20,11 @@ package vreplication
 // We violate the NO_ZERO_DATES and NO_ZERO_IN_DATE sql_modes that are enabled by default in
 // MySQL 5.7+ and MariaDB 10.2+ to ensure that vreplication still works everywhere and the
 // permissive sql_mode now used in vreplication causes no unwanted side effects.
-// The customer table also tests two important things:
+// The customer table also tests several important things:
 //  1. Composite or multi-column primary keys
 //  2. PKs that contain an ENUM column
+//  3. That we properly handle tables with auto_increment columns (which are stripped by default when
+//     moving the table to a sharded keyspace with vtctldclient and left in place when using vtctlclient)
 //
 // The Lead and Lead-1 tables also allows us to test several things:
 //  1. Mixed case identifiers
@@ -40,7 +42,7 @@ var (
 	// All standard user tables should have a primary key and at least one secondary key.
 	initialProductSchema = `
 create table product(pid int, description varbinary(128), date1 datetime not null default '0000-00-00 00:00:00', date2 datetime not null default '2021-00-01 00:00:00', primary key(pid), key(date1,date2)) CHARSET=utf8mb4;
-create table customer(cid int, name varchar(128) collate utf8mb4_bin, meta json default null, typ enum('individual','soho','enterprise'), sport set('football','cricket','baseball'),
+create table customer(cid int auto_increment, name varchar(128) collate utf8mb4_bin, meta json default null, typ enum('individual','soho','enterprise'), sport set('football','cricket','baseball'),
 	ts timestamp not null default current_timestamp, bits bit(2) default b'11', date1 datetime not null default '0000-00-00 00:00:00', 
 	date2 datetime not null default '2021-00-01 00:00:00', dec80 decimal(8,0), blb blob, primary key(cid,typ), key(name)) CHARSET=utf8mb4;
 create table customer_seq(id int, next_id bigint, cache bigint, primary key(id)) comment 'vitess_sequence';

go/test/endtoend/vreplication/resharding_workflows_v2_test.go

@@ -605,6 +605,14 @@ func testMoveTablesV2Workflow(t *testing.T) {
 
 	testVSchemaForSequenceAfterMoveTables(t)
 
+	// Confirm that the auto_increment clause on customer.cid was removed.
+	cs, err := vtgateConn.ExecuteFetch("show create table customer", 1, false)
+	require.NoError(t, err)
+	require.Len(t, cs.Rows, 1)
+	require.Len(t, cs.Rows[0], 2) // Table and "Create Table"
+	csddl := strings.ToLower(cs.Rows[0][1].ToString())
+	require.False(t, strings.Contains(csddl, "auto_increment"), "customer table still has auto_increment clause: %s", csddl)
+
 	createMoveTablesWorkflow(t, "Lead,Lead-1")
 	output, err = vc.VtctldClient.ExecuteCommandWithOutput(listAllArgs...)
 	require.NoError(t, err)

go/vt/vtctl/workflow/materializer.go

@@ -257,6 +257,20 @@ func (mz *materializer) deploySchema() error {
 	var sourceDDLs map[string]string
 	var mu sync.Mutex
 
+	// Auto-increment columns are typically used with unsharded MySQL tables
+	// but should not generally be used with sharded ones. Because it's common
+	// to use MoveTables to move table(s) from an unsharded keyspace to a
+	// sharded one we automatically remove the clauses by default to prevent
+	// accidents and avoid having to later do a costly ALTER TABLE operation
+	// to remove them.
+	// We do, however, allow the user to override this behavior and retain them.
+	removeAutoInc := false
+	if mz.workflowType == binlogdatapb.VReplicationWorkflowType_MoveTables &&
+		(mz.targetVSchema != nil && mz.targetVSchema.Keyspace != nil && mz.targetVSchema.Keyspace.Sharded) &&
+		(mz.ms != nil && mz.ms.GetWorkflowOptions().GetStripShardedAutoIncrement()) {
+		removeAutoInc = true
+	}
+
 	return forAllShards(mz.targetShards, func(target *topo.ShardInfo) error {
 		allTables := []string{"/.*/"}
 
@@ -301,7 +315,8 @@ func (mz *materializer) deploySchema() error {
 			}
 
 			createDDL := ts.CreateDdl
-			if createDDL == createDDLAsCopy || createDDL == createDDLAsCopyDropConstraint || createDDL == createDDLAsCopyDropForeignKeys {
+			// Make any necessary adjustments to the create DDL.
+			if removeAutoInc || createDDL == createDDLAsCopy || createDDL == createDDLAsCopyDropConstraint || createDDL == createDDLAsCopyDropForeignKeys {
 				if ts.SourceExpression != "" {
 					// Check for table if non-empty SourceExpression.
 					sourceTableName, err := mz.env.Parser().TableFromStatement(ts.SourceExpression)
@@ -336,6 +351,14 @@ func (mz *materializer) deploySchema() error {
 
 					ddl = strippedDDL
 				}
+
+				if removeAutoInc {
+					ddl, err = stripAutoIncrement(ddl, mz.env.Parser())
+					if err != nil {
+						return err
+					}
+				}
+
 				createDDL = ddl
 			}
 

go/vt/vtctl/workflow/materializer_test.go

@@ -195,6 +195,96 @@ func TestStripConstraints(t *testing.T) {
 	}
 }
 
+func TestStripAutoIncrement(t *testing.T) {
+	tcs := []struct {
+		desc      string
+		ddl       string
+		want      string
+		expectErr bool
+	}{
+		{
+			desc: "invalid DDL",
+			ddl: "CREATE TABLE `table1` (\n" +
+				"`id` massiveint NOT NULL,\n" +
+				"PRIMARY KEY (`id`)\n" +
+				") ENGINE=InnoDB DEFAULT CHARSET=latin1;",
+			expectErr: true,
+		},
+		{
+			desc: "has auto increment",
+			ddl: "CREATE TABLE `table1` (\n" +
+				"`id` int NOT NULL AUTO_INCREMENT,\n" +
+				"`c1` varchar(128),\n" +
+				"PRIMARY KEY (`id`)\n" +
+				") ENGINE=InnoDB DEFAULT CHARSET=latin1;",
+			want: "create table table1 (\n" +
+				"\tid int not null,\n" +
+				"\tc1 varchar(128),\n" +
+				"\tprimary key (id)\n" +
+				") ENGINE InnoDB,\n" +
+				"  CHARSET latin1",
+		},
+		{
+			desc: "has no auto increment",
+			ddl: "CREATE TABLE `table1` (\n" +
+				"`id` int NOT NULL,\n" +
+				"`c1` varchar(128),\n" +
+				"PRIMARY KEY (`id`)\n" +
+				") ENGINE=InnoDB DEFAULT CHARSET=latin1;",
+			want: "create table table1 (\n" +
+				"\tid int not null,\n" +
+				"\tc1 varchar(128),\n" +
+				"\tprimary key (id)\n" +
+				") ENGINE InnoDB,\n" +
+				"  CHARSET latin1",
+		},
+		{
+			desc: "has auto increment with secondary key",
+			ddl: "CREATE TABLE `table1` (\n" +
+				"`id` int NOT NULL auto_increment,\n" +
+				"`c1` varchar(128),\n" +
+				"`c2` varchar(128),\n" +
+				"UNIQUE KEY `c1` (`c1`),\n" +
+				"PRIMARY KEY (`id`)\n" +
+				") ENGINE=InnoDB DEFAULT CHARSET=latin1;",
+			want: "create table table1 (\n" +
+				"\tid int not null,\n" +
+				"\tc1 varchar(128),\n" +
+				"\tc2 varchar(128),\n" +
+				"\tunique key c1 (c1),\n" +
+				"\tprimary key (id)\n" +
+				") ENGINE InnoDB,\n" +
+				"  CHARSET latin1",
+		},
+		{
+			desc: "has auto increment with multi-col PK",
+			ddl: "CREATE TABLE `table1` (\n" +
+				"`id` int NOT NULL auto_increment,\n" +
+				"`c1` varchar(128) NOT NULL,\n" +
+				"`c2` varchar(128),\n" +
+				"PRIMARY KEY (`id`, `c2`)\n" +
+				") ENGINE=InnoDB DEFAULT CHARSET=latin1;",
+			want: "create table table1 (\n" +
+				"\tid int not null,\n" +
+				"\tc1 varchar(128) not null,\n" +
+				"\tc2 varchar(128),\n" +
+				"\tprimary key (id, c2)\n" +
+				") ENGINE InnoDB,\n" +
+				"  CHARSET latin1",
+		},
+	}
+
+	for _, tc := range tcs {
+		strippedDDL, err := stripAutoIncrement(tc.ddl, sqlparser.NewTestParser())
+		if tc.expectErr != (err != nil) {
+			require.Failf(t, "unexpected error result", "expected error %t, got: %v", tc.expectErr, err)
+		}
+		if strippedDDL != tc.want {
+			utils.MustMatch(t, tc.want, strippedDDL, fmt.Sprintf("stripped DDL %q does not match our expected result: %q", strippedDDL, tc.want))
+		}
+	}
+}
+
 func TestAddTablesToVSchema(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()

go/vt/vtctl/workflow/utils.go

@@ -217,6 +217,31 @@ func stripTableForeignKeys(ddl string, parser *sqlparser.Parser) (string, error)
 	return newDDL, nil
 }
 
+func stripAutoIncrement(ddl string, parser *sqlparser.Parser) (string, error) {
+	ast, err := parser.ParseStrictDDL(ddl)
+	if err != nil {
+		return "", err
+	}
+
+	stripAutoIncrement := func(cursor *sqlparser.Cursor) bool {
+		switch node := cursor.Node().(type) {
+		case sqlparser.DDLStatement:
+			if node.GetTableSpec() != nil {
+				for _, column := range node.GetTableSpec().Columns {
+					if column.Type.Options.Autoincrement {
+						column.Type.Options.Autoincrement = false
+					}
+				}
+			}
+		}
+		return true
+	}
+
+	noAutoIncAST := sqlparser.Rewrite(ast, stripAutoIncrement, nil)
+	newDDL := sqlparser.String(noAutoIncAST)
+	return newDDL, nil
+}
+
 func getSourceTableDDLs(ctx context.Context, ts *topo.Server, tmc tmclient.TabletManagerClient, shards []*topo.ShardInfo) (map[string]string, error) {
 	sourceDDLs := make(map[string]string)
 	allTables := []string{"/.*/"}

proto/vtctldata.proto

@@ -209,6 +209,9 @@ message Shard {
 message WorkflowOptions {
   string tenant_id = 1;
   string source_keyspace_alias = 2;
+  // Remove auto_increment clauses on tables when moving them to a sharded
+  // keyspace.
+  bool strip_sharded_auto_increment = 3;
 }
 
 // TODO: comment the hell out of this.