Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: fix data race of Column.GeneratedExpr (#48888) #48923

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion br/pkg/lightning/backend/kv/sql2kv.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func CollectGeneratedColumns(se *Session, meta *model.TableInfo, cols []*table.C
var genCols []GeneratedCol
for i, col := range cols {
if col.GeneratedExpr != nil {
expr, err := expression.RewriteAstExpr(se, col.GeneratedExpr, schema, names, true)
expr, err := expression.RewriteAstExpr(se, col.GeneratedExpr.Internal(), schema, names, true)
if err != nil {
return nil, err
}
Expand Down
5 changes: 3 additions & 2 deletions ddl/ddl_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -4874,7 +4874,8 @@ func processColumnOptions(ctx sessionctx.Context, col *table.Column, options []*
col.GeneratedExprString = sb.String()
col.GeneratedStored = opt.Stored
col.Dependences = make(map[string]struct{})
col.GeneratedExpr = opt.Expr
// Only used by checkModifyGeneratedColumn, there is no need to set a ctor for it.
col.GeneratedExpr = table.NewClonableExprNode(nil, opt.Expr)
for _, colName := range FindColumnNamesInExpr(opt.Expr) {
col.Dependences[colName.Name.L] = struct{}{}
}
Expand Down Expand Up @@ -5441,7 +5442,7 @@ func (d *ddl) RenameColumn(ctx sessionctx.Context, ident ast.Ident, spec *ast.Al
if col.GeneratedExpr == nil {
continue
}
dependedColNames := FindColumnNamesInExpr(col.GeneratedExpr)
dependedColNames := FindColumnNamesInExpr(col.GeneratedExpr.Internal())
for _, name := range dependedColNames {
if name.Name.L == oldColName.L {
if col.Hidden {
Expand Down
2 changes: 1 addition & 1 deletion ddl/generated_column.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ func checkModifyGeneratedColumn(sctx sessionctx.Context, schemaName model.CIStr,

if newCol.IsGenerated() {
// rule 3.
if err := checkIllegalFn4Generated(newCol.Name.L, typeColumn, newCol.GeneratedExpr); err != nil {
if err := checkIllegalFn4Generated(newCol.Name.L, typeColumn, newCol.GeneratedExpr.Internal()); err != nil {
return errors.Trace(err)
}

Expand Down
2 changes: 1 addition & 1 deletion ddl/schematracker/dm_tracker.go
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ func (d SchemaTracker) renameColumn(ctx sessionctx.Context, ident ast.Ident, spe
if col.GeneratedExpr == nil {
continue
}
dependedColNames := ddl.FindColumnNamesInExpr(col.GeneratedExpr)
dependedColNames := ddl.FindColumnNamesInExpr(col.GeneratedExpr.Internal())
for _, name := range dependedColNames {
if name.Name.L == oldColName.L {
if col.Hidden {
Expand Down
4 changes: 2 additions & 2 deletions planner/core/logical_plan_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -4913,7 +4913,7 @@ func (b *PlanBuilder) buildDataSource(ctx context.Context, tn *ast.TableName, as
var err error
originVal := b.allowBuildCastArray
b.allowBuildCastArray = true
expr, _, err = b.rewrite(ctx, columns[i].GeneratedExpr, ds, nil, true)
expr, _, err = b.rewrite(ctx, columns[i].GeneratedExpr.Clone(), ds, nil, true)
b.allowBuildCastArray = originVal
if err != nil {
return nil, err
Expand Down Expand Up @@ -5882,7 +5882,7 @@ func (b *PlanBuilder) buildUpdateLists(ctx context.Context, tableList []*ast.Tab
}
virtualAssignments = append(virtualAssignments, &ast.Assignment{
Column: &ast.ColumnName{Schema: tn.Schema, Table: tn.Name, Name: colInfo.Name},
Expr: tableVal.Cols()[i].GeneratedExpr,
Expr: tableVal.Cols()[i].GeneratedExpr.Clone(),
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion planner/core/planbuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -3751,7 +3751,7 @@ func (b *PlanBuilder) resolveGeneratedColumns(ctx context.Context, columns []*ta

originalVal := b.allowBuildCastArray
b.allowBuildCastArray = true
expr, _, err := b.rewrite(ctx, column.GeneratedExpr, mockPlan, nil, true)
expr, _, err := b.rewrite(ctx, column.GeneratedExpr.Clone(), mockPlan, nil, true)
b.allowBuildCastArray = originalVal
if err != nil {
return igc, err
Expand Down
30 changes: 29 additions & 1 deletion table/column.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,39 @@ import (
type Column struct {
*model.ColumnInfo
// If this column is a generated column, the expression will be stored here.
GeneratedExpr ast.ExprNode
GeneratedExpr *ClonableExprNode
// If this column has default expr value, this expression will be stored here.
DefaultExpr ast.ExprNode
}

// ClonableExprNode is a wrapper for ast.ExprNode.
type ClonableExprNode struct {
ctor func() ast.ExprNode
internal ast.ExprNode
}

// NewClonableExprNode creates a ClonableExprNode.
func NewClonableExprNode(ctor func() ast.ExprNode, internal ast.ExprNode) *ClonableExprNode {
return &ClonableExprNode{
ctor: ctor,
internal: internal,
}
}

// Clone makes a "copy" of internal ast.ExprNode by reconstructing it.
func (n *ClonableExprNode) Clone() ast.ExprNode {
if n.ctor == nil {
return n.internal
}
return n.ctor()
}

// Internal returns the reference of the internal ast.ExprNode.
// Note: only use this method when you are sure that the internal ast.ExprNode is not modified concurrently.
func (n *ClonableExprNode) Internal() ast.ExprNode {
return n.internal
}

// String implements fmt.Stringer interface.
func (c *Column) String() string {
ans := []string{c.Name.O, types.TypeToStr(c.GetType(), c.GetCharset())}
Expand Down
31 changes: 25 additions & 6 deletions table/tables/tables.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/pingcap/tidb/kv"
"github.com/pingcap/tidb/meta"
"github.com/pingcap/tidb/meta/autoid"
"github.com/pingcap/tidb/parser/ast"
"github.com/pingcap/tidb/parser/model"
"github.com/pingcap/tidb/parser/mysql"
"github.com/pingcap/tidb/sessionctx"
Expand Down Expand Up @@ -131,15 +132,21 @@ func TableFromMeta(allocs autoid.Allocators, tblInfo *model.TableInfo) (table.Ta

col := table.ToColumn(colInfo)
if col.IsGenerated() {
expr, err := generatedexpr.ParseExpression(colInfo.GeneratedExprString)
genStr := colInfo.GeneratedExprString
expr, err := buildGeneratedExpr(tblInfo, genStr)
if err != nil {
return nil, err
}
expr, err = generatedexpr.SimpleResolveName(expr, tblInfo)
if err != nil {
return nil, err
}
col.GeneratedExpr = expr
col.GeneratedExpr = table.NewClonableExprNode(func() ast.ExprNode {
newExpr, err1 := buildGeneratedExpr(tblInfo, genStr)
if err1 != nil {
logutil.BgLogger().Warn("unexpected parse generated string error",
zap.String("generatedStr", genStr),
zap.Error(err1))
return expr
}
return newExpr
}, expr)
}
// default value is expr.
if col.DefaultIsExpr {
Expand All @@ -166,6 +173,18 @@ func TableFromMeta(allocs autoid.Allocators, tblInfo *model.TableInfo) (table.Ta
return newPartitionedTable(&t, tblInfo)
}

func buildGeneratedExpr(tblInfo *model.TableInfo, genExpr string) (ast.ExprNode, error) {
expr, err := generatedexpr.ParseExpression(genExpr)
if err != nil {
return nil, err
}
expr, err = generatedexpr.SimpleResolveName(expr, tblInfo)
if err != nil {
return nil, err
}
return expr, nil
}

// initTableCommon initializes a TableCommon struct.
func initTableCommon(t *TableCommon, tblInfo *model.TableInfo, physicalTableID int64, cols []*table.Column, allocs autoid.Allocators) {
t.tableID = tblInfo.ID
Expand Down