diff --git a/go/vt/schema/online_ddl.go b/go/vt/schema/online_ddl.go index 07740039004..aff15a14f89 100644 --- a/go/vt/schema/online_ddl.go +++ b/go/vt/schema/online_ddl.go @@ -269,6 +269,11 @@ func OnlineDDLFromCommentedStatement(stmt sqlparser.Statement) (onlineDDL *Onlin default: return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported statement for Online DDL: %v", sqlparser.String(stmt)) } + // We clone the comments because they will end up being cached by the query planner. Then, the Directive() function actually modifies the comments. + // If comments are shared in cache, and Directive() modifies it, then we have a concurrency issue when someone else wants to read the comments. + // By cloning the comments we remove the concurrency problem. + comments = sqlparser.CloneRefOfParsedComments(comments) + comments.ResetDirectives() if comments.Length() == 0 { return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "no comments found in statement: %v", sqlparser.String(stmt)) diff --git a/go/vt/sqlparser/ast_test.go b/go/vt/sqlparser/ast_test.go index 3c518ee7886..0d6841755c8 100644 --- a/go/vt/sqlparser/ast_test.go +++ b/go/vt/sqlparser/ast_test.go @@ -821,3 +821,32 @@ func BenchmarkStringTraces(b *testing.B) { }) } } + +func TestCloneComments(t *testing.T) { + c := []string{"/*vt+ a=b */"} + parsedComments := Comments(c).Parsed() + directives := parsedComments.Directives() + { + assert.NotEmpty(t, directives.m) + val, ok := directives.m["a"] + assert.Truef(t, ok, "directives map: %v", directives.m) + assert.Equal(t, "b", val) + } + cloned := CloneRefOfParsedComments(parsedComments) + cloned.ResetDirectives() + clonedDirectives := cloned.Directives() + { + assert.NotEmpty(t, clonedDirectives.m) + val, ok := clonedDirectives.m["a"] + assert.Truef(t, ok, "directives map: %v", directives.m) + assert.Equal(t, "b", val) + } + { + delete(directives.m, "a") + assert.Empty(t, directives.m) + + val, ok := clonedDirectives.m["a"] + assert.Truef(t, ok, "directives map: %v", directives.m) + assert.Equal(t, "b", val) + } +} diff --git a/go/vt/sqlparser/comments.go b/go/vt/sqlparser/comments.go index 528d0e250bd..0faebe09f4b 100644 --- a/go/vt/sqlparser/comments.go +++ b/go/vt/sqlparser/comments.go @@ -206,6 +206,15 @@ type CommentDirectives struct { m map[string]string } +// ResetDirectives sets the _directives member to `nil`, which means the next call to Directives() +// will re-evaluate it. +func (c *ParsedComments) ResetDirectives() { + if c == nil { + return + } + c._directives = nil +} + // Directives parses the comment list for any execution directives // of the form: // diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 2e4cb822846..7cdb082b847 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2726,7 +2726,7 @@ func (e *Executor) executeAlterViewOnline(ctx context.Context, onlineDDL *schema Select: viewStmt.Select, CheckOption: viewStmt.CheckOption, IsReplace: true, - Comments: viewStmt.Comments, + Comments: sqlparser.CloneRefOfParsedComments(viewStmt.Comments), } stmt.SetTable("", artifactViewName) default: