*: fix the issue of executing DDL after executing SQL failure in txn #8044

zimulala · 2018-10-24T13:28:07Z

What problem does this PR solve?

Fix the issue of the following case:

Execute the SQL of "begin";
A SQL that will fail to execute;
Execute DDL.

Specific examples are as follows:

create table t (i int key);
insert into t values (1);
begin;
insert into t values (1);
truncate table t;

The goroutine stack as follows:

2018/10/24 17:24:22.168 conn.go:427: [error] lastCmd truncate table t, runtime error: invalid memory address or nil pointer dereference, goroutine 388 [running]:
github.com/pingcap/tidb/server.(*clientConn).Run.func1(0xc426fe2410, 0xc427091dff)
	/Users/xia/workspace/src/github.com/pingcap/tidb/server/conn.go:425 +0x10e
panic(0x1db59c0, 0x2a95190)
	/Users/xia/go/src/runtime/panic.go:505 +0x229
github.com/pingcap/tidb/executor.(*DDLExec).toErr(0xc4279be640, 0x2099240, 0xc42783c840, 0xc42783c840, 0x1b96a03)
	/Users/xia/workspace/src/github.com/pingcap/tidb/executor/ddl.go:58 +0x175
github.com/pingcap/tidb/executor.(*DDLExec).Next(0xc4279be640, 0x31bc488, 0xc4279d8680, 0xc427a79350, 0x2099240, 0xc42783c840)
	/Users/xia/workspace/src/github.com/pingcap/tidb/executor/ddl.go:94 +0x267
github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelayExecutor(0xc427f00f00, 0x31bc488, 0xc4279d8680, 0x20d0e20, 0xc426f147e0, 0x20b1fa0, 0xc4279be640, 0x0, 0x0, 0x0, ...)
	/Users/xia/workspace/src/github.com/pingcap/tidb/executor/adapter.go:272 +0x157
github.com/pingcap/tidb/executor.(*ExecStmt).Exec(0xc427f00f00, 0x31bc488, 0xc4279d8680, 0x0, 0x0, 0x0, 0x0)
...

What is changed and how it works?

The NewTxn will commit the original transaction, then begin a new transaction.
The reason is that we used the transaction to do something after we failed to execute NewTxn. So we can move NewTxn before using the transaction.

Check List

Tests

Unit test

Code changes
Side effects
Related changes

Need to cherry-pick to the release branch

…lure in txn

zimulala · 2018-10-24T13:34:53Z

PTAL @jackysp @winkyao @tiancaiamao

crazycs520

LGTM.
Please fix CI.

jackysp · 2018-10-24T15:38:04Z

executor/ddl_test.go

+	_, err := tk.Exec("truncate table t;")
+	c.Assert(err.Error(), Equals, "[kv:1062]Duplicate entry '1' for key 'PRIMARY'")
+	result := tk.MustQuery("select count(*) from t")
+	result.Check(testkit.Rows("1"))


Both the previous transaction and the DDL failed. Is it expected?

ngaut · 2018-10-25T02:42:04Z

executor/ddl.go

@@ -67,6 +67,11 @@ func (e *DDLExec) Next(ctx context.Context, chk *chunk.Chunk) (err error) {
 		return nil
 	}
 	e.done = true
+
+	// For every DDL, we must commit current transaction.
+	if err := e.ctx.NewTxn(); err != nil {


I am confused, according to the comments, i am expecting to call something like "Commit".

jackysp

I think we should always rollback the previous transaction and add this behavior to the docs when the user wants to do a ddl in the transaction. commit could fail in TiDB, which is nearly not happened in MySQL. If the ddl returns a dml error, it is really confused the user. If the ddl does not return the dml error anyway, the user could not notice the previous is committed or not.

zimulala · 2018-10-25T06:00:03Z

@jackysp

commit could fail in TiDB, which is nearly not happened in MySQL.

This PR will fix the bug. Regarding this compatibility issue, the original TiDB is handled differently than MySQL. So we can discuss it later.

zimulala · 2018-10-25T06:00:44Z

/run-all-tests

jackysp

LGTM

winkyao

LGTM, please fix ci

zimulala · 2018-10-25T07:28:56Z

/run-common-test
/run-integration-common-test
/run-integration-ddl-test

…ingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn

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

ddl, executor: fix the issue of executing DDL after executing SQL fai…

1162a3f

…lure in txn

zimulala added component/DDL-need-LGT3 type/bugfix This PR fixes a bug. labels Oct 24, 2018

crazycs520 reviewed Oct 24, 2018

View reviewed changes

jackysp reviewed Oct 24, 2018

View reviewed changes

ngaut reviewed Oct 25, 2018

View reviewed changes

jackysp reviewed Oct 25, 2018

View reviewed changes

executor: address comments

361a41d

zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 25, 2018

jackysp reviewed Oct 25, 2018

View reviewed changes

jackysp added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 25, 2018

winkyao approved these changes Oct 25, 2018

View reviewed changes

Merge branch 'master' into ddl-in-txn

2d2a3a3

zimulala added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Oct 25, 2018

zimulala merged commit 38274c3 into pingcap:master Oct 25, 2018

zimulala deleted the ddl-in-txn branch October 25, 2018 07:52

zimulala added a commit to zimulala/tidb that referenced this pull request Oct 25, 2018

*: fix the issue of executing DDL after executing SQL failure in txn (p…

2b2ce0c

…ingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn

zimulala mentioned this pull request Oct 25, 2018

*: fix the issue of executing DDL after executing SQL failure in txn … #8056

Merged

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: fix the issue of executing DDL after executing SQL failure in txn #8044

*: fix the issue of executing DDL after executing SQL failure in txn #8044

zimulala commented Oct 24, 2018 •

edited

Loading

zimulala commented Oct 24, 2018

crazycs520 left a comment

jackysp Oct 24, 2018

zimulala Oct 25, 2018

ngaut Oct 25, 2018

jackysp left a comment

zimulala commented Oct 25, 2018 •

edited

Loading

zimulala commented Oct 25, 2018

jackysp left a comment

winkyao left a comment

zimulala commented Oct 25, 2018

*: fix the issue of executing DDL after executing SQL failure in txn #8044

*: fix the issue of executing DDL after executing SQL failure in txn #8044

Conversation

zimulala commented Oct 24, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

zimulala commented Oct 24, 2018

crazycs520 left a comment

Choose a reason for hiding this comment

jackysp Oct 24, 2018

Choose a reason for hiding this comment

zimulala Oct 25, 2018

Choose a reason for hiding this comment

ngaut Oct 25, 2018

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

zimulala commented Oct 25, 2018 • edited Loading

zimulala commented Oct 25, 2018

jackysp left a comment

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

zimulala commented Oct 25, 2018

zimulala commented Oct 24, 2018 •

edited

Loading

zimulala commented Oct 25, 2018 •

edited

Loading