Skip to content

Commit 1ec94fb

Browse files
authored
*: fix data race of Column.GeneratedExpr (#48888) (#48924)
close #44919, close #48191
1 parent b31b3e5 commit 1ec94fb

File tree

8 files changed

+63
-15
lines changed

8 files changed

+63
-15
lines changed

br/pkg/lightning/backend/kv/sql2kv.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ func CollectGeneratedColumns(se *Session, meta *model.TableInfo, cols []*table.C
115115
var genCols []GeneratedCol
116116
for i, col := range cols {
117117
if col.GeneratedExpr != nil {
118-
expr, err := expression.RewriteAstExpr(se, col.GeneratedExpr, schema, names, true)
118+
expr, err := expression.RewriteAstExpr(se, col.GeneratedExpr.Internal(), schema, names, true)
119119
if err != nil {
120120
return nil, err
121121
}

pkg/ddl/ddl_api.go

+3-2
Original file line numberDiff line numberDiff line change
@@ -5349,7 +5349,8 @@ func ProcessColumnOptions(ctx sessionctx.Context, col *table.Column, options []*
53495349
col.GeneratedExprString = sb.String()
53505350
col.GeneratedStored = opt.Stored
53515351
col.Dependences = make(map[string]struct{})
5352-
col.GeneratedExpr = opt.Expr
5352+
// Only used by checkModifyGeneratedColumn, there is no need to set a ctor for it.
5353+
col.GeneratedExpr = table.NewClonableExprNode(nil, opt.Expr)
53535354
for _, colName := range FindColumnNamesInExpr(opt.Expr) {
53545355
col.Dependences[colName.Name.L] = struct{}{}
53555356
}
@@ -5415,7 +5416,7 @@ func checkModifyColumnWithGeneratedColumnsConstraint(allCols []*table.Column, ol
54155416
if col.GeneratedExpr == nil {
54165417
continue
54175418
}
5418-
dependedColNames := FindColumnNamesInExpr(col.GeneratedExpr)
5419+
dependedColNames := FindColumnNamesInExpr(col.GeneratedExpr.Internal())
54195420
for _, name := range dependedColNames {
54205421
if name.Name.L == oldColName.L {
54215422
if col.Hidden {

pkg/ddl/generated_column.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ func checkModifyGeneratedColumn(sctx sessionctx.Context, schemaName model.CIStr,
252252

253253
if newCol.IsGenerated() {
254254
// rule 3.
255-
if err := checkIllegalFn4Generated(newCol.Name.L, typeColumn, newCol.GeneratedExpr); err != nil {
255+
if err := checkIllegalFn4Generated(newCol.Name.L, typeColumn, newCol.GeneratedExpr.Internal()); err != nil {
256256
return errors.Trace(err)
257257
}
258258

pkg/ddl/schematracker/dm_tracker.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -613,7 +613,7 @@ func (d SchemaTracker) renameColumn(_ sessionctx.Context, ident ast.Ident, spec
613613
if col.GeneratedExpr == nil {
614614
continue
615615
}
616-
dependedColNames := ddl.FindColumnNamesInExpr(col.GeneratedExpr)
616+
dependedColNames := ddl.FindColumnNamesInExpr(col.GeneratedExpr.Internal())
617617
for _, name := range dependedColNames {
618618
if name.Name.L == oldColName.L {
619619
if col.Hidden {

pkg/planner/core/logical_plan_builder.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -5358,7 +5358,7 @@ func (b *PlanBuilder) buildDataSource(ctx context.Context, tn *ast.TableName, as
53585358
var err error
53595359
originVal := b.allowBuildCastArray
53605360
b.allowBuildCastArray = true
5361-
expr, _, err = b.rewrite(ctx, columns[i].GeneratedExpr, ds, nil, true)
5361+
expr, _, err = b.rewrite(ctx, columns[i].GeneratedExpr.Clone(), ds, nil, true)
53625362
b.allowBuildCastArray = originVal
53635363
if err != nil {
53645364
return nil, err
@@ -6327,7 +6327,7 @@ func (b *PlanBuilder) buildUpdateLists(ctx context.Context, tableList []*ast.Tab
63276327
}
63286328
virtualAssignments = append(virtualAssignments, &ast.Assignment{
63296329
Column: &ast.ColumnName{Schema: tn.Schema, Table: tn.Name, Name: colInfo.Name},
6330-
Expr: tableVal.Cols()[i].GeneratedExpr,
6330+
Expr: tableVal.Cols()[i].GeneratedExpr.Clone(),
63316331
})
63326332
}
63336333
}

pkg/planner/core/planbuilder.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -3889,7 +3889,7 @@ func (b *PlanBuilder) resolveGeneratedColumns(ctx context.Context, columns []*ta
38893889

38903890
originalVal := b.allowBuildCastArray
38913891
b.allowBuildCastArray = true
3892-
expr, _, err := b.rewrite(ctx, column.GeneratedExpr, mockPlan, nil, true)
3892+
expr, _, err := b.rewrite(ctx, column.GeneratedExpr.Clone(), mockPlan, nil, true)
38933893
b.allowBuildCastArray = originalVal
38943894
if err != nil {
38953895
return igc, err

pkg/table/column.go

+29-1
Original file line numberDiff line numberDiff line change
@@ -46,11 +46,39 @@ import (
4646
type Column struct {
4747
*model.ColumnInfo
4848
// If this column is a generated column, the expression will be stored here.
49-
GeneratedExpr ast.ExprNode
49+
GeneratedExpr *ClonableExprNode
5050
// If this column has default expr value, this expression will be stored here.
5151
DefaultExpr ast.ExprNode
5252
}
5353

54+
// ClonableExprNode is a wrapper for ast.ExprNode.
55+
type ClonableExprNode struct {
56+
ctor func() ast.ExprNode
57+
internal ast.ExprNode
58+
}
59+
60+
// NewClonableExprNode creates a ClonableExprNode.
61+
func NewClonableExprNode(ctor func() ast.ExprNode, internal ast.ExprNode) *ClonableExprNode {
62+
return &ClonableExprNode{
63+
ctor: ctor,
64+
internal: internal,
65+
}
66+
}
67+
68+
// Clone makes a "copy" of internal ast.ExprNode by reconstructing it.
69+
func (n *ClonableExprNode) Clone() ast.ExprNode {
70+
if n.ctor == nil {
71+
return n.internal
72+
}
73+
return n.ctor()
74+
}
75+
76+
// Internal returns the reference of the internal ast.ExprNode.
77+
// Note: only use this method when you are sure that the internal ast.ExprNode is not modified concurrently.
78+
func (n *ClonableExprNode) Internal() ast.ExprNode {
79+
return n.internal
80+
}
81+
5482
// String implements fmt.Stringer interface.
5583
func (c *Column) String() string {
5684
ans := []string{c.Name.O, types.TypeToStr(c.GetType(), c.GetCharset())}

pkg/table/tables/tables.go

+25-6
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333
"github.com/pingcap/tidb/pkg/kv"
3434
"github.com/pingcap/tidb/pkg/meta"
3535
"github.com/pingcap/tidb/pkg/meta/autoid"
36+
"github.com/pingcap/tidb/pkg/parser/ast"
3637
"github.com/pingcap/tidb/pkg/parser/model"
3738
"github.com/pingcap/tidb/pkg/parser/mysql"
3839
"github.com/pingcap/tidb/pkg/sessionctx"
@@ -138,15 +139,21 @@ func TableFromMeta(allocs autoid.Allocators, tblInfo *model.TableInfo) (table.Ta
138139

139140
col := table.ToColumn(colInfo)
140141
if col.IsGenerated() {
141-
expr, err := generatedexpr.ParseExpression(colInfo.GeneratedExprString)
142+
genStr := colInfo.GeneratedExprString
143+
expr, err := buildGeneratedExpr(tblInfo, genStr)
142144
if err != nil {
143145
return nil, err
144146
}
145-
expr, err = generatedexpr.SimpleResolveName(expr, tblInfo)
146-
if err != nil {
147-
return nil, err
148-
}
149-
col.GeneratedExpr = expr
147+
col.GeneratedExpr = table.NewClonableExprNode(func() ast.ExprNode {
148+
newExpr, err1 := buildGeneratedExpr(tblInfo, genStr)
149+
if err1 != nil {
150+
logutil.BgLogger().Warn("unexpected parse generated string error",
151+
zap.String("generatedStr", genStr),
152+
zap.Error(err1))
153+
return expr
154+
}
155+
return newExpr
156+
}, expr)
150157
}
151158
// default value is expr.
152159
if col.DefaultIsExpr {
@@ -177,6 +184,18 @@ func TableFromMeta(allocs autoid.Allocators, tblInfo *model.TableInfo) (table.Ta
177184
return newPartitionedTable(&t, tblInfo)
178185
}
179186

187+
func buildGeneratedExpr(tblInfo *model.TableInfo, genExpr string) (ast.ExprNode, error) {
188+
expr, err := generatedexpr.ParseExpression(genExpr)
189+
if err != nil {
190+
return nil, err
191+
}
192+
expr, err = generatedexpr.SimpleResolveName(expr, tblInfo)
193+
if err != nil {
194+
return nil, err
195+
}
196+
return expr, nil
197+
}
198+
180199
// initTableCommon initializes a TableCommon struct.
181200
func initTableCommon(t *TableCommon, tblInfo *model.TableInfo, physicalTableID int64, cols []*table.Column, allocs autoid.Allocators, constraints []*table.Constraint) {
182201
t.tableID = tblInfo.ID

0 commit comments

Comments
 (0)