planner: fix a panic of a cached prepared statement with IndexScan #8017

dbjoa · 2018-10-23T08:52:03Z

What problem does this PR solve?

Fix the panic of the prepared statement with IndexScan when using the prepared plan cache. Here, the plan tries to use the column info that is not in the data source schema.

BEFORE the PR

Database changed
mysql> drop table if exists t;
Query OK, 0 rows affected (0.37 sec)

mysql> create table t(a int, b int, c int, primary key (a, b));
Query OK, 0 rows affected (0.19 sec)

mysql> insert into t values(1, 1, 2), (1, 2, 3), (1, 3, 3), (2, 1, 2), (2, 2, 3), (2, 3, 3);
Query OK, 6 rows affected (0.06 sec)

mysql> prepare stmt1 from "select a, c from t where a = ? and c = ?";
Query OK, 0 rows affected (0.00 sec)

mysql> set @a=1, @b=3;
Query OK, 0 rows affected (0.00 sec)

mysql> execute stmt1 using @a, @b;
+---+------+
| a | c    |
+---+------+
| 1 |    3 |
| 1 |    3 |
+---+------+
2 rows in set (0.00 sec)

mysql> execute stmt1 using @a, @b;
ERROR 2013 (HY000): Lost connection to MySQL server during query

2018/10/23 17:23:56.297 conn.go:427: [error] lastCmd execute stmt1 using @a, @b, runtime error: invalid memory address or nil pointer dereference, goroutine 1250 [running]:
github.com/pingcap/tidb/server.(*clientConn).Run.func1(0xc008528410, 0xc007553dff)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/server/conn.go:425 +0x10c
panic(0x11ce380, 0x220b1e0)
	/usr/local/go/src/runtime/panic.go:513 +0x1b9
github.com/pingcap/tidb/util/ranger.newFieldType(...)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/util/ranger/ranger.go:453
github.com/pingcap/tidb/util/ranger.DetachCondAndBuildRangeForIndex(0x1533540, 0xc00845c8c0, 0xc0089deac0, 0x1, 0x1, 0xc008aa73c0, 0x2, 0x2, 0xc008bda0b0, 0x2, ...)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/util/ranger/detacher.go:308 +0xbd
github.com/pingcap/tidb/planner/core.(*Execute).buildRangeForIndexScan(0xc008b68ea0, 0x1533540, 0xc00845c8c0, 0xc008217500, 0x40baf8, 0x125aac0, 0x12cc4e0, 0x0, 0x1517d60)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/planner/core/common_plans.go:260 +0x2eb
github.com/pingcap/tidb/planner/core.(*Execute).rebuildRange(0xc008b68ea0, 0x1516080, 0xc008999a00, 0x1, 0x1517d60)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/planner/core/common_plans.go:239 +0x130
github.com/pingcap/tidb/planner/core.(*Execute).getPhysicalPlan(0xc008b68ea0, 0x1533540, 0xc00845c8c0, 0x151d2e0, 0xc00892d050, 0xc008af82c0, 0x1, 0x1, 0x0, 0x0)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/planner/core/common_plans.go:191 +0x3eb
github.com/pingcap/tidb/planner/core.(*Execute).OptimizePreparedPlan(0xc008b68ea0, 0x1533540, 0xc00845c8c0, 0x151d2e0, 0xc00892d050, 0x1, 0x0)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/planner/core/common_plans.go:173 +0x344
github.com/pingcap/tidb/planner.Optimize(0x1533540, 0xc00845c8c0, 0x1505640, 0xc008b71f80, 0x151d2e0, 0xc00892d050, 0x227fc00, 0x0, 0x0, 0x0)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/planner/optimize.go:53 +0x39e
github.com/pingcap/tidb/executor.(*Compiler).Compile(0xc007553980, 0x7f8e5d86c460, 0xc008b71f40, 0x150f640, 0xc008b71f80, 0x0, 0x0, 0x0)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/executor/compiler.go:49 +0x1d2
github.com/pingcap/tidb/session.(*session).execute(0xc00845c8c0, 0x7f8e5d86c460, 0xc008b71f40, 0xc008a66e21, 0x1a, 0x203002, 0x203002, 0xc008239180, 0xc008548000, 0x30)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/session/session.go:803 +0x6ba
github.com/pingcap/tidb/session.(*session).Execute(0xc00845c8c0, 0x7f8e5d86c460, 0xc008b71f40, 0xc008a66e21, 0x1a, 0x1, 0x0, 0x0, 0xc007553b58, 0x4d6e80)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/session/session.go:762 +0x69
github.com/pingcap/tidb/server.(*TiDBContext).Execute(0xc00852e5d0, 0x7f8e5d86c460, 0xc008b71f40, 0xc008a66e21, 0x1a, 0xc007553be8, 0x1052761, 0xc0001a3a40, 0x4000000000000000, 0x225fdc0)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/server/driver_tidb.go:240 +0x7c
github.com/pingcap/tidb/server.(*clientConn).handleQuery(0xc008528410, 0x7f8e5d86c460, 0xc008b71f40, 0xc008a66e21, 0x1a, 0x0, 0x0)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/server/conn.go:874 +0x8e
github.com/pingcap/tidb/server.(*clientConn).dispatch(0xc008528410, 0xc008a66e21, 0x1b, 0x1b, 0x0, 0x0)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/server/conn.go:626 +0x686
github.com/pingcap/tidb/server.(*clientConn).Run(0xc008528410)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/server/conn.go:470 +0x1be
github.com/pingcap/tidb/server.(*Server).onConn(0xc0074cbb00, 0x15196e0, 0xc000191920)
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/server/server.go:324 +0x224
created by github.com/pingcap/tidb/server.(*Server).Run
	/home/jsheo/dev/deps/go/src/github.com/pingcap/tidb/server/server.go:264 +0x4a9

AFTER the PR

mysql> use test;
create table t(a int, b int, c int, primary key (a, b));
insert into t values(1, 1, 2), (1, 2, 3), (1, 3, 3), (2, 1, 2), (2, 2, 3), (2, 3, 3);
prepare stmt1 from "select a, c from t where a = ? and c = ?";
Database changed
mysql> drop table if exists t;
set @a=1, @b=3;
execute stmt1 using @a, @b;
execute stmt1 using @a, @b;Query OK, 0 rows affected (0.34 sec)

mysql> create table t(a int, b int, c int, primary key (a, b));
Query OK, 0 rows affected (0.18 sec)

mysql> insert into t values(1, 1, 2), (1, 2, 3), (1, 3, 3), (2, 1, 2), (2, 2, 3), (2, 3, 3);
Query OK, 6 rows affected (0.05 sec)

mysql> prepare stmt1 from "select a, c from t where a = ? and c = ?";
Query OK, 0 rows affected (0.00 sec)

mysql> set @a=1, @b=3;
Query OK, 0 rows affected (0.00 sec)

mysql> execute stmt1 using @a, @b;
+---+------+
| a | c    |
+---+------+
| 1 |    3 |
| 1 |    3 |
+---+------+
2 rows in set (0.01 sec)

mysql> execute stmt1 using @a, @b;
+---+------+
| a | c    |
+---+------+
| 1 |    3 |
| 1 |    3 |
+---+------+
2 rows in set (0.00 sec)

What is changed and how it works?

When creating the schema of the PhysicalIndexScan, the PR uses the table schema to get a specified column if a specified column is not found from the datasource schema.

Check List

Tests

Unit test

Code changes

Has exported variable/fields change

Side effects

No

Related changes

Need to cherry-pick to the release branch

sre-bot · 2018-10-23T08:52:07Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

dbjoa · 2018-10-23T08:52:41Z

/run-all-tests

iamxy · 2018-10-23T09:20:16Z

/run-integration-tests

jackysp · 2018-10-23T11:48:52Z

Thanks @dbjoa ! PTAL @winoros .

winoros · 2018-10-24T06:44:18Z

planner/core/find_best_task.go

@@ -424,8 +424,12 @@ func (ds *DataSource) convertToIndexScan(prop *property.PhysicalProperty, path *
 // TODO: refactor this part, we should not call Clone in fact.
 func (is *PhysicalIndexScan) initSchema(id int, idx *model.IndexInfo, isDoubleRead bool) {
 	indexCols := make([]*expression.Column, 0, len(idx.Columns))
+	baseSchema := expression.TableInfo2SchemaWithDBName(is.ctx, is.DBName, is.Table)


The columns in datasourceSchema is generated in the same as TableInfo2SchemaWithDBName.
Why this change can take effect?

Let's consider the table and the query below:

use test; create table t(a int, b int, c int, primary key (a, b)); prepare stmt1 from "select a, c from t where a = ? and c = ?"; set @a=1, @c=3; execute stmt1 using @a, @c;

Before the PR, datasourceSchema contains only two columns, test.t.a and test.t.c. However, indexCols contains test.t.a, b, _row_id. Here, the RetType of the Column expression of b is nil because b is not in datasourceSchema. It causes the panic when accessing the RetType of b

After the PR, indexCols contains test.t.a, test.t.b, _row_id, and the RetType of the Column expression of b is not nil.

Oh, i got it.
Seems that you can directly use PhysicalIndexScan.Table.Columns[IndexCol.Offset].FieldType to get its RetType.

@winoros Is it enough to only add RetType: &is.Table.Columns[col.Offset].FieldType to the line below instead of filling out all fields?

tidb/planner/core/find_best_task.go

Line 430 in d65ce86

colFound = &expression.Column{ColName: col.Name, UniqueID: is.ctx.GetSessionVars().AllocPlanColumnID()}

Yes, it should be enough.

…g the prepared plan cache

dbjoa · 2018-10-25T05:08:36Z

@winoros I've updated the PR.

dbjoa · 2018-10-25T05:08:46Z

/run-all-tests

winoros

lgtm
Thanks a lot!

zz-jason

LGTM

zz-jason · 2018-10-25T05:54:31Z

@dbjoa please cherrypick this commit to release-2.1.

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

winoros reviewed Oct 24, 2018

View reviewed changes

zz-jason added type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Oct 24, 2018

planner: fix a panic of a prepared statement with IndexScan when usin…

43937b3

…g the prepared plan cache

winoros reviewed Oct 25, 2018

View reviewed changes

zz-jason approved these changes Oct 25, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. status/all tests passed labels Oct 25, 2018

zz-jason merged commit b556638 into pingcap:master Oct 25, 2018

dbjoa mentioned this pull request Oct 25, 2018

planner: fix a panic of a cached prepared statement with IndexScan(#8017) #8055

Merged

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: fix a panic of a cached prepared statement with IndexScan #8017

planner: fix a panic of a cached prepared statement with IndexScan #8017

dbjoa commented Oct 23, 2018

sre-bot commented Oct 23, 2018

dbjoa commented Oct 23, 2018

iamxy commented Oct 23, 2018

jackysp commented Oct 23, 2018

winoros Oct 24, 2018

dbjoa Oct 24, 2018 •

edited

Loading

winoros Oct 24, 2018

dbjoa Oct 24, 2018

winoros Oct 25, 2018

dbjoa commented Oct 25, 2018

dbjoa commented Oct 25, 2018

winoros left a comment

zz-jason left a comment

zz-jason commented Oct 25, 2018

planner: fix a panic of a cached prepared statement with IndexScan #8017

planner: fix a panic of a cached prepared statement with IndexScan #8017

Conversation

dbjoa commented Oct 23, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

sre-bot commented Oct 23, 2018

dbjoa commented Oct 23, 2018

iamxy commented Oct 23, 2018

jackysp commented Oct 23, 2018

winoros Oct 24, 2018

Choose a reason for hiding this comment

dbjoa Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

winoros Oct 24, 2018

Choose a reason for hiding this comment

dbjoa Oct 24, 2018

Choose a reason for hiding this comment

winoros Oct 25, 2018

Choose a reason for hiding this comment

dbjoa commented Oct 25, 2018

dbjoa commented Oct 25, 2018

winoros left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Oct 25, 2018

dbjoa Oct 24, 2018 •

edited

Loading