Handle `last_insert_id()` and `database()` in vtgate #5451

systay · 2019-11-22T13:12:13Z

sougou

I think the builder APIs don't need to be changed. This is because the primitiveBuilder can keep track of the flag. We can see it as analogous to the relationship that Plan has with Instruction. Essentially, Rewrite can just set this flag into primitiveBuilder, and then our job is done.

I think this will lead to a much smaller code change.

sougou · 2019-12-11T23:50:29Z

go/vt/vtgate/endtoend/last_insert_id_test.go

+ exec(t, conn, "insert into t1_last_insert_id(id1) values(42)")
+
+ qr := exec(t, conn, "select last_insert_id()")
+ if got, want := fmt.Sprintf("%v", qr.Rows), `[[INT64(1)]]`; got != want {


I'm surprised you didn't use assert :)

sougou · 2019-12-11T23:54:09Z

go/vt/vtgate/planbuilder/builder.go

+}
+
+// lastInsertIDTracker implements common functionality amongst most builders
+type lastInsertIDTracker struct {


This can just be type lastInsertIDTracker bool.

sougou · 2019-12-11T23:56:49Z

go/vt/vtgate/planbuilder/expression_rewriting.go

+
+ for _, node := range matchingNodes {
+ in = sqlparser.ReplaceExpr(in, node, bindVarExpression())
+ }


As discussed, this won't work for subqueries, but we need to think of a more elaborate solution for those.

I added subquery tests, and they seem to work. Not sure how that works

sougou

This looks good. Found couple of issues that could be addressed as a follow-up PR. Do you want to do that or rework this PR itself?

sougou · 2019-12-13T05:51:19Z

go/vt/vtgate/planbuilder/builder.go

 case *sqlparser.Insert:
- plan.Instructions, err = buildInsertPlan(stmt, vschema)
+ instruction, err = buildInsertPlan(stmt, vschema)


It just occurred to me that these statements can also use last_insert_id as well as database:

mysql> create table a(id int auto_increment, val int, primary key(id)); Query OK, 0 rows affected (0.04 sec) mysql> insert into a(val) values(10); Query OK, 1 row affected (0.01 sec) mysql> select last_insert_id(); +------------------+ | last_insert_id() | +------------------+ | 1 | +------------------+ 1 row in set (0.01 sec) mysql> insert into a(val) values(last_insert_id()); Query OK, 1 row affected (0.01 sec) mysql> select * from a; +----+------+ | id | val | +----+------+ | 1 | 10 | | 2 | 1 | +----+------+ 2 rows in set (0.00 sec)

sougou · 2019-12-13T05:58:58Z

go/vt/vtgate/planbuilder/expression_rewriting.go

+)
+
+// RewriteResult contains the rewritten expression and meta information about it
+type RewriteResult struct {


Optional improvement: we probably don't need this struct: Rewrite could directly update primitiveBuilder instead. I can't think of a situation where we'd do a rewrite without updating primitiveBuilder.

I like having this intermediate struct to make it easy to write unit tests for the expression rewriting without having to first create a primitiveBuilder

Signed-off-by: Andres Taylor <andres@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Andres Taylor <andres@planetscale.com>

sougou

Two final nits.

sougou · 2020-01-07T21:44:42Z

go/vt/vtgate/planbuilder/builder.go

 case *sqlparser.Delete:
- plan.Instructions, err = buildDeletePlan(stmt, vschema)
+ instruction, err = buildDeletePlan(stmt, vschema)


i think delete could also need last insert id.

sougou · 2020-01-07T21:46:44Z

go/vt/vtgate/planbuilder/expression_rewriting.go

+
+// Rewrite will rewrite an expression. Currently it does the following rewrites:
+// - `last_insert_id()` => `:vtlastid`
+// - `database()` => `:vtdbname`


Need to update these to use the new bind var names.

Signed-off-by: Andres Taylor <andres@planetscale.com>

systay added the Status: Work in progress label Nov 22, 2019

systay requested a review from sougou as a code owner November 22, 2019 13:12

systay force-pushed the last_insert_id branch 4 times, most recently from 9440d6f to 88e44ff Compare December 3, 2019 14:47

systay force-pushed the last_insert_id branch from 01e7718 to 88e44ff Compare December 10, 2019 09:09

rohit-nayak-ps force-pushed the last_insert_id branch from 88e44ff to 59acdbf Compare December 10, 2019 12:03

sougou reviewed Dec 12, 2019

View reviewed changes

systay removed the Status: Work in progress label Dec 12, 2019

systay force-pushed the last_insert_id branch 2 times, most recently from 1c15506 to daa439e Compare December 12, 2019 10:37

systay changed the title ~~last_insert_id()~~ Handle last_insert_id() and database() in vtgate Dec 12, 2019

systay changed the title ~~Handle last_insert_id() and database() in vtgate~~ Handle last_insert_id() and database() in vtgate Dec 12, 2019

sougou reviewed Dec 13, 2019

View reviewed changes

systay force-pushed the last_insert_id branch 4 times, most recently from a0dcf1d to 790cfef Compare December 16, 2019 05:55

systay and others added 11 commits January 6, 2020 11:27

Added LastInsertId to the grpc Session object

ad9c353

Signed-off-by: Andres Taylor <andres@planetscale.com>

select last_insert_id() working for non-streaming case

255e321

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Fixed tests

e8604e9

Signed-off-by: Andres Taylor <andres@planetscale.com>

Only add last insert id bind var if needed

f6c0c4d

Signed-off-by: Andres Taylor <andres@planetscale.com>

Make sure to handle last_insert_id in subqueries

b6796a4

Signed-off-by: Andres Taylor <andres@planetscale.com>

Rewrite database() expressions

5d24355

Signed-off-by: Andres Taylor <andres@planetscale.com>

Fix shared state between tests

ca3d99a

Signed-off-by: Andres Taylor <andres@planetscale.com>

Make sure to add column aliases when needed

46c5793

Signed-off-by: Andres Taylor <andres@planetscale.com>

Small refactoring

9138f44

Signed-off-by: Andres Taylor <andres@planetscale.com>

Handle last_insert_id() on insert statements as well

e56dd20

Signed-off-by: Andres Taylor <andres@planetscale.com>

Make sure to handle last_insert_id for union

0134933

Signed-off-by: Andres Taylor <andres@planetscale.com>

Make sure to handle last_insert_id for update

a3acc14

Signed-off-by: Andres Taylor <andres@planetscale.com>

systay force-pushed the last_insert_id branch from 790cfef to a3acc14 Compare January 6, 2020 19:29

sougou reviewed Jan 7, 2020

View reviewed changes

Make sure to rewrite last_insert_id() for DELETEs

a40f4ce

Signed-off-by: Andres Taylor <andres@planetscale.com>

sougou approved these changes Jan 7, 2020

View reviewed changes

sougou merged commit 219b34f into vitessio:master Jan 7, 2020

ajm188 mentioned this pull request Mar 10, 2020

Slack sync upstream 2020 03 10.r0 tinyspeck/vitess#147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `last_insert_id()` and `database()` in vtgate #5451

Handle `last_insert_id()` and `database()` in vtgate #5451

systay commented Nov 22, 2019 •

edited

Loading

sougou left a comment

sougou Dec 11, 2019

sougou Dec 11, 2019

sougou Dec 11, 2019

systay Dec 12, 2019 •

edited

Loading

sougou left a comment

sougou Dec 13, 2019

sougou Dec 13, 2019

systay Dec 13, 2019

sougou left a comment

sougou Jan 7, 2020

sougou Jan 7, 2020

Handle last_insert_id() and database() in vtgate #5451

Handle last_insert_id() and database() in vtgate #5451

Conversation

systay commented Nov 22, 2019 • edited Loading

sougou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

systay Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

sougou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sougou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Handle `last_insert_id()` and `database()` in vtgate #5451

Handle `last_insert_id()` and `database()` in vtgate #5451

systay commented Nov 22, 2019 •

edited

Loading

systay Dec 12, 2019 •

edited

Loading