Solve RevertMigration.Comment read/write concurrency issue (vitessio#…

…13700)

Signed-off-by: Shlomi Noach <[email protected]>

go/vt/schema/online_ddl.go

@@ -274,6 +274,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))

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)
+	}
+}

go/vt/sqlparser/comments.go

@@ -210,6 +210,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:
 //

go/vt/vttablet/onlineddl/executor.go

@@ -2844,7 +2844,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: