planner: update binding's original_sql when upgrading (pingcap#46940)

close pingcap#44298

bindinfo/handle.go

@@ -136,18 +136,20 @@ func (h *BindHandle) Reset(ctx sessionctx.Context) {
 func (h *BindHandle) Update(fullLoad bool) (err error) {
 	h.bindInfo.Lock()
 	lastUpdateTime := h.bindInfo.lastUpdateTime
-	updateTime := lastUpdateTime.String()
-	if fullLoad {
-		updateTime = "0000-00-00 00:00:00"
+	var timeCondition string
+	if !fullLoad {
+		timeCondition = fmt.Sprintf("WHERE update_time>'%s'", lastUpdateTime.String())
 	}
 
 	exec := h.sctx.Context.(sqlexec.RestrictedSQLExecutor)
 
 	ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnBindInfo)
 	// No need to acquire the session context lock for ExecRestrictedSQL, it
 	// uses another background session.
-	rows, _, err := exec.ExecRestrictedSQL(ctx, nil, `SELECT original_sql, bind_sql, default_db, status, create_time, update_time, charset, collation, source, sql_digest, plan_digest
-	FROM mysql.bind_info WHERE update_time > %? ORDER BY update_time, create_time`, updateTime)
+	selectStmt := fmt.Sprintf(`SELECT original_sql, bind_sql, default_db, status, create_time,
+       update_time, charset, collation, source, sql_digest, plan_digest FROM mysql.bind_info
+       %s ORDER BY update_time, create_time`, timeCondition)
+	rows, _, err := exec.ExecRestrictedSQL(ctx, nil, selectStmt)
 
 	if err != nil {
 		h.bindInfo.Unlock()

session/bootstrap.go

@@ -984,11 +984,15 @@ const (
 	//   add column `step`, `error`; delete unique key; and add key idx_state_update_time
 	//   to `mysql.tidb_background_subtask_history`.
 	version174 = 174
+
+	// version 175
+	//   update normalized bindings of `in (?)` to `in (...)` to solve #44298.
+	version175 = 175
 )
 
 // currentBootstrapVersion is defined as a variable, so we can modify its value for testing.
 // please make sure this is the largest version
-var currentBootstrapVersion int64 = version173
+var currentBootstrapVersion int64 = version175
 
 // DDL owner key's expired time is ManagerSessionTTL seconds, we should wait the time and give more time to have a chance to finish it.
 var internalSQLTimeout = owner.ManagerSessionTTL + 15
@@ -1131,6 +1135,7 @@ var (
 		upgradeToVer172,
 		upgradeToVer173,
 		upgradeToVer174,
+		upgradeToVer175,
 	}
 )
 
@@ -2737,6 +2742,56 @@ func upgradeToVer174(s Session, ver int64) {
 	doReentrantDDL(s, "ALTER TABLE mysql.tidb_background_subtask_history ADD INDEX `idx_state_update_time`(`state_update_time`)", dbterror.ErrDupKeyName)
 }
 
+// upgradeToVer175 updates normalized bindings of `in (?)` to `in (...)` to solve
+// the issue #44298 that bindings for `in (?)` can't work for `in (?, ?, ?)`.
+// After this update, multiple bindings may have the same `original_sql`, but it's OK, and
+// for safety, don't remove duplicated bindings when upgrading.
+func upgradeToVer175(s Session, ver int64) {
+	if ver >= version175 {
+		return
+	}
+
+	var err error
+	mustExecute(s, "BEGIN PESSIMISTIC")
+	defer func() {
+		if err != nil {
+			mustExecute(s, "ROLLBACK")
+			return
+		}
+		mustExecute(s, "COMMIT")
+	}()
+	ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnBootstrap)
+	rs, err := s.ExecuteInternal(ctx, "SELECT original_sql, bind_sql FROM mysql.bind_info WHERE source != 'builtin'")
+	if err != nil {
+		logutil.BgLogger().Fatal("upgradeToVer175 error", zap.Error(err))
+		return
+	}
+	req := rs.NewChunk(nil)
+	for {
+		err = rs.Next(ctx, req)
+		if err != nil {
+			logutil.BgLogger().Fatal("upgradeToVer175 error", zap.Error(err))
+			return
+		}
+		if req.NumRows() == 0 {
+			break
+		}
+		for i := 0; i < req.NumRows(); i++ {
+			originalNormalizedSQL, bindSQL := req.GetRow(i).GetString(0), req.GetRow(i).GetString(1)
+			newNormalizedSQL := parser.NormalizeForBinding(bindSQL)
+			// update `in (?)` to `in (...)`
+			if originalNormalizedSQL == newNormalizedSQL {
+				continue // no need to update
+			}
+			mustExecute(s, fmt.Sprintf("UPDATE mysql.bind_info SET original_sql='%s' WHERE original_sql='%s'", newNormalizedSQL, originalNormalizedSQL))
+		}
+		req.Reset()
+	}
+	if err := rs.Close(); err != nil {
+		logutil.BgLogger().Fatal("upgradeToVer175 error", zap.Error(err))
+	}
+}
+
 func writeOOMAction(s Session) {
 	comment := "oom-action is `log` by default in v3.0.x, `cancel` by default in v4.0.11+"
 	mustExecute(s, `INSERT HIGH_PRIORITY INTO %n.%n VALUES (%?, %?, %?) ON DUPLICATE KEY UPDATE VARIABLE_VALUE= %?`,

session/bootstrap_test.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"sort"
 	"testing"
 	"time"
 
@@ -2238,3 +2239,83 @@ func TestTiDBUpgradeToVer170(t *testing.T) {
 	require.Less(t, int64(ver169), ver)
 	dom.Close()
 }
+
+func TestTiDBBindingInListToVer175(t *testing.T) {
+	ctx := context.Background()
+	store, _ := CreateStoreAndBootstrap(t)
+	defer func() { require.NoError(t, store.Close()) }()
+
+	// bootstrap as version174
+	ver174 := version174
+	seV174 := CreateSessionAndSetID(t, store)
+	txn, err := store.Begin()
+	require.NoError(t, err)
+	m := meta.NewMeta(txn)
+	err = m.FinishBootstrap(int64(ver174))
+	require.NoError(t, err)
+	MustExec(t, seV174, fmt.Sprintf("update mysql.tidb set variable_value=%d where variable_name='tidb_server_version'", ver174))
+	err = txn.Commit(context.Background())
+	require.NoError(t, err)
+	unsetStoreBootstrapped(store.UUID())
+
+	// create some bindings at version174
+	MustExec(t, seV174, "use test")
+	MustExec(t, seV174, "create table t (a int, b int, c int, key(c))")
+	MustExec(t, seV174, "insert into mysql.bind_info values ('select * from `test` . `t` where `a` in ( ... )', 'SELECT /*+ use_index(`t` `c`)*/ * FROM `test`.`t` WHERE `a` IN (1,2,3)', 'test', 'enabled', '2023-09-13 14:41:38.319', '2023-09-13 14:41:35.319', 'utf8', 'utf8_general_ci', 'manual', '', '')")
+	MustExec(t, seV174, "insert into mysql.bind_info values ('select * from `test` . `t` where `a` in ( ? )', 'SELECT /*+ use_index(`t` `c`)*/ * FROM `test`.`t` WHERE `a` IN (1)', 'test', 'enabled', '2023-09-13 14:41:38.319', '2023-09-13 14:41:36.319', 'utf8', 'utf8_general_ci', 'manual', '', '')")
+	MustExec(t, seV174, "insert into mysql.bind_info values ('select * from `test` . `t` where `a` in ( ? ) and `b` in ( ... )', 'SELECT /*+ use_index(`t` `c`)*/ * FROM `test`.`t` WHERE `a` IN (1) AND `b` IN (1,2,3)', 'test', 'enabled', '2023-09-13 14:41:37.319', '2023-09-13 14:41:38.319', 'utf8', 'utf8_general_ci', 'manual', '', '')")
+
+	showBindings := func(s Session) (records []string) {
+		MustExec(t, s, "admin reload bindings")
+		res := MustExecToRecodeSet(t, s, "show global bindings")
+		chk := res.NewChunk(nil)
+		for {
+			require.NoError(t, res.Next(ctx, chk))
+			if chk.NumRows() == 0 {
+				break
+			}
+			for i := 0; i < chk.NumRows(); i++ {
+				originalSQL := chk.GetRow(i).GetString(0)
+				bindSQL := chk.GetRow(i).GetString(1)
+				records = append(records, fmt.Sprintf("%s:%s", bindSQL, originalSQL))
+			}
+		}
+		require.NoError(t, res.Close())
+		sort.Strings(records)
+		return
+	}
+	bindings := showBindings(seV174)
+	// on ver174, `in (1)` and `in (1,2,3)` have different normalized results: `in (?)` and `in (...)`
+	require.Equal(t, []string{"SELECT /*+ use_index(`t` `c`)*/ * FROM `test`.`t` WHERE `a` IN (1) AND `b` IN (1,2,3):select * from `test` . `t` where `a` in ( ? ) and `b` in ( ... )",
+		"SELECT /*+ use_index(`t` `c`)*/ * FROM `test`.`t` WHERE `a` IN (1):select * from `test` . `t` where `a` in ( ? )",
+		"SELECT /*+ use_index(`t` `c`)*/ * FROM `test`.`t` WHERE `a` IN (1,2,3):select * from `test` . `t` where `a` in ( ... )"}, bindings)
+
+	// upgrade to ver175
+	domCurVer, err := BootstrapSession(store)
+	require.NoError(t, err)
+	defer domCurVer.Close()
+	seCurVer := CreateSessionAndSetID(t, store)
+	ver, err := getBootstrapVersion(seCurVer)
+	require.NoError(t, err)
+	require.Equal(t, currentBootstrapVersion, ver)
+
+	// `in (?)` becomes to `in ( ... )`
+	bindings = showBindings(seCurVer)
+	require.Equal(t, []string{"SELECT /*+ use_index(`t` `c`)*/ * FROM `test`.`t` WHERE `a` IN (1) AND `b` IN (1,2,3):select * from `test` . `t` where `a` in ( ... ) and `b` in ( ... )",
+		"SELECT /*+ use_index(`t` `c`)*/ * FROM `test`.`t` WHERE `a` IN (1):select * from `test` . `t` where `a` in ( ... )"}, bindings)
+
+	planFromBinding := func(s Session, q string) {
+		MustExec(t, s, q)
+		res := MustExecToRecodeSet(t, s, "select @@last_plan_from_binding")
+		chk := res.NewChunk(nil)
+		require.NoError(t, res.Next(ctx, chk))
+		require.Equal(t, int64(1), chk.GetRow(0).GetInt64(0))
+		require.NoError(t, res.Close())
+	}
+	planFromBinding(seCurVer, "select * from test.t where a in (1)")
+	planFromBinding(seCurVer, "select * from test.t where a in (1,2,3)")
+	planFromBinding(seCurVer, "select * from test.t where a in (1,2,3,4,5,6,7)")
+	planFromBinding(seCurVer, "select * from test.t where a in (1,2,3,4,5,6,7) and b in(1)")
+	planFromBinding(seCurVer, "select * from test.t where a in (1,2,3,4,5,6,7) and b in(1,2,3,4)")
+	planFromBinding(seCurVer, "select * from test.t where a in (7) and b in(1,2,3,4)")
+}