diff --git a/pkg/table/tables/BUILD.bazel b/pkg/table/tables/BUILD.bazel index eb6dac9730867..25b602792f09a 100644 --- a/pkg/table/tables/BUILD.bazel +++ b/pkg/table/tables/BUILD.bazel @@ -71,6 +71,7 @@ go_test( "assertion_test.go", "bench_test.go", "cache_test.go", + "canskip_test.go", "export_test.go", "index_test.go", "main_test.go", @@ -80,7 +81,7 @@ go_test( ], embed = [":tables"], flaky = True, - shard_count = 43, + shard_count = 50, deps = [ "//pkg/ddl", "//pkg/domain", diff --git a/pkg/table/tables/canskip_test.go b/pkg/table/tables/canskip_test.go new file mode 100644 index 0000000000000..8807b6813dbb1 --- /dev/null +++ b/pkg/table/tables/canskip_test.go @@ -0,0 +1,276 @@ +// Copyright 2026 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tables_test + +import ( + "context" + "testing" + + "github.com/pingcap/tidb/pkg/meta/model" + "github.com/pingcap/tidb/pkg/parser/ast" + "github.com/pingcap/tidb/pkg/parser/mysql" + "github.com/pingcap/tidb/pkg/table" + "github.com/pingcap/tidb/pkg/table/tables" + "github.com/pingcap/tidb/pkg/testkit" + ttypes "github.com/pingcap/tidb/pkg/types" + "github.com/stretchr/testify/require" +) + +// TestCanSkipNullColumnAfterNotNullToNullDDL tests that CanSkip does NOT skip +// encoding null column IDs for columns that changed from NOT NULL to NULL. +// +// This is the regression test for https://github.com/pingcap/tidb/issues/61709 +// +// Row Format v2 spec (docs/design/2018-07-19-row-format.md) states: +// +// "we can not omit the null column ID because if the column ID is not found +// in the row, we will use the default value in the schema which may not be null" +// +// After NOT NULL→NULL DDL, both DefaultValue and OriginDefaultValue are nil. +// The old CanSkip logic would skip encoding the null column ID in this case, +// causing downstream components (TiFlash) to misinterpret the missing column +// as "use default value" instead of "value is NULL". +func TestCanSkipNullColumnAfterNotNullToNullDDL(t *testing.T) { + // Simulate a column that was changed from NOT NULL to NULL via DDL. + // After the DDL, DefaultValue=nil, OriginDefaultValue=nil, and the column + // is now nullable (NotNullFlag is cleared). + colInfo := &model.ColumnInfo{ + ID: 1, + Name: ast.NewCIStr("c"), + } + colInfo.SetType(mysql.TypeLong) + colInfo.SetFlag(0) // NOT NULL flag is cleared after DDL + // After NOT NULL→NULL DDL, both defaults are nil + colInfo.DefaultValue = nil + colInfo.OriginDefaultValue = nil + colInfo.State = model.StatePublic + + col := &table.Column{ColumnInfo: colInfo} + + tblInfo := &model.TableInfo{ + ID: 1, + Name: ast.NewCIStr("t"), + Columns: []*model.ColumnInfo{colInfo}, + } + + // The value is NULL + nullDatum := ttypes.NewDatum(nil) + + // CanSkip should return false — the null column ID MUST be encoded + // so downstream can distinguish "value is NULL" from "column not in row". + result := tables.CanSkip(tblInfo, col, &nullDatum) + require.False(t, result, + "CanSkip should NOT skip null column encoding after NOT NULL→NULL DDL. "+ + "Row Format v2 requires null column IDs to be encoded so downstream "+ + "components can distinguish NULL values from missing columns.") +} + +// TestCanSkipNullColumnAlwaysNullable tests that CanSkip correctly handles +// columns that have always been nullable with nil defaults. +// This is the case where the optimization was originally intended to work. +func TestCanSkipNullColumnAlwaysNullable(t *testing.T) { + // A column that was always nullable, with nil defaults from creation. + colInfo := &model.ColumnInfo{ + ID: 2, + Name: ast.NewCIStr("d"), + } + colInfo.SetType(mysql.TypeLong) + colInfo.SetFlag(0) // nullable + colInfo.DefaultValue = nil + colInfo.OriginDefaultValue = nil + colInfo.State = model.StatePublic + + col := &table.Column{ColumnInfo: colInfo} + + tblInfo := &model.TableInfo{ + ID: 2, + Name: ast.NewCIStr("t2"), + Columns: []*model.ColumnInfo{colInfo}, + } + + nullDatum := ttypes.NewDatum(nil) + + // Even for always-nullable columns, per Row Format v2 spec, null column IDs + // should be encoded. The spec says "we can not omit the null column ID". + // This test documents the correct behavior after the fix. + result := tables.CanSkip(tblInfo, col, &nullDatum) + require.False(t, result, + "CanSkip should NOT skip null column encoding even for always-nullable columns. "+ + "Row Format v2 spec requires null column IDs to always be encoded.") +} + +// TestCanSkipPKColumn tests that CanSkip correctly skips PK handle columns. +func TestCanSkipPKColumn(t *testing.T) { + colInfo := &model.ColumnInfo{ + ID: 1, + Name: ast.NewCIStr("id"), + } + colInfo.SetType(mysql.TypeLong) + colInfo.SetFlag(mysql.PriKeyFlag | mysql.NotNullFlag) + colInfo.State = model.StatePublic + + col := &table.Column{ColumnInfo: colInfo} + + tblInfo := &model.TableInfo{ + ID: 3, + Name: ast.NewCIStr("t3"), + Columns: []*model.ColumnInfo{colInfo}, + PKIsHandle: true, + } + + datum := ttypes.NewIntDatum(1) + + // PK handle columns should always be skipped + result := tables.CanSkip(tblInfo, col, &datum) + require.True(t, result, "CanSkip should skip PK handle columns") +} + +// TestCanSkipVirtualGeneratedColumn tests that CanSkip correctly skips virtual generated columns. +func TestCanSkipVirtualGeneratedColumn(t *testing.T) { + colInfo := &model.ColumnInfo{ + ID: 3, + Name: ast.NewCIStr("v"), + GeneratedExprString: "`a` + 1", + GeneratedStored: false, // virtual + } + colInfo.SetType(mysql.TypeLong) + colInfo.State = model.StatePublic + + col := &table.Column{ColumnInfo: colInfo} + + tblInfo := &model.TableInfo{ + ID: 4, + Name: ast.NewCIStr("t4"), + Columns: []*model.ColumnInfo{colInfo}, + } + + datum := ttypes.NewIntDatum(42) + + // Virtual generated columns should always be skipped + result := tables.CanSkip(tblInfo, col, &datum) + require.True(t, result, "CanSkip should skip virtual generated columns") +} + +// TestCanSkipNonNullValueWithDefault tests that CanSkip does NOT skip +// columns with non-null values. +func TestCanSkipNonNullValueWithDefault(t *testing.T) { + colInfo := &model.ColumnInfo{ + ID: 4, + Name: ast.NewCIStr("e"), + } + colInfo.SetType(mysql.TypeLong) + colInfo.SetFlag(0) // nullable + colInfo.DefaultValue = nil + colInfo.OriginDefaultValue = nil + colInfo.State = model.StatePublic + + col := &table.Column{ColumnInfo: colInfo} + + tblInfo := &model.TableInfo{ + ID: 5, + Name: ast.NewCIStr("t5"), + Columns: []*model.ColumnInfo{colInfo}, + } + + // Non-null value should never be skipped + datum := ttypes.NewIntDatum(42) + result := tables.CanSkip(tblInfo, col, &datum) + require.False(t, result, "CanSkip should NOT skip columns with non-null values") +} + +// TestCanSkipNullColumnWithNonNilDefault tests that CanSkip does NOT skip +// columns where the value is null but the default is non-nil. +func TestCanSkipNullColumnWithNonNilDefault(t *testing.T) { + colInfo := &model.ColumnInfo{ + ID: 5, + Name: ast.NewCIStr("f"), + } + colInfo.SetType(mysql.TypeLong) + colInfo.SetFlag(0) // nullable + colInfo.DefaultValue = int64(0) + colInfo.OriginDefaultValue = int64(0) + colInfo.State = model.StatePublic + + col := &table.Column{ColumnInfo: colInfo} + + tblInfo := &model.TableInfo{ + ID: 6, + Name: ast.NewCIStr("t6"), + Columns: []*model.ColumnInfo{colInfo}, + } + + nullDatum := ttypes.NewDatum(nil) + + // Null value with non-nil default should NOT be skipped + result := tables.CanSkip(tblInfo, col, &nullDatum) + require.False(t, result, + "CanSkip should NOT skip null column when default value is non-nil") +} + +// TestCanSkipEndToEndNotNullToNull is an integration test that verifies +// the CanSkip behavior through the full DDL + DML flow. +// It tests scenarios that produce DefaultValue=nil and OriginDefaultValue=nil, +// which is the exact trigger condition for the bug in issue #61709. +// After the fix, inserting NULL must encode the null column ID in the row. +func TestCanSkipEndToEndNotNullToNull(t *testing.T) { + store, dom := testkit.CreateMockStoreAndDomain(t) + tk := testkit.NewTestKit(t, store) + tk.MustExec("use test") + + // Scenario 1: Column created as nullable with no explicit DEFAULT. + // This produces DefaultValue=nil, OriginDefaultValue=nil. + tk.MustExec("CREATE TABLE t_canskip1 (id INT PRIMARY KEY, c INT NULL)") + + // Verify schema state: both defaults must be nil to hit the bug trigger. + tbl1, err := dom.InfoSchema().TableByName(context.Background(), ast.NewCIStr("test"), ast.NewCIStr("t_canskip1")) + require.NoError(t, err) + colC := tbl1.Meta().Columns[1] + require.Equal(t, "c", colC.Name.L) + require.Nil(t, colC.DefaultValue, "DefaultValue should be nil for nullable column without DEFAULT") + require.Nil(t, colC.OriginDefaultValue, "OriginDefaultValue should be nil for nullable column without DEFAULT") + + // Insert NULL — this is the exact path that triggered the bug. + tk.MustExec("INSERT INTO t_canskip1 VALUES (1, NULL)") + tk.MustQuery("SELECT id, c FROM t_canskip1 WHERE id = 1").Check( + testkit.Rows("1 "), + ) + + // Scenario 2: ADD COLUMN as nullable with no explicit DEFAULT. + tk.MustExec("CREATE TABLE t_canskip2 (id INT PRIMARY KEY)") + tk.MustExec("ALTER TABLE t_canskip2 ADD COLUMN d INT NULL") + + // Verify schema state after ADD COLUMN. + tbl2, err := dom.InfoSchema().TableByName(context.Background(), ast.NewCIStr("test"), ast.NewCIStr("t_canskip2")) + require.NoError(t, err) + colD := tbl2.Meta().Columns[1] + require.Equal(t, "d", colD.Name.L) + require.Nil(t, colD.DefaultValue, "DefaultValue should be nil after ADD COLUMN nullable") + require.Nil(t, colD.OriginDefaultValue, "OriginDefaultValue should be nil after ADD COLUMN nullable") + + tk.MustExec("INSERT INTO t_canskip2 VALUES (1, NULL)") + tk.MustQuery("SELECT id, d FROM t_canskip2 WHERE id = 1").Check( + testkit.Rows("1 "), + ) + + // Verify non-NULL values still work correctly. + tk.MustExec("INSERT INTO t_canskip1 VALUES (2, 42)") + tk.MustExec("INSERT INTO t_canskip2 VALUES (2, 42)") + tk.MustQuery("SELECT id, c FROM t_canskip1 ORDER BY id").Check( + testkit.Rows("1 ", "2 42"), + ) + tk.MustQuery("SELECT id, d FROM t_canskip2 ORDER BY id").Check( + testkit.Rows("1 ", "2 42"), + ) +} diff --git a/pkg/table/tables/tables.go b/pkg/table/tables/tables.go index 8fba8268c54e9..7eae9f53c8eba 100644 --- a/pkg/table/tables/tables.go +++ b/pkg/table/tables/tables.go @@ -1523,8 +1523,18 @@ func (t *TableCommon) canSkip(col *table.Column, value *types.Datum) bool { // CanSkip is for these cases, we can skip the columns in encoded row: // 1. the column is included in primary key; -// 2. the column's default value is null, and the value equals to that but has no origin default; -// 3. the column is virtual generated. +// 2. the column is virtual generated. +// +// Note: Previously, columns with nil default, nil origin default, and null value +// were also skipped. This was removed because it violates the Row Format v2 spec +// (docs/design/2018-07-19-row-format.md) which states: +// +// "we can not omit the null column ID because if the column ID is not found +// in the row, we will use the default value in the schema which may not be null" +// +// After NOT NULL→NULL DDL changes, both defaults become nil, causing downstream +// components (TiFlash) to misinterpret missing null column IDs as "use default" +// instead of "value is NULL". See https://github.com/pingcap/tidb/issues/61709 func CanSkip(info *model.TableInfo, col *table.Column, value *types.Datum) bool { if col.IsPKHandleColumn(info) { return true @@ -1540,9 +1550,6 @@ func CanSkip(info *model.TableInfo, col *table.Column, value *types.Datum) bool return canSkip } } - if col.GetDefaultValue() == nil && value.IsNull() && col.GetOriginDefaultValue() == nil { - return true - } if col.IsVirtualGenerated() { return true }