diff --git a/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go b/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go index 9a63ac29960..0efed92f440 100644 --- a/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go +++ b/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go @@ -1142,7 +1142,7 @@ func testRevert(t *testing.T) { checkPartitionedTableCountRows(t, 6) }) t.Run("partitions: drop first partition", func(t *testing.T) { - uuid := testOnlineDDLStatementForTable(t, "alter table part_test drop partition `p1`", ddlStrategy+" --fast-range-rotation", "vtgate", "") + uuid := testOnlineDDLStatementForTable(t, "alter table part_test drop partition `p1`", ddlStrategy, "vtgate", "") uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, partitionedTableName, true) @@ -1157,7 +1157,7 @@ func testRevert(t *testing.T) { checkPartitionedTableCountRows(t, 5) }) t.Run("partitions: add new partition", func(t *testing.T) { - uuid := testOnlineDDLStatementForTable(t, "alter table part_test add partition (PARTITION p7 VALUES LESS THAN (70))", ddlStrategy+" --fast-range-rotation", "vtgate", "") + uuid := testOnlineDDLStatementForTable(t, "alter table part_test add partition (PARTITION p7 VALUES LESS THAN (70))", ddlStrategy, "vtgate", "") uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, partitionedTableName, true) diff --git a/go/vt/schema/ddl_strategy.go b/go/vt/schema/ddl_strategy.go index 71d434b5e09..e3b03c3f330 100644 --- a/go/vt/schema/ddl_strategy.go +++ b/go/vt/schema/ddl_strategy.go @@ -207,11 +207,6 @@ func (setting *DDLStrategySetting) IsPreferInstantDDL() bool { return setting.hasFlag(preferInstantDDL) } -// IsFastRangeRotationFlag checks if strategy options include --fast-range-rotation -func (setting *DDLStrategySetting) IsFastRangeRotationFlag() bool { - return setting.hasFlag(fastRangeRotationFlag) -} - // isCutOverThresholdFlag returns true when given option denotes a `--cut-over-threshold=[...]` flag func isCutOverThresholdFlag(opt string) (string, bool) { submatch := cutOverThresholdFlagRegexp.FindStringSubmatch(opt) @@ -324,7 +319,7 @@ func (setting *DDLStrategySetting) RuntimeOptions() []string { } switch { case isFlag(opt, declarativeFlag): - case isFlag(opt, skipTopoFlag): + case isFlag(opt, skipTopoFlag): // deprecated flag, parsed for backwards compatibility case isFlag(opt, singletonFlag): case isFlag(opt, singletonContextFlag): case isFlag(opt, allowZeroInDateFlag): @@ -333,7 +328,7 @@ func (setting *DDLStrategySetting) RuntimeOptions() []string { case isFlag(opt, inOrderCompletionFlag): case isFlag(opt, allowConcurrentFlag): case isFlag(opt, preferInstantDDL): - case isFlag(opt, fastRangeRotationFlag): + case isFlag(opt, fastRangeRotationFlag): // deprecated flag, parsed for backwards compatibility case isFlag(opt, vreplicationTestSuite): case isFlag(opt, allowForeignKeysFlag): case isFlag(opt, analyzeTableFlag): diff --git a/go/vt/schema/ddl_strategy_test.go b/go/vt/schema/ddl_strategy_test.go index ae6c65815cc..f27f0963e80 100644 --- a/go/vt/schema/ddl_strategy_test.go +++ b/go/vt/schema/ddl_strategy_test.go @@ -384,7 +384,6 @@ func TestParseDDLStrategy(t *testing.T) { assert.Equal(t, ts.isPostponeLaunch, setting.IsPostponeLaunch()) assert.Equal(t, ts.isAllowConcurrent, setting.IsAllowConcurrent()) assert.Equal(t, ts.fastOverRevertible, setting.IsPreferInstantDDL()) - assert.Equal(t, ts.fastRangeRotation, setting.IsFastRangeRotationFlag()) assert.Equal(t, ts.allowForeignKeys, setting.IsAllowForeignKeysFlag()) assert.Equal(t, ts.analyzeTable, setting.IsAnalyzeTableFlag()) cutOverThreshold, err := setting.CutOverThreshold() diff --git a/go/vt/schemadiff/analysis.go b/go/vt/schemadiff/analysis.go new file mode 100644 index 00000000000..ae0f22559f2 --- /dev/null +++ b/go/vt/schemadiff/analysis.go @@ -0,0 +1,68 @@ +/* +Copyright 2024 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package schemadiff + +import ( + "vitess.io/vitess/go/vt/sqlparser" +) + +// AlterTableRotatesRangePartition answers `true` when the given ALTER TABLE statement performs any sort +// of range partition rotation, that is applicable immediately and without moving data. +// Such would be: +// - Dropping any partition(s) +// - Adding a new partition (empty, at the end of the list) +func AlterTableRotatesRangePartition(createTable *sqlparser.CreateTable, alterTable *sqlparser.AlterTable) (bool, error) { + // Validate original table is partitioned by RANGE + if createTable.TableSpec.PartitionOption == nil { + return false, nil + } + if createTable.TableSpec.PartitionOption.Type != sqlparser.RangeType { + return false, nil + } + + spec := alterTable.PartitionSpec + if spec == nil { + return false, nil + } + errorResult := func(conflictingNode sqlparser.SQLNode) error { + return &PartitionSpecNonExclusiveError{ + Table: alterTable.Table.Name.String(), + PartitionSpec: spec, + ConflictingStatement: sqlparser.CanonicalString(conflictingNode), + } + } + if len(alterTable.AlterOptions) > 0 { + // This should never happen, unless someone programmatically tampered with the AlterTable AST. + return false, errorResult(alterTable.AlterOptions[0]) + } + if alterTable.PartitionOption != nil { + // This should never happen, unless someone programmatically tampered with the AlterTable AST. + return false, errorResult(alterTable.PartitionOption) + } + switch spec.Action { + case sqlparser.AddAction: + if len(spec.Definitions) > 1 { + // This should never happen, unless someone programmatically tampered with the AlterTable AST. + return false, errorResult(spec.Definitions[1]) + } + return true, nil + case sqlparser.DropAction: + return true, nil + default: + return false, nil + } +} diff --git a/go/vt/schemadiff/analysis_test.go b/go/vt/schemadiff/analysis_test.go new file mode 100644 index 00000000000..b0092fb7aac --- /dev/null +++ b/go/vt/schemadiff/analysis_test.go @@ -0,0 +1,83 @@ +/* +Copyright 2024 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package schemadiff + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "vitess.io/vitess/go/vt/sqlparser" +) + +// AnalyzePartitionRotation analyzes a given AlterTable statement to see whether it has partition rotation +// commands, and if so, is the ALTER TABLE statement valid in MySQL. In MySQL, a single ALTER TABLE statement +// cannot apply multiple rotation commands, nor can it mix rotation commands with other types of changes. +func TestAlterTableRotatesRangePartition(t *testing.T) { + tcases := []struct { + create string + alter string + expect bool + }{ + { + alter: "ALTER TABLE t ADD PARTITION (PARTITION p1 VALUES LESS THAN (10))", + expect: true, + }, + { + alter: "ALTER TABLE t DROP PARTITION p1", + expect: true, + }, + { + alter: "ALTER TABLE t DROP PARTITION p1, p2", + expect: true, + }, + { + alter: "ALTER TABLE t TRUNCATE PARTITION p3", + }, + { + alter: "ALTER TABLE t COALESCE PARTITION 3", + }, + { + alter: "ALTER TABLE t partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", + }, + { + alter: "ALTER TABLE t ADD COLUMN c1 INT, DROP COLUMN c2", + }, + } + + for _, tcase := range tcases { + t.Run(tcase.alter, func(t *testing.T) { + if tcase.create == "" { + tcase.create = "CREATE TABLE t (id int PRIMARY KEY) PARTITION BY RANGE (id) (PARTITION p0 VALUES LESS THAN (10))" + } + stmt, err := sqlparser.NewTestParser().ParseStrictDDL(tcase.create) + require.NoError(t, err) + createTable, ok := stmt.(*sqlparser.CreateTable) + require.True(t, ok) + + stmt, err = sqlparser.NewTestParser().ParseStrictDDL(tcase.alter) + require.NoError(t, err) + alterTable, ok := stmt.(*sqlparser.AlterTable) + require.True(t, ok) + + result, err := AlterTableRotatesRangePartition(createTable, alterTable) + require.NoError(t, err) + assert.Equal(t, tcase.expect, result) + }) + } +} diff --git a/go/vt/schemadiff/errors.go b/go/vt/schemadiff/errors.go index 02a192a925d..a941c406be0 100644 --- a/go/vt/schemadiff/errors.go +++ b/go/vt/schemadiff/errors.go @@ -22,6 +22,7 @@ import ( "strings" "vitess.io/vitess/go/sqlescape" + "vitess.io/vitess/go/vt/sqlparser" ) var ( @@ -468,3 +469,21 @@ func (e *SubsequentDiffRejectedError) Error() string { } return b.String() } + +// PartitionSpecNonExclusiveError is returned when a partition spec change is found alongside other changes. +// for example, in MySQL it is invalid to both DROP PARTITION (a partition spec change) and ADD COLUMN +// in the same ALTER TABLE statement. In fact, even two partition spec changes in the same ALTER TABLE +// statement are not allowed. +// This error should never be encountered in normal circumstances, because: +// - `sqlparser` should not allow such statements to be parsed. +// - schemadiff's `Diff()` function will never generate a single `ALTER TABLE` statement with such multiple changes. +// The error is used for integrity checks only, and should be considered a bug if encountered. +type PartitionSpecNonExclusiveError struct { + Table string + PartitionSpec *sqlparser.PartitionSpec + ConflictingStatement string +} + +func (e *PartitionSpecNonExclusiveError) Error() string { + return fmt.Sprintf("ALTER TABLE on %s, may only have a single partition spec change, and other changes are not allowed. Found spec: %s; and change: %s", sqlescape.EscapeID(e.Table), sqlparser.CanonicalString(e.PartitionSpec), e.ConflictingStatement) +} diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go index 6da003bb8d7..f8ecfd6081e 100644 --- a/go/vt/schemadiff/table.go +++ b/go/vt/schemadiff/table.go @@ -19,6 +19,7 @@ package schemadiff import ( "fmt" "math" + "slices" "sort" "strconv" "strings" @@ -1203,52 +1204,64 @@ func (c *CreateTableEntity) isRangePartitionsRotation( if t1Partitions.Type != sqlparser.RangeType { return false, nil, nil } - definitions1 := t1Partitions.Definitions + definitions1 := slices.Clone(t1Partitions.Definitions) definitions2 := t2Partitions.Definitions - // there has to be a non-empty shared list, therefore both definitions must be non-empty: if len(definitions1) == 0 { return false, nil, nil } if len(definitions2) == 0 { return false, nil, nil } + definitions2map := make(map[string]*sqlparser.PartitionDefinition, len(definitions2)) + for _, definition := range definitions2 { + definitions2map[sqlparser.CanonicalString(definition)] = definition + } + // Find dropped partitions: var droppedPartitions1 []*sqlparser.PartitionDefinition - // It's OK for prefix of t1 partitions to be nonexistent in t2 (as they may have been rotated away in t2) - for len(definitions1) > 0 && !sqlparser.Equals.RefOfPartitionDefinition(definitions1[0], definitions2[0]) { - droppedPartitions1 = append(droppedPartitions1, definitions1[0]) - definitions1 = definitions1[1:] + for i := len(definitions1) - 1; i >= 0; i-- { + definition := definitions1[i] + if _, ok := definitions2map[sqlparser.CanonicalString(definition)]; !ok { + // In range partitioning, it's allowed to drop any partition, whether it's the first, somewhere in the middle, or last. + droppedPartitions1 = append(droppedPartitions1, definition) + // We remove the definition from the list, so that we can then compare the remaining definitions + definitions1 = append(definitions1[:i], definitions1[i+1:]...) + } } + slices.Reverse(droppedPartitions1) if len(definitions1) == 0 { - // We've exhausted definition1 trying to find a shared partition with definitions2. Nothing found. - // so there is no shared sequence between the two tables. + // Nothing shared between the two partition lists. return false, nil, nil } + // In range partitioning, it's only allowed to ADD one partition at the end of the range. + // We allow multiple here, and the diff mechanism will later split them to subsequent diffs. + + // Let's now validate that any added partitions in t2Partitions are strictly a suffix of t1Partitions if len(definitions1) > len(definitions2) { return false, nil, nil } - // To save computation, and because we've already shown that sqlparser.EqualsRefOfPartitionDefinition(definitions1[0], definitions2[0]), nil, - // we can skip one element - definitions1 = definitions1[1:] - definitions2 = definitions2[1:] - // Now let's ensure that whatever is remaining in definitions1 is an exact match for a prefix of definitions2 - // It's ok if we end up with leftover elements in definition2 - for len(definitions1) > 0 { - if !sqlparser.Equals.RefOfPartitionDefinition(definitions1[0], definitions2[0]) { + for i := range definitions1 { + if !sqlparser.Equals.RefOfPartitionDefinition(definitions1[i], definitions2[i]) { + // Not a suffix return false, nil, nil } - definitions1 = definitions1[1:] - definitions2 = definitions2[1:] } - addedPartitions2 := definitions2 - partitionSpecs := make([]*sqlparser.PartitionSpec, 0, len(droppedPartitions1)+len(addedPartitions2)) - for _, p := range droppedPartitions1 { + // And the suffix is any remaining definitions + addedPartitions2 := definitions2[len(definitions1):] + + var partitionSpecs []*sqlparser.PartitionSpec + // Dropped partitions: + if len(droppedPartitions1) > 0 { + // A single DROP PARTITION clause can specify multiple partition names partitionSpec := &sqlparser.PartitionSpec{ Action: sqlparser.DropAction, - Names: []sqlparser.IdentifierCI{p.Name}, + } + for _, p := range droppedPartitions1 { + partitionSpec.Names = append(partitionSpec.Names, p.Name) + annotations.MarkRemoved(sqlparser.CanonicalString(p)) } partitionSpecs = append(partitionSpecs, partitionSpec) - annotations.MarkRemoved(sqlparser.CanonicalString(p)) } + // Added partitions: for _, p := range addedPartitions2 { partitionSpec := &sqlparser.PartitionSpec{ Action: sqlparser.AddAction, diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index ea209ce4eea..2177f1570e9 100644 --- a/go/vt/schemadiff/table_test.go +++ b/go/vt/schemadiff/table_test.go @@ -1332,6 +1332,17 @@ func TestCreateTableDiff(t *testing.T) { "-(PARTITION `p1` VALUES LESS THAN (10),", }, }, + { + name: "change partitioning range: statements, drop middle", + from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", + to: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p3 values less than (30))", + rotation: RangeRotationDistinctStatements, + diff: "alter table t1 drop partition p2", + cdiff: "ALTER TABLE `t1` DROP PARTITION `p2`", + textdiffs: []string{ + "- PARTITION `p2` VALUES LESS THAN (20),", + }, + }, { name: "change partitioning range: statements, add", from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20))", @@ -1344,12 +1355,12 @@ func TestCreateTableDiff(t *testing.T) { }, }, { - name: "change partitioning range: statements, multiple drops", + name: "change partitioning range: statements, multiple drops, distinct", from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", to: "create table t1 (id int primary key) partition by range (id) (partition p3 values less than (30))", rotation: RangeRotationDistinctStatements, - diffs: []string{"alter table t1 drop partition p1", "alter table t1 drop partition p2"}, - cdiffs: []string{"ALTER TABLE `t1` DROP PARTITION `p1`", "ALTER TABLE `t1` DROP PARTITION `p2`"}, + diffs: []string{"alter table t1 drop partition p1, p2"}, + cdiffs: []string{"ALTER TABLE `t1` DROP PARTITION `p1`, `p2`"}, textdiffs: []string{ "-(PARTITION `p1` VALUES LESS THAN (10),", "- PARTITION `p2` VALUES LESS THAN (20),", @@ -1392,8 +1403,8 @@ func TestCreateTableDiff(t *testing.T) { from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", to: "create table t1 (id int primary key, i int) partition by range (id) (partition p3 values less than (30))", rotation: RangeRotationDistinctStatements, - diffs: []string{"alter table t1 add column i int", "alter table t1 drop partition p1", "alter table t1 drop partition p2"}, - cdiffs: []string{"ALTER TABLE `t1` ADD COLUMN `i` int", "ALTER TABLE `t1` DROP PARTITION `p1`", "ALTER TABLE `t1` DROP PARTITION `p2`"}, + diffs: []string{"alter table t1 add column i int", "alter table t1 drop partition p1, p2"}, + cdiffs: []string{"ALTER TABLE `t1` ADD COLUMN `i` int", "ALTER TABLE `t1` DROP PARTITION `p1`, `p2`"}, textdiffs: []string{ "+ `i` int", "-(PARTITION `p1` VALUES LESS THAN (10),", @@ -1438,7 +1449,7 @@ func TestCreateTableDiff(t *testing.T) { }, }, { - name: "change partitioning range: ignore rotate, not a rotation", + name: "change partitioning range: not a rotation, ignore", from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", to: "create table t1 (id int primary key) partition by range (id) (partition p2 values less than (25), partition p3 values less than (30), partition p4 values less than (40))", rotation: RangeRotationIgnore, @@ -1456,43 +1467,83 @@ func TestCreateTableDiff(t *testing.T) { }, }, { - name: "change partitioning range: ignore rotate, not a rotation 2", + name: "change partitioning range: not a rotation, ignore 2", from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", - to: "create table t1 (id int primary key) partition by range (id) (partition p2 values less than (20), partition p3 values less than (35), partition p4 values less than (40))", + to: "create table t1 (id int primary key) partition by range (id) (partition p2 values less than (25), partition p3 values less than (30), partition p4 values less than (40))", rotation: RangeRotationIgnore, - diff: "alter table t1 \npartition by range (id)\n(partition p2 values less than (20),\n partition p3 values less than (35),\n partition p4 values less than (40))", - cdiff: "ALTER TABLE `t1` \nPARTITION BY RANGE (`id`)\n(PARTITION `p2` VALUES LESS THAN (20),\n PARTITION `p3` VALUES LESS THAN (35),\n PARTITION `p4` VALUES LESS THAN (40))", + diff: "alter table t1 \npartition by range (id)\n(partition p2 values less than (25),\n partition p3 values less than (30),\n partition p4 values less than (40))", + cdiff: "ALTER TABLE `t1` \nPARTITION BY RANGE (`id`)\n(PARTITION `p2` VALUES LESS THAN (25),\n PARTITION `p3` VALUES LESS THAN (30),\n PARTITION `p4` VALUES LESS THAN (40))", textdiffs: []string{ "-PARTITION BY RANGE (`id`)", "-(PARTITION `p1` VALUES LESS THAN (10),", "- PARTITION `p2` VALUES LESS THAN (20),", "- PARTITION `p3` VALUES LESS THAN (30))", "+PARTITION BY RANGE (`id`)", - "+(PARTITION `p2` VALUES LESS THAN (20)", + "+(PARTITION `p2` VALUES LESS THAN (25)", + "+ PARTITION `p3` VALUES LESS THAN (30),", + "+ PARTITION `p4` VALUES LESS THAN (40))", + }, + }, + { + name: "change partitioning range: complex rotate, ignore", + from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", + to: "create table t1 (id int primary key) partition by range (id) (partition p2 values less than (20), partition p3 values less than (35), partition p4 values less than (40))", + rotation: RangeRotationIgnore, + }, + { + name: "change partitioning range: complex rotate, distinct", + from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", + to: "create table t1 (id int primary key) partition by range (id) (partition p2 values less than (20), partition p3 values less than (35), partition p4 values less than (40))", + rotation: RangeRotationDistinctStatements, + diffs: []string{"alter table t1 drop partition p1, p3", "alter table t1 add partition (partition p3 values less than (35))", "alter table t1 add partition (partition p4 values less than (40))"}, + cdiffs: []string{"ALTER TABLE `t1` DROP PARTITION `p1`, `p3`", "ALTER TABLE `t1` ADD PARTITION (PARTITION `p3` VALUES LESS THAN (35))", "ALTER TABLE `t1` ADD PARTITION (PARTITION `p4` VALUES LESS THAN (40))"}, + textdiffs: []string{ + "-(PARTITION `p1` VALUES LESS THAN (10),", + "- PARTITION `p3` VALUES LESS THAN (30))", "+ PARTITION `p3` VALUES LESS THAN (35),", "+ PARTITION `p4` VALUES LESS THAN (40))", }, }, { - name: "change partitioning range: ignore rotate, not a rotation 3", + name: "change partitioning range: complex rotate 2, ignore", from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", to: "create table t1 (id int primary key) partition by range (id) (partition p2 values less than (20), partition pX values less than (30), partition p4 values less than (40))", rotation: RangeRotationIgnore, - diff: "alter table t1 \npartition by range (id)\n(partition p2 values less than (20),\n partition pX values less than (30),\n partition p4 values less than (40))", - cdiff: "ALTER TABLE `t1` \nPARTITION BY RANGE (`id`)\n(PARTITION `p2` VALUES LESS THAN (20),\n PARTITION `pX` VALUES LESS THAN (30),\n PARTITION `p4` VALUES LESS THAN (40))", + }, + { + name: "change partitioning range: complex rotate 2, distinct", + from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", + to: "create table t1 (id int primary key) partition by range (id) (partition p2 values less than (20), partition pX values less than (30), partition p4 values less than (40))", + rotation: RangeRotationDistinctStatements, + diffs: []string{"alter table t1 drop partition p1, p3", "alter table t1 add partition (partition pX values less than (30))", "alter table t1 add partition (partition p4 values less than (40))"}, + cdiffs: []string{"ALTER TABLE `t1` DROP PARTITION `p1`, `p3`", "ALTER TABLE `t1` ADD PARTITION (PARTITION `pX` VALUES LESS THAN (30))", "ALTER TABLE `t1` ADD PARTITION (PARTITION `p4` VALUES LESS THAN (40))"}, + textdiffs: []string{ + "-(PARTITION `p1` VALUES LESS THAN (10),", + "- PARTITION `p3` VALUES LESS THAN (30))", + "+ PARTITION `pX` VALUES LESS THAN (30),", + "+ PARTITION `p4` VALUES LESS THAN (40))", + }, + }, + { + name: "change partitioning range: not a rotation", + from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", + to: "create table t1 (id int primary key) partition by range (id) (partition p2 values less than (25), partition p3 values less than (30), partition p4 values less than (40))", + rotation: RangeRotationDistinctStatements, + diff: "alter table t1 \npartition by range (id)\n(partition p2 values less than (25),\n partition p3 values less than (30),\n partition p4 values less than (40))", + cdiff: "ALTER TABLE `t1` \nPARTITION BY RANGE (`id`)\n(PARTITION `p2` VALUES LESS THAN (25),\n PARTITION `p3` VALUES LESS THAN (30),\n PARTITION `p4` VALUES LESS THAN (40))", textdiffs: []string{ "-PARTITION BY RANGE (`id`)", "-(PARTITION `p1` VALUES LESS THAN (10),", "- PARTITION `p2` VALUES LESS THAN (20),", "- PARTITION `p3` VALUES LESS THAN (30))", "+PARTITION BY RANGE (`id`)", - "+(PARTITION `p2` VALUES LESS THAN (20)", - "+ PARTITION `pX` VALUES LESS THAN (30),", + "+(PARTITION `p2` VALUES LESS THAN (25)", + "+ PARTITION `p3` VALUES LESS THAN (30),", "+ PARTITION `p4` VALUES LESS THAN (40))", }, }, { - name: "change partitioning range: ignore rotate, not a rotation 4", + name: "change partitioning range: ignore rotate, not a rotation 2", from: "create table t1 (id int primary key) partition by range (id) (partition p1 values less than (10), partition p2 values less than (20), partition p3 values less than (30))", to: "create table t1 (id int primary key) partition by range (id) (partition pX values less than (20), partition p3 values less than (30), partition p4 values less than (40))", rotation: RangeRotationIgnore, @@ -2036,8 +2087,12 @@ func TestCreateTableDiff(t *testing.T) { if len(ts.diffs) > 0 { allSubsequentDiffs := AllSubsequent(alter) - require.Equal(t, len(ts.diffs), len(allSubsequentDiffs)) - require.Equal(t, len(ts.cdiffs), len(allSubsequentDiffs)) + allSubsequentDiffsStatements := []string{} + for _, d := range allSubsequentDiffs { + allSubsequentDiffsStatements = append(allSubsequentDiffsStatements, d.CanonicalStatementString()) + } + require.Equal(t, len(ts.diffs), len(allSubsequentDiffs), allSubsequentDiffsStatements) + require.Equal(t, len(ts.cdiffs), len(allSubsequentDiffs), allSubsequentDiffsStatements) for i := range ts.diffs { assert.Equal(t, ts.diffs[i], allSubsequentDiffs[i].StatementString()) assert.Equal(t, ts.cdiffs[i], allSubsequentDiffs[i].CanonicalStatementString()) diff --git a/go/vt/sqlparser/parse_test.go b/go/vt/sqlparser/parse_test.go index 6c0e32acf45..1159db80584 100644 --- a/go/vt/sqlparser/parse_test.go +++ b/go/vt/sqlparser/parse_test.go @@ -6081,6 +6081,18 @@ var ( input: "create table 2t.3t2 (c1 bigint not null, c2 text, primary key(c1))", output: "syntax error at position 18 near '.3'", excludeMulti: true, + }, { + input: "ALTER TABLE t ADD PARTITION (PARTITION p10 VALUES LESS THAN (10)), ADD PARTITION (PARTITION p20 VALUES LESS THAN (20))", + output: "syntax error at position 67", + }, { + input: "ALTER TABLE t DROP PARTITION p1, DROP PARTITION p2", + output: "syntax error at position 38 near 'DROP'", + }, { + input: "ALTER TABLE t DROP PARTITION p1, ADD COLUMN c INT", + output: "syntax error at position 37 near 'ADD'", + }, { + input: "ALTER TABLE t ADD COLUMN c INT, DROP PARTITION p1", + output: "syntax error at position 47 near 'PARTITION'", }, { input: "execute stmt1 using a, @b", output: "syntax error at position 22 near 'a'", diff --git a/go/vt/vttablet/onlineddl/analysis.go b/go/vt/vttablet/onlineddl/analysis.go index 1dc073bb7d0..970104877f2 100644 --- a/go/vt/vttablet/onlineddl/analysis.go +++ b/go/vt/vttablet/onlineddl/analysis.go @@ -31,9 +31,8 @@ import ( type specialAlterOperation string const ( - instantDDLSpecialOperation specialAlterOperation = "instant-ddl" - dropRangePartitionSpecialOperation specialAlterOperation = "drop-range-partition" - addRangePartitionSpecialOperation specialAlterOperation = "add-range-partition" + instantDDLSpecialOperation specialAlterOperation = "instant-ddl" + rangePartitionSpecialOperation specialAlterOperation = "range-partition" ) type SpecialAlterPlan struct { @@ -86,95 +85,6 @@ func (e *Executor) getCreateTableStatement(ctx context.Context, tableName string return createTable, nil } -// analyzeDropRangePartition sees if the online DDL drops a single partition in a range partitioned table -func analyzeDropRangePartition(alterTable *sqlparser.AlterTable, createTable *sqlparser.CreateTable) (*SpecialAlterPlan, error) { - // we are looking for a `ALTER TABLE DROP PARTITION ` statement with nothing else - if len(alterTable.AlterOptions) > 0 { - return nil, nil - } - if alterTable.PartitionOption != nil { - return nil, nil - } - spec := alterTable.PartitionSpec - if spec == nil { - return nil, nil - } - if spec.Action != sqlparser.DropAction { - return nil, nil - } - if len(spec.Names) != 1 { - return nil, vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "vitess only supports dropping a single partition per query: %v", sqlparser.CanonicalString(alterTable)) - } - partitionName := spec.Names[0].String() - // OK then! - - // Now, is this query dropping the first partition in a RANGE partitioned table? - part := createTable.TableSpec.PartitionOption - if part.Type != sqlparser.RangeType { - return nil, nil - } - if len(part.Definitions) == 0 { - return nil, nil - } - var partitionDefinition *sqlparser.PartitionDefinition - var nextPartitionName string - for i, p := range part.Definitions { - if p.Name.String() == partitionName { - partitionDefinition = p - if i+1 < len(part.Definitions) { - nextPartitionName = part.Definitions[i+1].Name.String() - } - break - } - } - if partitionDefinition == nil { - // dropping a nonexistent partition. We'll let the "standard" migration execution flow deal with that. - return nil, nil - } - op := NewSpecialAlterOperation(dropRangePartitionSpecialOperation, alterTable, createTable) - op.SetDetail("partition_name", partitionName) - op.SetDetail("partition_definition", sqlparser.CanonicalString(partitionDefinition)) - op.SetDetail("next_partition_name", nextPartitionName) - return op, nil -} - -// analyzeAddRangePartition sees if the online DDL adds a partition in a range partitioned table -func analyzeAddRangePartition(alterTable *sqlparser.AlterTable, createTable *sqlparser.CreateTable) *SpecialAlterPlan { - // we are looking for a `ALTER TABLE
ADD PARTITION (PARTITION ...)` statement with nothing else - if len(alterTable.AlterOptions) > 0 { - return nil - } - if alterTable.PartitionOption != nil { - return nil - } - spec := alterTable.PartitionSpec - if spec == nil { - return nil - } - if spec.Action != sqlparser.AddAction { - return nil - } - if len(spec.Definitions) != 1 { - return nil - } - partitionDefinition := spec.Definitions[0] - partitionName := partitionDefinition.Name.String() - // OK then! - - // Now, is this query adding a partition in a RANGE partitioned table? - part := createTable.TableSpec.PartitionOption - if part.Type != sqlparser.RangeType { - return nil - } - if len(part.Definitions) == 0 { - return nil - } - op := NewSpecialAlterOperation(addRangePartitionSpecialOperation, alterTable, createTable) - op.SetDetail("partition_name", partitionName) - op.SetDetail("partition_definition", sqlparser.CanonicalString(partitionDefinition)) - return op -} - // analyzeInstantDDL takes declarative CreateTable and AlterTable, as well as a server version, and checks whether it is possible to run the ALTER // using ALGORITHM=INSTANT for that version. func analyzeInstantDDL(alterTable *sqlparser.AlterTable, createTable *sqlparser.CreateTable, capableOf capabilities.CapableOf) (*SpecialAlterPlan, error) { @@ -208,19 +118,28 @@ func (e *Executor) analyzeSpecialAlterPlan(ctx context.Context, onlineDDL *schem } // special plans which support reverts are trivially desired: - // special plans which do not support reverts are flag protected: - if onlineDDL.StrategySetting().IsFastRangeRotationFlag() { - op, err := analyzeDropRangePartition(alterTable, createTable) + // + // - nothing here thus far + // + // special plans that do not support revert, but are always desired over Online DDL, + // hence not flag protected: + { + // Dropping a range partition has to run directly. It is incorrect to run with Online DDL + // because the table copy will make the second-oldest partition "adopt" the rows which + // we really want purged from the oldest partition. + // Adding a range partition _can_ technically run with Online DDL, but it is wasteful + // and pointless. The user fully expects the operation to run immediately and without + // any copy of data. + isRangeRotation, err := schemadiff.AlterTableRotatesRangePartition(createTable, alterTable) if err != nil { return nil, err } - if op != nil { - return op, nil - } - if op := analyzeAddRangePartition(alterTable, createTable); op != nil { + if isRangeRotation { + op := NewSpecialAlterOperation(rangePartitionSpecialOperation, alterTable, createTable) return op, nil } } + // special plans which do not support reverts are flag protected: if onlineDDL.StrategySetting().IsPreferInstantDDL() { op, err := analyzeInstantDDL(alterTable, createTable, capableOf) if err != nil { diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index acbfbd8fd69..42b2a4f827b 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -3161,42 +3161,7 @@ func (e *Executor) executeSpecialAlterDDLActionMigrationIfApplicable(ctx context if _, err := e.executeDirectly(ctx, onlineDDL); err != nil { return false, err } - case dropRangePartitionSpecialOperation: - dropPartition := func() error { - artifactTableName, err := schema.GenerateGCTableName(schema.HoldTableGCState, newGCTableRetainTime()) - if err != nil { - return err - } - if err := e.updateArtifacts(ctx, onlineDDL.UUID, artifactTableName); err != nil { - return err - } - - // Apply CREATE TABLE for artifact table - if _, _, err := e.createDuplicateTableLike(ctx, artifactTableName, onlineDDL, conn); err != nil { - return err - } - // Remove partitioning - parsed := sqlparser.BuildParsedQuery(sqlAlterTableRemovePartitioning, artifactTableName) - if _, err := conn.ExecuteFetch(parsed.Query, 0, false); err != nil { - return err - } - // Exchange with partition - partitionName := specialPlan.Detail("partition_name") - parsed = sqlparser.BuildParsedQuery(sqlAlterTableExchangePartition, onlineDDL.Table, partitionName, artifactTableName) - if _, err := conn.ExecuteFetch(parsed.Query, 0, false); err != nil { - return err - } - // Drop table's partition - parsed = sqlparser.BuildParsedQuery(sqlAlterTableDropPartition, onlineDDL.Table, partitionName) - if _, err := conn.ExecuteFetch(parsed.Query, 0, false); err != nil { - return err - } - return nil - } - if err := dropPartition(); err != nil { - return false, err - } - case addRangePartitionSpecialOperation: + case rangePartitionSpecialOperation: if _, err := e.executeDirectly(ctx, onlineDDL); err != nil { return false, err }