From 5f68180c11fae24686f33fc5cbcf57d2d0960aa6 Mon Sep 17 00:00:00 2001 From: Ewan Chou Date: Tue, 10 Apr 2018 01:09:31 +0800 Subject: [PATCH 1/4] *: fix update write-only not null column with null value. GetColDefaultValue returns null if the column is not public which cause writing null value into the not null column. --- ddl/ddl_db_test.go | 30 ++++++++++++++++++++++++++++++ executor/write.go | 2 +- table/tables/tables.go | 3 --- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/ddl/ddl_db_test.go b/ddl/ddl_db_test.go index 418851c248a6a..c55ee054a3fbd 100644 --- a/ddl/ddl_db_test.go +++ b/ddl/ddl_db_test.go @@ -20,6 +20,7 @@ import ( "math/rand" "strconv" "strings" + "sync" "time" "github.com/juju/errors" @@ -1811,3 +1812,32 @@ func (s *testDBSuite) TestCharacterSetInColumns(c *C) { s.tk.MustQuery("select count(*) from information_schema.columns where table_schema = 'varchar_test' and character_set_name != 'utf8'").Check(testkit.Rows("0")) s.tk.MustQuery("select count(*) from information_schema.columns where table_schema = 'varchar_test' and character_set_name = 'utf8'").Check(testkit.Rows("2")) } + +func (s *testDBSuite) TestAddNotNullColumnWhileInsertOnDupUpdate(c *C) { + tk1 := testkit.NewTestKitWithInit(c, s.store) + tk2 := testkit.NewTestKitWithInit(c, s.store) + closeCh := make(chan bool) + wg := new(sync.WaitGroup) + wg.Add(1) + tk1.MustExec("create table nn (a int primary key, b int)") + tk1.MustExec("insert nn values (1, 1)") + var tk2Err error + go func() { + defer wg.Done() + for { + select { + case <-closeCh: + return + default: + } + _, tk2Err = tk2.Exec("insert nn (a, b) values (1, 1) on duplicate key update a = 1, b = b + 1") + if tk2Err != nil { + return + } + } + }() + tk1.MustExec("alter table nn add column c int not null default 0") + close(closeCh) + wg.Wait() + c.Assert(tk2Err, IsNil) +} diff --git a/executor/write.go b/executor/write.go index 8b6faa1cc9651..d8e64466bebdf 100644 --- a/executor/write.go +++ b/executor/write.go @@ -921,7 +921,7 @@ func batchGetOldValues(ctx sessionctx.Context, t table.Table, handles []int64) ( func encodeNewRow(ctx sessionctx.Context, t table.Table, row []types.Datum) ([]byte, error) { colIDs := make([]int64, 0, len(row)) skimmedRow := make([]types.Datum, 0, len(row)) - for _, col := range t.WritableCols() { + for _, col := range t.Cols() { if !tables.CanSkip(t.Meta(), col, row[col.Offset]) { colIDs = append(colIDs, col.ID) skimmedRow = append(skimmedRow, row[col.Offset]) diff --git a/table/tables/tables.go b/table/tables/tables.go index c0a90b1b38b5f..2ea847cb80bc2 100644 --- a/table/tables/tables.go +++ b/table/tables/tables.go @@ -760,9 +760,6 @@ func GetColDefaultValue(ctx sessionctx.Context, col *table.Column, defaultVals [ if col.OriginDefaultValue == nil && mysql.HasNotNullFlag(col.Flag) { return colVal, errors.New("Miss column") } - if col.State != model.StatePublic { - return colVal, nil - } if defaultVals[col.Offset].IsNull() { colVal, err = table.GetColOriginDefaultValue(ctx, col.ToInfo()) if err != nil { From 87afd18b30dcdcfd5b27e6b33de1a8870ab1856c Mon Sep 17 00:00:00 2001 From: Ewan Chou Date: Tue, 10 Apr 2018 01:50:15 +0800 Subject: [PATCH 2/4] *: fix CI --- ddl/ddl_db_test.go | 6 ++++-- executor/write.go | 12 +++++++++++- table/tables/tables.go | 3 +++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/ddl/ddl_db_test.go b/ddl/ddl_db_test.go index c55ee054a3fbd..9daad6bb86c89 100644 --- a/ddl/ddl_db_test.go +++ b/ddl/ddl_db_test.go @@ -1814,8 +1814,10 @@ func (s *testDBSuite) TestCharacterSetInColumns(c *C) { } func (s *testDBSuite) TestAddNotNullColumnWhileInsertOnDupUpdate(c *C) { - tk1 := testkit.NewTestKitWithInit(c, s.store) - tk2 := testkit.NewTestKitWithInit(c, s.store) + tk1 := testkit.NewTestKit(c, s.store) + tk1.MustExec("use " + s.schemaName) + tk2 := testkit.NewTestKit(c, s.store) + tk2.MustExec("use " + s.schemaName) closeCh := make(chan bool) wg := new(sync.WaitGroup) wg.Add(1) diff --git a/executor/write.go b/executor/write.go index d8e64466bebdf..c32bdbf5ae5e8 100644 --- a/executor/write.go +++ b/executor/write.go @@ -1107,10 +1107,20 @@ func (e *InsertExec) updateDupRow(keys []keyWithDupError, k keyWithDupError, val if !ok { return errors.NotFoundf("can not be duplicated row, due to old row not found. handle %d", oldHandle) } - oldRow, err := tables.DecodeRawRowData(e.ctx, e.Table.Meta(), oldHandle, e.Table.WritableCols(), oldValue) + cols := e.Table.WritableCols() + oldRow, err := tables.DecodeRawRowData(e.ctx, e.Table.Meta(), oldHandle, cols, oldValue) if err != nil { return errors.Trace(err) } + // Fill write-only columns with originDefaultValue if value is null. + for _, col := range cols { + if col.State != model.StatePublic && oldRow[col.Offset].IsNull() { + oldRow[col.Offset], err = table.GetColOriginDefaultValue(e.ctx, col.ToInfo()) + if err != nil { + return errors.Trace(err) + } + } + } // Do update row. updatedRow, handleChanged, newHandle, err := e.doDupRowUpdate(oldHandle, oldRow, newRow, onDuplicate) diff --git a/table/tables/tables.go b/table/tables/tables.go index 2ea847cb80bc2..c0a90b1b38b5f 100644 --- a/table/tables/tables.go +++ b/table/tables/tables.go @@ -760,6 +760,9 @@ func GetColDefaultValue(ctx sessionctx.Context, col *table.Column, defaultVals [ if col.OriginDefaultValue == nil && mysql.HasNotNullFlag(col.Flag) { return colVal, errors.New("Miss column") } + if col.State != model.StatePublic { + return colVal, nil + } if defaultVals[col.Offset].IsNull() { colVal, err = table.GetColOriginDefaultValue(ctx, col.ToInfo()) if err != nil { From 68b4eedf4d4c5b8fb99669e19e72bdc7e8112355 Mon Sep 17 00:00:00 2001 From: Ewan Chou Date: Tue, 10 Apr 2018 02:35:13 +0800 Subject: [PATCH 3/4] *: cover a corner case --- executor/write.go | 13 ++++++++----- table/tables/tables.go | 11 +++++------ 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/executor/write.go b/executor/write.go index c32bdbf5ae5e8..52e89829a0ada 100644 --- a/executor/write.go +++ b/executor/write.go @@ -1108,16 +1108,19 @@ func (e *InsertExec) updateDupRow(keys []keyWithDupError, k keyWithDupError, val return errors.NotFoundf("can not be duplicated row, due to old row not found. handle %d", oldHandle) } cols := e.Table.WritableCols() - oldRow, err := tables.DecodeRawRowData(e.ctx, e.Table.Meta(), oldHandle, cols, oldValue) + oldRow, oldRowMap, err := tables.DecodeRawRowData(e.ctx, e.Table.Meta(), oldHandle, cols, oldValue) if err != nil { return errors.Trace(err) } - // Fill write-only columns with originDefaultValue if value is null. + // Fill write-only columns with originDefaultValue if not found in oldValue is null. for _, col := range cols { if col.State != model.StatePublic && oldRow[col.Offset].IsNull() { - oldRow[col.Offset], err = table.GetColOriginDefaultValue(e.ctx, col.ToInfo()) - if err != nil { - return errors.Trace(err) + _, found := oldRowMap[col.ID] + if !found { + oldRow[col.Offset], err = table.GetColOriginDefaultValue(e.ctx, col.ToInfo()) + if err != nil { + return errors.Trace(err) + } } } } diff --git a/table/tables/tables.go b/table/tables/tables.go index c0a90b1b38b5f..5b26d345ca923 100644 --- a/table/tables/tables.go +++ b/table/tables/tables.go @@ -506,7 +506,7 @@ func (t *Table) RowWithCols(ctx sessionctx.Context, h int64, cols []*table.Colum if err != nil { return nil, errors.Trace(err) } - v, err := DecodeRawRowData(ctx, t.Meta(), h, cols, value) + v, _, err := DecodeRawRowData(ctx, t.Meta(), h, cols, value) if err != nil { return nil, errors.Trace(err) } @@ -515,8 +515,7 @@ func (t *Table) RowWithCols(ctx sessionctx.Context, h int64, cols []*table.Colum // DecodeRawRowData decodes raw row data to a datum row. func DecodeRawRowData(ctx sessionctx.Context, meta *model.TableInfo, h int64, cols []*table.Column, - value []byte) ([]types.Datum, - error) { + value []byte) ([]types.Datum, map[int64]types.Datum, error) { v := make([]types.Datum, len(cols)) colTps := make(map[int64]*types.FieldType, len(cols)) for i, col := range cols { @@ -535,7 +534,7 @@ func DecodeRawRowData(ctx sessionctx.Context, meta *model.TableInfo, h int64, co } rowMap, err := tablecodec.DecodeRow(value, colTps, ctx.GetSessionVars().GetTimeZone()) if err != nil { - return nil, errors.Trace(err) + return nil, rowMap, errors.Trace(err) } defaultVals := make([]types.Datum, len(cols)) for i, col := range cols { @@ -552,10 +551,10 @@ func DecodeRawRowData(ctx sessionctx.Context, meta *model.TableInfo, h int64, co } v[i], err = GetColDefaultValue(ctx, col, defaultVals) if err != nil { - return nil, errors.Trace(err) + return nil, rowMap, errors.Trace(err) } } - return v, nil + return v, rowMap, nil } // Row implements table.Table Row interface. From 1c2e05bdacc9e78c6fc04b7502d8f4db8aa90357 Mon Sep 17 00:00:00 2001 From: Ewan Chou Date: Tue, 10 Apr 2018 03:05:47 +0800 Subject: [PATCH 4/4] *: refine comments --- executor/write.go | 2 +- table/tables/tables.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/executor/write.go b/executor/write.go index 52e89829a0ada..b11700933b878 100644 --- a/executor/write.go +++ b/executor/write.go @@ -1112,7 +1112,7 @@ func (e *InsertExec) updateDupRow(keys []keyWithDupError, k keyWithDupError, val if err != nil { return errors.Trace(err) } - // Fill write-only columns with originDefaultValue if not found in oldValue is null. + // Fill write-only and write-reorg columns with originDefaultValue if not found in oldValue. for _, col := range cols { if col.State != model.StatePublic && oldRow[col.Offset].IsNull() { _, found := oldRowMap[col.ID] diff --git a/table/tables/tables.go b/table/tables/tables.go index 5b26d345ca923..d03fdac733121 100644 --- a/table/tables/tables.go +++ b/table/tables/tables.go @@ -513,7 +513,7 @@ func (t *Table) RowWithCols(ctx sessionctx.Context, h int64, cols []*table.Colum return v, nil } -// DecodeRawRowData decodes raw row data to a datum row. +// DecodeRawRowData decodes raw row data into a datum slice and a (columnID:columnValue) map. func DecodeRawRowData(ctx sessionctx.Context, meta *model.TableInfo, h int64, cols []*table.Column, value []byte) ([]types.Datum, map[int64]types.Datum, error) { v := make([]types.Datum, len(cols))