planner/core,binloginfo: add a projection to fix 'delete from join' schema #8805

tiancaiamao · 2018-12-25T07:20:24Z

What problem does this PR solve?

CREATE TABLE `b1` (
`id` int(11) NOT NULL AUTO_INCREMENT,
`job_id` varchar(50) NOT NULL,
`split_job_id` varchar(30) DEFAULT NULL,
PRIMARY KEY (`id`),
KEY `b1` (`job_id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin

CREATE TABLE `b2` (
`id` int(11) NOT NULL AUTO_INCREMENT,
`job_id` varchar(50) NOT NULL,
`batch_class` varchar(20) DEFAULT NULL,
PRIMARY KEY (`id`),
UNIQUE KEY `bu` (`job_id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4

mysql> insert into b2 (job_id, batch_class) values (2, 'TEST');
Query OK, 1 row affected (0.01 sec)

mysql> insert into b1 (job_id) values (2);
Query OK, 1 row affected (0.00 sec)

mysql> delete from b1 where job_id in (select job_id from b2 where batch_class = 'TEST') or split_job_id in (select job_id from b2 where batch_class = 'TEST');

With binlog on, the last SQL fail like this:

2018/12/24 14:10:22.596 session.go:851: [warning] con:1 schema_ver:20 session error:
2018/12/24 14:10:22.596 session.go:851: [warning] con:1 schema_ver:20 session error: EncodeRow error: data and columnID count not match 5 vs 3
github.com/pingcap/tidb/tablecodec.EncodeRow /home/robi/Code/go/src/github.com/pingcap/tidb/tablecodec/tablecodec.go:229
github.com/pingcap/tidb/table/tables.(*tableCommon).addDeleteBinlog /home/robi/Code/go/src/github.com/pingcap/tidb/table/tables/tables.go:734
github.com/pingcap/tidb/table/tables.(*tableCommon).RemoveRecord /home/robi/Code/go/src/github.com/pingcap/tidb/table/tables/tables.go:688
github.com/pingcap/tidb/executor.(*DeleteExec).removeRow /home/robi/Code/go/src/github.com/pingcap/tidb/executor/delete.go:223
github.com/pingcap/tidb/executor.(*DeleteExec).deleteOneRow /home/robi/Code/go/src/github.com/pingcap/tidb/executor/delete.go:80
github.com/pingcap/tidb/executor.(*DeleteExec).deleteSingleTableByChunk /home/robi/Code/go/src/github.com/pingcap/tidb/executor/delete.go:130
github.com/pingcap/tidb/executor.(*DeleteExec).Next /home/robi/Code/go/src/github.com/pingcap/tidb/executor/delete.go:57
github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelayExecutor /home/robi/Code/go/src/github.com/pingcap/tidb/executor/adapter.go:296
github.com/pingcap/tidb/executor.(*ExecStmt).Exec /home/robi/Code/go/src/github.com/pingcap/tidb/executor/adapter.go:242
github.com/pingcap/tidb/session.runStmt /home/robi/Code/go/src/github.com/pingcap/tidb/session/tidb.go:195
github.com/pingcap/tidb/session.(*session).executeStatement /home/robi/Code/go/src/github.com/pingcap/tidb/session/session.go:848
github.com/pingcap/tidb/session.(*session).execute /home/robi/Code/go/src/github.com/pingcap/tidb/session/session.go:916
github.com/pingcap/tidb/session.(*session).Execute /home/robi/Code/go/src/github.com/pingcap/tidb/session/session.go:869
github.com/pingcap/tidb/server.(*TiDBContext).Execute /home/robi/Code/go/src/github.com/pingcap/tidb/server/driver_tidb.go:245
github.com/pingcap/tidb/server.(*clientConn).handleQuery /home/robi/Code/go/src/github.com/pingcap/tidb/server/conn.go:922
github.com/pingcap/tidb/server.(*clientConn).dispatch /home/robi/Code/go/src/github.com/pingcap/tidb/server/conn.go:658
github.com/pingcap/tidb/server.(*clientConn).Run /home/robi/Code/go/src/github.com/pingcap/tidb/server/conn.go:502
github.com/pingcap/tidb/server.(*Server).onConn /home/robi/Code/go/src/github.com/pingcap/tidb/server/server.go:324

What is changed and how it works?

The final schema should be the schema of table b1, rather than the schema of join.
Add a projection to the plan to get the correct schema.

Check List

Tests

Unit test

This change is

'delete from t where a in (select ...) or b in (select ...)' The correct schema should be the schema of t, rather than the schema of the join. Add a projection in this case.

tiancaiamao · 2018-12-25T07:47:07Z

PTAL @lysu @winoros

eurekaka · 2018-12-25T08:03:50Z

planner/core/logical_plan_builder.go

+		proj.SetSchema(oldSchema.Clone())
+		p = proj
+	}
+


Should we add projection for Update as well?

Yes, update have the same problem

I simply add a projection but the CI failed.
I'll fix delete first, and leave update to somebody else.

winoros

lgtm

sessionctx/binloginfo/binloginfo_test.go

lysu · 2018-12-25T08:36:51Z

planner/core/logical_plan_builder.go

+	// Add a projection for the following case, otherwise the final schema will be the schema of the join.
+	// delete from t where a in (select ...) or b in (select ...)
+	if oldLen := oldSchema.Len(); oldLen != p.Schema().Len() {
+		proj := LogicalProjection{Exprs: expression.Column2Exprs(p.Schema().Columns[:oldLen])}.Init(b.ctx)


it seems to delete target table isn't always the first table, MySQL support delete multiple tables syntax, although I never use that https://dev.mysql.com/doc/refman/5.7/en/delete.html 🤣

Yes, we should consider the case of multi-table deletion.

Delete from A where ... // delete single table
Delete A B ... from ... // delete multiple table
Delete from A B using ... // delete multiple table

Multiple tables case is more complex. This PR will only handle the single table cases.

Let's have a look what's the bug actually:
select from A where A join B ... there is a projection on A join B
but for
delete from A where A join B ..., the projection is missing.

What output should it be if it's a multiple tables deleting?

shenli · 2018-12-26T01:19:11Z

@tiancaiamao Please address the comments.

tiancaiamao · 2018-12-26T09:11:04Z

/run-all-tests

tiancaiamao · 2018-12-26T13:23:09Z

/run-all-tests

tiancaiamao · 2018-12-26T13:29:18Z

/run-all-tests

tiancaiamao · 2018-12-26T14:12:50Z

/run-unit-test

eurekaka · 2018-12-27T02:30:42Z

/run-unit-test

tiancaiamao · 2018-12-29T02:29:30Z

/run-all-tests

lysu · 2018-12-29T03:50:55Z

LGTM.

and it's a little stranger, delete by subquery eliminate unused column by projection, but delete multiple tables eliminate unused column in deleteMultiTablesByChunk.

tiancaiamao · 2018-12-29T06:58:45Z

/run-unit-test

…chema (pingcap#8805) 'delete from t where a in (select ...) or b in (select ...)' The correct schema should be the schema of t, rather than the schema of the join. Add a projection in this case.

tshqin · 2019-05-25T09:24:24Z

need a cherry pick to 2.1 version

…chema (pingcap#8805) 'delete from t where a in (select ...) or b in (select ...)' The correct schema should be the schema of t, rather than the schema of the join. Add a projection in this case.

…chema (#8805) (#10595) 'delete from t where a in (select ...) or b in (select ...)' The correct schema should be the schema of t, rather than the schema of the join. Add a projection in this case.

tiancaiamao added 4 commits December 24, 2018 23:45

planner: add a projection to fix 'delete from join' schema

288794f

'delete from t where a in (select ...) or b in (select ...)' The correct schema should be the schema of t, rather than the schema of the join. Add a projection in this case.

Merge branch 'master' into delete-schema

b61e780

add test

a5fd91d

add a test in binlog

e08a3ff

eurekaka reviewed Dec 25, 2018

View reviewed changes

winoros reviewed Dec 25, 2018

View reviewed changes

winoros added status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Dec 25, 2018

lysu reviewed Dec 25, 2018

View reviewed changes

sessionctx/binloginfo/binloginfo_test.go Show resolved Hide resolved

lysu reviewed Dec 25, 2018

View reviewed changes

tiancaiamao added 2 commits December 26, 2018 17:09

address comment

242e0f5

Merge branch 'master' into delete-schema

3a569d6

revert buildUpdate change

ac8e2d8

Merge branch 'master' into delete-schema

a630b4c

winoros mentioned this pull request Dec 28, 2018

Incorrect index after deleting from a table with index of multiple columns using a subquery, on latest master version (ADMIN CHECK TABLE failed) #8853

Closed

Merge branch 'master' into delete-schema

302c15b

lysu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 29, 2018

lysu approved these changes Dec 29, 2018

View reviewed changes

tiancaiamao added the status/all tests passed label Dec 29, 2018

Merge branch 'master' into delete-schema

216a43b

tiancaiamao merged commit 599b9eb into pingcap:master Dec 29, 2018

tiancaiamao deleted the delete-schema branch December 29, 2018 07:41

tiancaiamao mentioned this pull request May 25, 2019

planner/core,binloginfo: add a projection to fix 'delete from join' schema (#8805) #10595

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner/core,binloginfo: add a projection to fix 'delete from join' schema #8805

planner/core,binloginfo: add a projection to fix 'delete from join' schema #8805

tiancaiamao commented Dec 25, 2018 •

edited by c4pt0r

Loading

tiancaiamao commented Dec 25, 2018

eurekaka Dec 25, 2018

tiancaiamao Dec 26, 2018

tiancaiamao Dec 26, 2018

winoros left a comment

lysu Dec 25, 2018

zz-jason Dec 25, 2018

tiancaiamao Dec 26, 2018

winoros Dec 28, 2018

shenli commented Dec 26, 2018

tiancaiamao commented Dec 26, 2018

tiancaiamao commented Dec 26, 2018

tiancaiamao commented Dec 26, 2018

tiancaiamao commented Dec 26, 2018

eurekaka commented Dec 27, 2018

tiancaiamao commented Dec 29, 2018

lysu commented Dec 29, 2018

tiancaiamao commented Dec 29, 2018

tshqin commented May 25, 2019

planner/core,binloginfo: add a projection to fix 'delete from join' schema #8805

planner/core,binloginfo: add a projection to fix 'delete from join' schema #8805

Conversation

tiancaiamao commented Dec 25, 2018 • edited by c4pt0r Loading

What problem does this PR solve?

What is changed and how it works?

Check List

tiancaiamao commented Dec 25, 2018

eurekaka Dec 25, 2018

Choose a reason for hiding this comment

tiancaiamao Dec 26, 2018

Choose a reason for hiding this comment

tiancaiamao Dec 26, 2018

Choose a reason for hiding this comment

winoros left a comment

Choose a reason for hiding this comment

lysu Dec 25, 2018

Choose a reason for hiding this comment

zz-jason Dec 25, 2018

Choose a reason for hiding this comment

tiancaiamao Dec 26, 2018

Choose a reason for hiding this comment

winoros Dec 28, 2018

Choose a reason for hiding this comment

shenli commented Dec 26, 2018

tiancaiamao commented Dec 26, 2018

tiancaiamao commented Dec 26, 2018

tiancaiamao commented Dec 26, 2018

tiancaiamao commented Dec 26, 2018

eurekaka commented Dec 27, 2018

tiancaiamao commented Dec 29, 2018

lysu commented Dec 29, 2018

tiancaiamao commented Dec 29, 2018

tshqin commented May 25, 2019

tiancaiamao commented Dec 25, 2018 •

edited by c4pt0r

Loading