From 1485b956a5711f37528497abff9350eb5d4f062b Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 3 Aug 2023 16:00:22 +0300 Subject: [PATCH 1/3] Solve RevertMigration.Comment read/write concurrency issue Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schema/online_ddl.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/go/vt/schema/online_ddl.go b/go/vt/schema/online_ddl.go index 889caf811bc..66dfa0b4b24 100644 --- a/go/vt/schema/online_ddl.go +++ b/go/vt/schema/online_ddl.go @@ -269,7 +269,10 @@ func OnlineDDLFromCommentedStatement(stmt sqlparser.Statement) (onlineDDL *Onlin case sqlparser.DDLStatement: comments = stmt.GetParsedComments() case *sqlparser.RevertMigration: - comments = stmt.Comments + // 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(stmt.Comments) default: return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported statement for Online DDL: %v", sqlparser.String(stmt)) } From 7a75a5d8ca3cdebca5fcbbd6e6f1b5cbd63b3e41 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 6 Aug 2023 13:20:20 +0300 Subject: [PATCH 2/3] ResetDirectives Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schema/online_ddl.go | 10 ++++++---- go/vt/sqlparser/ast_test.go | 29 ++++++++++++++++++++++++++++ go/vt/sqlparser/comments.go | 6 ++++++ go/vt/vttablet/onlineddl/executor.go | 2 +- 4 files changed, 42 insertions(+), 5 deletions(-) diff --git a/go/vt/schema/online_ddl.go b/go/vt/schema/online_ddl.go index 66dfa0b4b24..71f07add0c2 100644 --- a/go/vt/schema/online_ddl.go +++ b/go/vt/schema/online_ddl.go @@ -269,13 +269,15 @@ func OnlineDDLFromCommentedStatement(stmt sqlparser.Statement) (onlineDDL *Onlin case sqlparser.DDLStatement: comments = stmt.GetParsedComments() case *sqlparser.RevertMigration: - // 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(stmt.Comments) + comments = stmt.Comments 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 a4254ad7989..3f0fb850857 100644 --- a/go/vt/sqlparser/ast_test.go +++ b/go/vt/sqlparser/ast_test.go @@ -818,3 +818,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 ed29e3fe3be..77743a8fb9a 100644 --- a/go/vt/sqlparser/comments.go +++ b/go/vt/sqlparser/comments.go @@ -224,6 +224,12 @@ 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() { + 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 244aa8ce8b2..d84a8e7e15e 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2850,7 +2850,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: From bf5af7993d63ffa6bb3ec875dd44cc82559aaff1 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Sun, 6 Aug 2023 13:32:48 +0300 Subject: [PATCH 3/3] check nil pointer Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/sqlparser/comments.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/go/vt/sqlparser/comments.go b/go/vt/sqlparser/comments.go index 77743a8fb9a..911498d8242 100644 --- a/go/vt/sqlparser/comments.go +++ b/go/vt/sqlparser/comments.go @@ -227,6 +227,9 @@ type CommentDirectives struct { // 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 }