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

execute prepared statement with binary protocol will panic when table schema changed #33509

Closed
lcwangchao opened this issue Mar 28, 2022 · 2 comments · Fixed by #33519 or #34425
Closed
Assignees
Labels
affects-4.0 This bug affects 4.0.x versions. affects-5.0 This bug affects 5.0.x versions. affects-5.1 This bug affects 5.1.x versions. affects-5.2 This bug affects 5.2.x versions. affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects 5.4.x versions. affects-6.0 severity/major sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug. type/regression

Comments

@lcwangchao
Copy link
Collaborator

lcwangchao commented Mar 28, 2022

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

import (
	"context"
	"database/sql"
	"testing"

	_ "github.com/go-sql-driver/mysql"
	"github.com/stretchr/testify/require"
)

func TestPrepareExecute(t *testing.T) {
	ctx := context.TODO()
	db, err := sql.Open("mysql", "root@tcp(127.0.0.1:4000)/test")
	require.NoError(t, err)

	conn, err := db.Conn(ctx)
	require.NoError(t, err)
	_, err = conn.ExecContext(ctx, "drop table if exists t1")
	require.NoError(t, err)
	_, err = conn.ExecContext(ctx, "create table t1 (id int primary key, v int)")
	require.NoError(t, err)
	_, err = conn.ExecContext(ctx, "insert into t1 values(1, 10)")
	require.NoError(t, err)

	stmt, err := conn.PrepareContext(context.TODO(), "select * from t1")
	require.NoError(t, err)

	require.Equal(t, []int{1, 10}, executePreparedStmtResult(t, stmt, 2))

	// drop one column
	_, err = conn.ExecContext(ctx, "alter table t1 drop column v")
	require.NoError(t, err)

	// the below will fail when get next row
	require.Equal(t, []int{1}, executePreparedStmtResult(t, stmt, 1))
}

func executePreparedStmtResult(t *testing.T, stmt *sql.Stmt, columns int) []int {
	rows, err := stmt.Query()
	require.NoError(t, err)
	if ok := rows.Next(); !ok {
		// fail here for the third execute
		require.Fail(t, rows.Err().Error())
	}
	row := make([]int, columns)
	scan := make([]interface{}, columns)
	for i := range row {
		scan[i] = &row[i]
	}

	require.NoError(t, rows.Scan(scan...))
	require.NoError(t, rows.Close())
	return row
}

2. What did you expect to see? (Required)

no error

3. What did you see instead (Required)

=== RUN   TestPrepareExecute
    staleread_test.go:91: 
        	Error Trace:	staleread_test.go:91
        	            				staleread_test.go:84
        	Error:      	Error 1105: runtime error: index out of range [1] with length 1
        	Test:       	TestPrepareExecute
--- FAIL: TestPrepareExecute (0.30s)

FAIL

Got a panic in tidb side:

[2022/03/28 17:45:17.492 +08:00] [ERROR] [conn.go:1031] ["connection running loop panic"] [conn=4089] [lastSQL="select * from t1"] [err="runtime error: index out of range [1] with length 1"] [stack="goroutine 380115 [running]:
github.com/pingcap/tidb/server.(*clientConn).Run.func1()
	/Users/wangchao/Code/pingcap/tidb/server/conn.go:1029 +0x88
panic({0x10738fac0, 0x14001f98510})
	/usr/local/go/src/runtime/panic.go:844 +0x26c
github.com/pingcap/tidb/server.(*clientConn).writeResultset.func1()
	/Users/wangchao/Code/pingcap/tidb/server/conn.go:2119 +0x3d4
panic({0x10738fac0, 0x14001f98510})
	/usr/local/go/src/runtime/panic.go:844 +0x26c
github.com/pingcap/tidb/util/chunk.Row.IsNull({0x140096b60a0, 0x0}, 0x1)
	/Users/wangchao/Code/pingcap/tidb/util/chunk/row.go:206 +0x74
github.com/pingcap/tidb/server.dumpBinaryRow({0x14001c26000, 0xa, 0x400}, {0x140051f81e0, 0x2, 0x2}, {0x140096b60a0, 0x0}, 0x140056a0780)
	/Users/wangchao/Code/pingcap/tidb/server/util.go:248 +0x1b0
github.com/pingcap/tidb/server.(*clientConn).writeChunks(0x140020ef400, {0x1075a5f10, 0x14003af06c0}, {0x1075ae598, 0x140096b6050}, 0x1, 0x0)
	/Users/wangchao/Code/pingcap/tidb/server/conn.go:2208 +0x390
github.com/pingcap/tidb/server.(*clientConn).writeResultset(0x140020ef400, {0x1075a5f10, 0x14003af06c0}, {0x1075ae598, 0x140096b6050}, 0x1, 0x0, 0x0)
	/Users/wangchao/Code/pingcap/tidb/server/conn.go:2136 +0x23c
github.com/pingcap/tidb/server.(*clientConn).executePreparedStmtAndWriteResult(0x140020ef400, {0x1075a5f10, 0x14003af06c0}, {0x1075bc220, 0x1400d01eaf0}, {0x1090cd0b8, 0x0, 0x0}, 0x0)
	/Users/wangchao/Code/pingcap/tidb/server/conn_stmt.go:263 +0x410
github.com/pingcap/tidb/server.(*clientConn).handleStmtExecute(0x140020ef400, {0x1075a5f10, 0x14003af06c0}, {0x14004c56205, 0x9, 0x9})
	/Users/wangchao/Code/pingcap/tidb/server/conn_stmt.go:209 +0x7c4
github.com/pingcap/tidb/server.(*clientConn).dispatch(0x140020ef400, {0x1075a5e68, 0x140056a0380}, {0x14004c56205, 0x9, 0x9})
	/Users/wangchao/Code/pingcap/tidb/server/conn.go:1367 +0xfd8
github.com/pingcap/tidb/server.(*clientConn).Run(0x140020ef400, {0x1075a5f10, 0x1401408c0c0})
	/Users/wangchao/Code/pingcap/tidb/server/conn.go:1095 +0x7b4
github.com/pingcap/tidb/server.(*Server).onConn(0x140077d00d0, 0x140020ef400)
	/Users/wangchao/Code/pingcap/tidb/server/server.go:551 +0xa64
created by github.com/pingcap/tidb/server.(*Server).startNetworkListener
	/Users/wangchao/Code/pingcap/tidb/server/server.go:448 +0x7a4
"]

4. What is your TiDB version? (Required)

master

@lcwangchao lcwangchao added type/bug The issue is confirmed as a bug. affects-4.0 This bug affects 4.0.x versions. affects-5.0 This bug affects 5.0.x versions. affects-5.1 This bug affects 5.1.x versions. affects-5.2 This bug affects 5.2.x versions. affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects 5.4.x versions. affects-6.0 sig/execution SIG execution severity/major labels Mar 28, 2022
@lcwangchao
Copy link
Collaborator Author

Seems introduced by this PR: #12388 , and the below code:

tidb/server/driver_tidb.go

Lines 347 to 370 in 4ff3ec2

func (trs *tidbResultSet) Columns() []*ColumnInfo {
if trs.columns != nil {
return trs.columns
}
// for prepare statement, try to get cached columnInfo array
if trs.preparedStmt != nil {
ps := trs.preparedStmt
if colInfos, ok := ps.ColumnInfos.([]*ColumnInfo); ok {
trs.columns = colInfos
}
}
if trs.columns == nil {
fields := trs.recordSet.Fields()
for _, v := range fields {
trs.columns = append(trs.columns, convertColumnInfo(v))
}
if trs.preparedStmt != nil {
// if ColumnInfo struct has allocated object,
// here maybe we need deep copy ColumnInfo to do caching
trs.preparedStmt.ColumnInfos = trs.columns
}
}
return trs.columns
}

The columns will be cached in prepared statement, but if the table's schema changed, the cache will not be cleared

lcwangchao added a commit to lcwangchao/tidb that referenced this issue May 6, 2022
@kennedy8312
Copy link

/type regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-4.0 This bug affects 4.0.x versions. affects-5.0 This bug affects 5.0.x versions. affects-5.1 This bug affects 5.1.x versions. affects-5.2 This bug affects 5.2.x versions. affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects 5.4.x versions. affects-6.0 severity/major sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug. type/regression
Projects
None yet
3 participants