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

planner: "for update" should not work in a single autocommit statement (#11715) #11736

Merged
merged 2 commits into from
Aug 15, 2019

Conversation

sre-bot
Copy link
Contributor

@sre-bot sre-bot commented Aug 14, 2019

cherry-pick #11715 to release-3.0


What problem does this PR solve?

mysql> show create table t;
+-------+------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                 |
+-------+------------------------------------------------------------------------------------------------------------------------------+
| t     | CREATE TABLE `t` (
  `id` int(11) NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+-------+------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.01 sec)

mysql> select * from t where id = 9 for update;
ERROR 2013 (HY000): Lost connection to MySQL server during query

What is changed and how it works?

The bug is introduced in this PR #10972

github.com/pingcap/tidb/server.(*clientConn).Run.func1
	/media/genius/OS/project/src/github.com/pingcap/tidb/server/conn.go:600
runtime.gopanic
	/media/genius/OS/project/go/src/runtime/panic.go:522
github.com/pingcap/tidb/server.(*clientConn).writeResultset.func1
	/media/genius/OS/project/src/github.com/pingcap/tidb/server/conn.go:1268
runtime.gopanic
	/media/genius/OS/project/go/src/runtime/panic.go:522
runtime.panicmem
	/media/genius/OS/project/go/src/runtime/panic.go:82
runtime.sigpanic
	/media/genius/OS/project/go/src/runtime/signal_unix.go:390
github.com/pingcap/tidb/session.(*TxnState).LockKeys
	<autogenerated>:1
github.com/pingcap/tidb/executor.(*PointGetExecutor).lockKeyIfNeeded
	/media/genius/OS/project/src/github.com/pingcap/tidb/executor/point_get.go:152
github.com/pingcap/tidb/executor.(*PointGetExecutor).Next
	/media/genius/OS/project/src/github.com/pingcap/tidb/executor/point_get.go:132
github.com/pingcap/tidb/executor.Next
	/media/genius/OS/project/src/github.com/pingcap/tidb/executor/executor.go:200
github.com/pingcap/tidb/executor.(*recordSet).Next
	/media/genius/OS/project/src/github.com/pingcap/tidb/executor/adapter.go:113
github.com/pingcap/tidb/server.(*tidbResultSet).Next
	/media/genius/OS/project/src/github.com/pingcap/tidb/server/driver_tidb.go:365
github.com/pingcap/tidb/server.(*clientConn).writeChunks
	/media/genius/OS/project/src/github.com/pingcap/tidb/server/conn.go:1315
github.com/pingcap/tidb/server.(*clientConn).writeResultset
	/media/genius/OS/project/src/github.com/pingcap/tidb/server/conn.go:1281
github.com/pingcap/tidb/server.(*clientConn).handleQuery
	/media/genius/OS/project/src/github.com/pingcap/tidb/server/conn.go:1198
github.com/pingcap/tidb/server.(*clientConn).dispatch
	/media/genius/OS/project/src/github.com/pingcap/tidb/server/conn.go:897
github.com/pingcap/tidb/server.(*clientConn).Run
	/media/genius/OS/project/src/github.com/pingcap/tidb/server/conn.go:652
github.com/pingcap/tidb/server.(*Server).onConn
	/media/genius/OS/project/src/github.com/pingcap/tidb/server/server.go:438

The root cause of the panic is the mismatch of the life span between statement and executor.
We close the statement (or Txn in the auto-commit case) after Execute returns a RecordSet.
However, RecordSet.Next is still using the Txn, so the panic occurs.

This fix is actually a workaround: change select ... for update statement to normal select ... statement in the auto-commit case.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

@sre-bot
Copy link
Contributor Author

sre-bot commented Aug 14, 2019

/run-all-tests

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@winoros winoros added the status/can-merge Indicates a PR has been approved by a committer. label Aug 14, 2019
@sre-bot
Copy link
Contributor Author

sre-bot commented Aug 14, 2019

Sorry @winoros, you don't have permission to trigger auto merge event on this branch.

@zz-jason zz-jason merged commit afa2a12 into pingcap:release-3.0 Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants