Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -2042,6 +2042,25 @@ func testScheduler(t *testing.T) {
})
testTableCompletionTimes(t, t1uuid, v1uuid)
})
t.Run("cancel migrations by context", func(t *testing.T) {
// Submit two migrations with the same explicit context, both postponed so they stay running.
t1uuid = testOnlineDDLStatement(t, &testOnlineDDLStatementParams{ddlStatement: trivialAlterT1Statement, ddlStrategy: ddlStrategy + " --allow-concurrent --postpone-completion", executeStrategy: "vtctl", migrationContext: "ctx-cancel-by-context", skipWait: true})
t2uuid = testOnlineDDLStatement(t, &testOnlineDDLStatementParams{ddlStatement: trivialAlterT2Statement, ddlStrategy: ddlStrategy + " --allow-concurrent --postpone-completion", executeStrategy: "vtctl", migrationContext: "ctx-cancel-by-context", skipWait: true})
onlineddl.WaitForMigrationStatus(t, &vtParams, shards, t1uuid, normalWaitTime, schema.OnlineDDLStatusRunning)
onlineddl.WaitForMigrationStatus(t, &vtParams, shards, t2uuid, normalWaitTime, schema.OnlineDDLStatusRunning)

// A non-matching context cancels nothing.
onlineddl.CheckCancelContextMigrations(t, &vtParams, "ctx-cancel-by-context-other", 0)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, t1uuid, schema.OnlineDDLStatusRunning)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, t2uuid, schema.OnlineDDLStatusRunning)

// Cancel by context: both migrations must be cancelled.
onlineddl.CheckCancelContextMigrations(t, &vtParams, "ctx-cancel-by-context", 2)
onlineddl.WaitForMigrationStatus(t, &vtParams, shards, t1uuid, normalWaitTime, schema.OnlineDDLStatusCancelled, schema.OnlineDDLStatusFailed)
onlineddl.WaitForMigrationStatus(t, &vtParams, shards, t2uuid, normalWaitTime, schema.OnlineDDLStatusCancelled, schema.OnlineDDLStatusFailed)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, t1uuid, schema.OnlineDDLStatusCancelled)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, t2uuid, schema.OnlineDDLStatusCancelled)
})
}

func testSingleton(t *testing.T) {
Expand Down
11 changes: 11 additions & 0 deletions go/test/endtoend/onlineddl/vtgate_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,17 @@ func CheckCancelAllMigrations(t *testing.T, vtParams *mysql.ConnParams, expectCo
}
}

// CheckCancelContextMigrations cancels all pending migrations with a given context and expect number of affected rows
// A negative value for expectCount indicates "don't care, no need to check"
func CheckCancelContextMigrations(t *testing.T, vtParams *mysql.ConnParams, migrationContext string, expectCount int) {
cancelQuery := fmt.Sprintf("alter vitess_migration cancel context '%s'", migrationContext)
Comment thread
shlomi-noach marked this conversation as resolved.
r := VtgateExecQuery(t, vtParams, cancelQuery, "")

if expectCount >= 0 {
assert.Equal(t, expectCount, int(r.RowsAffected))
}
}

// CheckCleanupAllMigrations cleans up all applicable migrations and expect number of affected rows
// A negative value for expectCount indicates "don't care, no need to check"
func CheckCleanupAllMigrations(t *testing.T, vtParams *mysql.ConnParams, expectCount int) uint64 {
Expand Down
1 change: 1 addition & 0 deletions go/vt/sqlparser/ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,7 @@ type (
AlterMigration struct {
Type AlterMigrationType
UUID string
Context string
Expire string
Ratio *Literal
Threshold string
Expand Down
1 change: 1 addition & 0 deletions go/vt/sqlparser/ast_equals.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 11 additions & 4 deletions go/vt/sqlparser/ast_format.go
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,11 @@ func (node *AlterMigration) Format(buf *TrackedBuffer) {
case CancelMigrationType:
alterType = "cancel"
case CancelAllMigrationType:
alterType = "cancel all"
if node.Context != "" {
alterType = "cancel"
} else {
alterType = "cancel all"
}
case ThrottleMigrationType:
alterType = "throttle"
case ThrottleAllMigrationType:
Expand All @@ -355,16 +359,19 @@ func (node *AlterMigration) Format(buf *TrackedBuffer) {
}
buf.astPrintf(node, " %#s", alterType)
if node.Threshold != "" {
buf.astPrintf(node, " '%#s'", node.Threshold)
buf.astPrintf(node, " %s", encodeSQLString(node.Threshold))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these remain as %#s to maintain casing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my understanding the # directive is only applicable to keyword/identifier nodes. It will be completely harmless to change to %#s and I'm happy to do that -- at the same time it will be ineffective and possibly misleading since there is no keyword/identifier involved. I have absolutely zero preferences. What would make more sense in your opinion?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem to apply to literals as well, here's an example unit test: 300c07d

❯ go test ./go/vt/sqlparser -run TestCanonicalOutput/alter_vitess_migration_cancel_context -count 1
--- FAIL: TestCanonicalOutput (0.00s)
    --- FAIL: TestCanonicalOutput/alter_vitess_migration_cancel_context_'ProdCtx' (0.00s)
        tracked_buffer_test.go:292:
            	Error Trace:	/Users/mhamza/.local/share/forest/worktrees/vitess/planetscale-onlineddl-cancel-migration-context/go/vt/sqlparser/tracked_buffer_test.go:292
            	Error:      	Not equal:
            	            	expected: "ALTER VITESS_MIGRATION cancel CONTEXT 'ProdCtx'"
            	            	actual  : "ALTER VITESS_MIGRATION cancel CONTEXT 'PRODCTX'"
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1 +1 @@
            	            	-ALTER VITESS_MIGRATION cancel CONTEXT 'ProdCtx'
            	            	+ALTER VITESS_MIGRATION cancel CONTEXT 'PRODCTX'
            	Test:       	TestCanonicalOutput/alter_vitess_migration_cancel_context_'ProdCtx'
            	Messages:   	bad serialization
FAIL
FAIL	vitess.io/vitess/go/vt/sqlparser	0.394s
FAIL

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and pushed! Interestingly ast_format_fast.go was unaffected by the change.

}
if node.Expire != "" {
buf.astPrintf(node, " expire '%#s'", node.Expire)
buf.astPrintf(node, " expire %s", encodeSQLString(node.Expire))
}
if node.Ratio != nil {
buf.astPrintf(node, " ratio %v", node.Ratio)
}
if node.Context != "" {
buf.astPrintf(node, " context %s", encodeSQLString(node.Context))
}
Comment thread
shlomi-noach marked this conversation as resolved.
if node.Shards != "" {
buf.astPrintf(node, " vitess_shards '%#s'", node.Shards)
buf.astPrintf(node, " vitess_shards %s", encodeSQLString(node.Shards))
}
}

Expand Down
25 changes: 15 additions & 10 deletions go/vt/sqlparser/ast_format_fast.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion go/vt/sqlparser/cached_size.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions go/vt/sqlparser/keywords.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ var keywords = []keyword{
{"constraint_catalog", CONSTRAINT_CATALOG},
{"constraint_name", CONSTRAINT_NAME},
{"constraint_schema", CONSTRAINT_SCHEMA},
{"context", CONTEXT},
{"continue", CONTINUE},
{"convert", CONVERT},
{"copy", COPY},
Expand Down
5 changes: 5 additions & 0 deletions go/vt/sqlparser/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2689,6 +2689,8 @@ var validSQL = []struct {
}, {
input: "alter vitess_migration '9748c3b7_7fdb_11eb_ac2c_f875a4d24e90' FORCE_CUTOVER",
output: "alter vitess_migration '9748c3b7_7fdb_11eb_ac2c_f875a4d24e90' force_cutover",
}, {
input: "alter vitess_migration cancel context 'some-context'",
}, {
input: "alter vitess_migration cancel all",
}, {
Expand Down Expand Up @@ -6488,6 +6490,9 @@ var invalidSQL = []struct {
input string
output string
}{{
input: "alter vitess_migration cancel context ''",
output: "migration context cannot be empty at position 41",
}, {
input: "select : from t",
output: "syntax error at position 9 near ':'",
}, {
Expand Down
Loading
Loading