plan: fix a bug when alter table drop column meets insert #5790
Conversation
|
Could you add a test case? |
|
/run-all-tests |
jackysp
changed the title
|
Could you add the original schrodinger test case to unit test? |
plan/planbuilder.go
Outdated
| @@ -878,8 +890,8 @@ func (b *planBuilder) buildInsert(insert *ast.InsertStmt) Plan { | |||
| return nil | |||
| } | |||
| // Check set list contains generated column or not. | |||
| if columnByName[assign.Column.Name.L].IsGenerated() { | |||
There was a problem hiding this comment.
If it does panic here, because the assign.Column.Name.L is not in columnByName, I think, we can just skip it here( check whether the value is null is ok). No need to add checkGeneratedColumn function to iterator the columns array every time.
There was a problem hiding this comment.
I don't think it is a good solution to skip it directly. We should not use columnByName to avoid potential bugs. @winkyao
There was a problem hiding this comment.
This is not potential bugs, actually it is expected that there is some inserting column not in columnByName. We should not reduce the performance here. @jackysp
There was a problem hiding this comment.
line 796 check the situation, but here not, so it panic.
plan/planbuilder.go
Outdated
| if column.IsGenerated() { | ||
| b.err = ErrBadGeneratedColumn.GenByArgs(col.Name.O, tableInfo.Name.O) | ||
| if err := checkGeneratedColumn(col, insertPlan.Table); err != nil { | ||
| if terror.ErrorEqual(err, ErrBadGeneratedColumn) { |
There was a problem hiding this comment.
Why not return Can't find column... error?
|
@jackysp |
| @@ -285,6 +285,9 @@ func TableInfo2SchemaWithDBName(dbName model.CIStr, tbl *model.TableInfo) *Schem | |||
| func ColumnInfos2ColumnsWithDBName(dbName, tblName model.CIStr, colInfos []*model.ColumnInfo) []*Column { | |||
| columns := make([]*Column, 0, len(colInfos)) | |||
| for i, col := range colInfos { | |||
| if col.State != model.StatePublic { | |||
There was a problem hiding this comment.
what about write only column? @zimulala PTAL
There was a problem hiding this comment.
If the column is given by the client, we need the Public column.
|
No pause support in gofail @winkyao , I've add a concurrency test, it will panic without the bug fix. |
ddl/ddl_db_test.go
Outdated
| @@ -988,6 +995,28 @@ LOOP: | |||
| c.Assert(count, Greater, int64(0)) | |||
| } | |||
|
|
|||
| func (s *testDBSuite) testDropColumn2(c *C) { | |||
There was a problem hiding this comment.
Please add a comment for this test.
|
/run-all-tests |
|
PTAL @zimulala |
session 1:
session 2:
could panic, due to TiDB save the column name as a map in
buildInsert. PTAL @zimulala @hicqu