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.

Sign up for GitHub

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

Jump to bottom

infoschema: fix panic for TableByID when table id is negative #52016

Merged
merged 3 commits into from
Mar 27, 2024
Merged

infoschema: fix panic for TableByID when table id is negative #52016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1f9083
infoschema: fix panic for `TableByID` when table id is negative
lcwangchao Mar 22, 2024
f9ea645
Merge branch 'master' into fix_52012
lcwangchao Mar 25, 2024
a00db6d
Merge branch 'master' into fix_52012
lcwangchao Mar 27, 2024
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/restore/db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ func TestGetExistedUserDBs(t *testing.T) {
{Name: model.NewCIStr("d1")},
{
Name: model.NewCIStr("test"),
Tables: []*model.TableInfo{{Name: model.NewCIStr("t1"), State: model.StatePublic}},
Tables: []*model.TableInfo{{ID: 1, Name: model.NewCIStr("t1"), State: model.StatePublic}},
State: model.StatePublic,
},
},
Expand Down
4 changes: 3 additions & 1 deletion pkg/infoschema/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"github.com/pingcap/tidb/pkg/table"
"github.com/pingcap/tidb/pkg/table/tables"
"github.com/pingcap/tidb/pkg/util/domainutil"
"github.com/pingcap/tidb/pkg/util/intest"
)

// Builder builds a new InfoSchema.
Expand Down Expand Up @@ -970,9 +971,10 @@ func NewBuilder(r autoid.Requirement, factory func() (pools.Resource, error), in
}

func tableBucketIdx(tableID int64) int {
intest.Assert(tableID > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this tableID > 0 be false in the test scenario? If so, whether it is possible to encounter real scenes. If not, is the test unnecessary?

Copy link
Collaborator Author

@lcwangchao lcwangchao Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We found tableID <= 0 in some test cases but they are handled before tableBucketIdx. However, adding assert here for two propose:

  • Provide a more clear reason for the failed test even if it would also fail without the assert function. The assert function is a more straightforward way to check the constraints.

  • This assert also checks tableID should not be 0. It may not fail if some codes building infoschema with an zero id table.

return int(tableID % bucketCount)
}

func tableIDIsValid(tableID int64) bool {
return tableID != 0
return tableID > 0
}
18 changes: 18 additions & 0 deletions pkg/infoschema/infoschema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,17 @@ func MockInfoSchema(tbList []*model.TableInfo) InfoSchema {
tables: make(map[string]table.Table),
}
result.schemaMap["test"] = tableNames
var tableIDs map[int64]struct{}
for _, tb := range tbList {
intest.AssertFunc(func() bool {
if tableIDs == nil {
tableIDs = make(map[int64]struct{})
}
_, ok := tableIDs[tb.ID]
intest.Assert(!ok)
tableIDs[tb.ID] = struct{}{}
return true
})
tb.DBID = dbInfo.ID
tbl := table.MockTableFromMeta(tb)
tableNames.tables[tb.Name.L] = tbl
Expand Down Expand Up @@ -238,6 +248,10 @@ func SchemaByTable(is InfoSchema, tableInfo *model.TableInfo) (val *model.DBInfo
}

func (is *infoSchema) TableByID(id int64) (val table.Table, ok bool) {
if !tableIDIsValid(id) {
return nil, false
}

slice := is.sortedTablesBuckets[tableBucketIdx(id)]
idx := slice.searchTable(id)
if idx == -1 {
Expand Down Expand Up @@ -713,6 +727,10 @@ func (ts *SessionExtendedInfoSchema) FindTableInfoByPartitionID(

// TableByID implements InfoSchema.TableByID
func (ts *SessionExtendedInfoSchema) TableByID(id int64) (table.Table, bool) {
if !tableIDIsValid(id) {
return nil, false
}

if ts.LocalTemporaryTables != nil {
if tbl, ok := ts.LocalTemporaryTables.TableByID(id); ok {
return tbl, true
Expand Down
27 changes: 27 additions & 0 deletions pkg/infoschema/infoschema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,14 @@ func TestBasic(t *testing.T) {
require.False(t, ok)
require.Nil(t, gotTblInfo)

tb, ok = is.TableByID(-12345)
require.False(t, ok)
require.Nil(t, tb)

gotTblInfo, ok = is.TableInfoByID(-12345)
require.False(t, ok)
require.Nil(t, gotTblInfo)

tb, err = is.TableByName(dbName, tbName)
require.NoError(t, err)
require.NotNil(t, tb)
Expand All @@ -187,6 +195,17 @@ func TestBasic(t *testing.T) {
require.Error(t, err)
require.Nil(t, gotTblInfo)

// negative id should always be seen as not exists
tb, ok = is.TableByID(-1)
require.False(t, ok)
require.Nil(t, tb)
schema, ok = is.SchemaByID(-1)
require.False(t, ok)
require.Nil(t, schema)
gotTblInfo, ok = is.TableInfoByID(-1)
require.Nil(t, gotTblInfo)
require.False(t, ok)

tbs := is.SchemaTables(dbName)
require.Len(t, tbs, 1)

Expand Down Expand Up @@ -855,6 +874,14 @@ func TestLocalTemporaryTables(t *testing.T) {
info, ok = is.SchemaByID(tb22.Meta().DBID)
require.False(t, ok)
require.Nil(t, info)

// negative id should always be seen as not exists
tbl, ok = is.TableByID(-1)
require.False(t, ok)
require.Nil(t, tbl)
info, ok = is.SchemaByID(-1)
require.False(t, ok)
require.Nil(t, info)
}

// TestInfoSchemaCreateTableLike tests the table's column ID and index ID for memory database.
Expand Down
4 changes: 4 additions & 0 deletions pkg/infoschema/infoschema_v2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,10 @@ func (is *infoschemaV2) base() *infoSchema {
}

func (is *infoschemaV2) TableByID(id int64) (val table.Table, ok bool) {
if !tableIDIsValid(id) {
return
}

// Get from the cache.
key := tableCacheKey{id, is.infoSchema.schemaMetaVersion}
tbl, found := is.tableCache.Get(key)
Expand Down
11 changes: 11 additions & 0 deletions pkg/infoschema/infoschema_v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,17 @@ func TestV2Basic(t *testing.T) {
require.True(t, ok)
require.Same(t, gotTblInfo, getTableInfo.Meta())

// negative id should always be seen as not exists
getTableInfo, ok = is.TableByID(-1)
require.False(t, ok)
require.Nil(t, getTableInfo)
gotTblInfo, ok = is.TableInfoByID(-1)
require.False(t, ok)
require.Nil(t, gotTblInfo)
getDBInfo, ok = is.SchemaByID(-1)
require.False(t, ok)
require.Nil(t, getDBInfo)

gotTblInfo, ok = is.TableInfoByID(1234567)
require.False(t, ok)
require.Nil(t, gotTblInfo)
Expand Down
2 changes: 1 addition & 1 deletion pkg/planner/core/logical_plans_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func createPlannerSuite() (s *plannerSuite) {
MockListPartitionTable(),
MockStateNoneColumnTable(),
}
id := int64(0)
id := int64(1)
for _, tblInfo := range tblInfos {
tblInfo.ID = id
id += 1
Expand Down
8 changes: 8 additions & 0 deletions pkg/planner/core/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ func MockSignedTable() *model.TableInfo {
col5.SetFlag(mysql.NotNullFlag)
col6.SetFlag(mysql.NoDefaultValueFlag)
table := &model.TableInfo{
ID: 1,
Columns: []*model.ColumnInfo{pkColumn, col0, col1, col2, col3, colStr1, colStr2, colStr3, col4, col5, col6, col7},
Indices: indices,
Name: model.NewCIStr("t"),
Expand Down Expand Up @@ -336,6 +337,7 @@ func MockUnsignedTable() *model.TableInfo {
col0.SetFlag(mysql.NotNullFlag)
col1.SetFlag(mysql.UnsignedFlag)
table := &model.TableInfo{
ID: 2,
Columns: []*model.ColumnInfo{pkColumn, col0, col1},
Indices: indices,
Name: model.NewCIStr("t2"),
Expand Down Expand Up @@ -365,6 +367,7 @@ func MockNoPKTable() *model.TableInfo {
col0.SetFlag(mysql.NotNullFlag)
col1.SetFlag(mysql.UnsignedFlag)
table := &model.TableInfo{
ID: 3,
Columns: []*model.ColumnInfo{col0, col1},
Name: model.NewCIStr("t3"),
PKIsHandle: true,
Expand Down Expand Up @@ -395,6 +398,7 @@ func MockView() *model.TableInfo {
}
view := &model.ViewInfo{SelectStmt: selectStmt, Security: model.SecurityDefiner, Definer: &auth.UserIdentity{Username: "root", Hostname: ""}, Cols: []model.CIStr{col0.Name, col1.Name, col2.Name}}
table := &model.TableInfo{
ID: 4,
Name: model.NewCIStr("v"),
Columns: []*model.ColumnInfo{col0, col1, col2},
View: view,
Expand Down Expand Up @@ -462,6 +466,7 @@ func MockRangePartitionTable() *model.TableInfo {
},
}
tableInfo := MockSignedTable()
tableInfo.ID = 5
tableInfo.Name = model.NewCIStr("pt1")
cols := make([]*model.ColumnInfo, 0, len(tableInfo.Columns))
cols = append(cols, tableInfo.Columns...)
Expand Down Expand Up @@ -497,6 +502,7 @@ func MockHashPartitionTable() *model.TableInfo {
},
}
tableInfo := MockSignedTable()
tableInfo.ID = 6
tableInfo.Name = model.NewCIStr("pt2")
cols := make([]*model.ColumnInfo, 0, len(tableInfo.Columns))
cols = append(cols, tableInfo.Columns...)
Expand Down Expand Up @@ -543,6 +549,7 @@ func MockListPartitionTable() *model.TableInfo {
},
}
tableInfo := MockSignedTable()
tableInfo.ID = 7
tableInfo.Name = model.NewCIStr("pt3")
cols := make([]*model.ColumnInfo, 0, len(tableInfo.Columns))
cols = append(cols, tableInfo.Columns...)
Expand Down Expand Up @@ -610,6 +617,7 @@ func MockStateNoneColumnTable() *model.TableInfo {
col0.SetFlag(mysql.NotNullFlag)
col1.SetFlag(mysql.UnsignedFlag)
table := &model.TableInfo{
ID: 8,
Columns: []*model.ColumnInfo{pkColumn, col0, col1},
Indices: indices,
Name: model.NewCIStr("T_StateNoneColumn"),
Expand Down
4 changes: 2 additions & 2 deletions pkg/ttl/ttlworker/job_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,8 @@ func TestReadyForLockHBTimeoutJobTables(t *testing.T) {
tables := m.readyForLockHBTimeoutJobTables(se.Now())
if c.shouldSchedule {
assert.Len(t, tables, 1)
assert.Equal(t, int64(0), tables[0].ID)
assert.Equal(t, int64(0), tables[0].TableInfo.ID)
assert.Equal(t, tbl.ID, tables[0].ID)
assert.Equal(t, tbl.ID, tables[0].TableInfo.ID)
} else {
assert.Len(t, tables, 0)
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/ttl/ttlworker/session_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"errors"
"strings"
"sync/atomic"
"testing"
"time"

Expand All @@ -36,8 +37,11 @@ import (
"github.com/stretchr/testify/require"
)

var idAllocator atomic.Int64

func newMockTTLTbl(t *testing.T, name string) *cache.PhysicalTable {
tblInfo := &model.TableInfo{
ID: idAllocator.Add(1),
Name: model.NewCIStr(name),
Columns: []*model.ColumnInfo{
{
Expand Down
Loading