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

Match MySQL's LAST_INSERT_ID behaviour when no rows are inserted / updated #15699

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 68 additions & 0 deletions go/test/endtoend/vtgate/queries/dml/insert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,74 @@ func TestInsertSelectUnshardedUsingSharded(t *testing.T) {
}
}

// MySQL returns 0 as the `LastInsertId` on `... ON DUPLICATE KEY UPDATE ...` queries
// when no new row is inserted. This test verifies that Vitess behaves the same way.
func TestInsertShardedWithOnDuplicateKeyNoInserts(t *testing.T) {
mcmp, closer := start(t)
defer closer()

mcmp.Exec("insert into last_insert_id_test (id, sharding_key, user_id, reason) values (1, '1:1:1', 1, 'foo'), (2, '2:2:2', 2, 'bar')")

// Bump the sequence value so the sequence accounts for the 2 explicit inserts above.
utils.Exec(t, mcmp.VtConn, "SELECT NEXT 2 VALUES FROM uks.last_insert_id_test_seq")

// First test case, insert a row that already exists, and don't actually change any column at all.
query := "insert into last_insert_id_test (sharding_key, user_id, reason) values ('1:1:1', 1, 'foo') on duplicate key update reason = reason"

mysqlResult := utils.Exec(t, mcmp.MySQLConn, query)
// no new row inserted, so insert id should be 0.
assert.Equal(t, uint64(0), mysqlResult.InsertID)
// no row was modified, so rows affected should be 0.
assert.Equal(t, uint64(0), mysqlResult.RowsAffected)

vitessResult := utils.Exec(t, mcmp.VtConn, query)
assert.Equal(t, mysqlResult.RowsAffected, vitessResult.RowsAffected)
assert.Equal(t, mysqlResult.InsertID, vitessResult.InsertID)

// Second test case, insert a row that already exists, and change a column on the existing row.
query = "insert into last_insert_id_test (sharding_key, user_id, reason) values ('1:1:1', 1, 'bar') on duplicate key update reason = VALUES(reason)"

mysqlResult = utils.Exec(t, mcmp.MySQLConn, query)
// a row was modified, so insert id should match the auto increment column value of the modified row
assert.Equal(t, uint64(1), mysqlResult.InsertID)
// one row was modified, so rows affected should be 2.
assert.Equal(t, uint64(2), mysqlResult.RowsAffected)

vitessResult = utils.Exec(t, mcmp.VtConn, query)
assert.Equal(t, mysqlResult.RowsAffected, vitessResult.RowsAffected)
// Vitess can't return the `auto_increment` value of the last updated row, but it returns a value larger than 0.
assert.Greater(t, vitessResult.InsertID, uint64(0))
Comment on lines +401 to +435
Copy link
Member

Choose a reason for hiding this comment

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

We do not need to test with way, mcmp.Exec can validate all of this.
With latest changes to this test package we can add new compare option

assert.NotEqual(t, mysqlResult.InsertID, vitessResult.InsertID)

// Second test case, insert multiple rows, all of which already exist, and change a column on existing rows.
query = "insert into last_insert_id_test (sharding_key, user_id, reason) values ('2:2:2', 2, 'qux'), ('1:1:1', 1, 'baz') on duplicate key update reason = VALUES(reason)"

mysqlResult = utils.Exec(t, mcmp.MySQLConn, query)
// two rows were modified, so insert id will match the auto increment column value of the last modified row
assert.Equal(t, uint64(1), mysqlResult.InsertID)
// two rows were modified, so rows affected should be 2.
assert.Equal(t, uint64(4), mysqlResult.RowsAffected)

vitessResult = utils.Exec(t, mcmp.VtConn, query)
assert.Equal(t, mysqlResult.RowsAffected, vitessResult.RowsAffected)
// Vitess can't return the `auto_increment` value of the last updated row, but it returns a value larger than 0.
assert.Greater(t, vitessResult.InsertID, uint64(0))
assert.NotEqual(t, mysqlResult.InsertID, vitessResult.InsertID)

// Third test case, insert multiple rows, some of which already exist, and change a column on existing rows.
query = "insert into last_insert_id_test (sharding_key, user_id, reason) values ('3:3:3', 3, 'apa'), ('2:2:2', 2, 'bpa'), ('1:1:1', 1, 'cpa') on duplicate key update reason = VALUES(reason)"

mysqlResult = utils.Exec(t, mcmp.MySQLConn, query)
// a new row was inserted, so insert id should match the auto increment column value of the new row
assert.Equal(t, uint64(7), mysqlResult.InsertID)
// one row was inserted, two rows were modified, so rows affected should be 5.
assert.Equal(t, uint64(5), mysqlResult.RowsAffected)

vitessResult = utils.Exec(t, mcmp.VtConn, query)
assert.Equal(t, mysqlResult.RowsAffected, vitessResult.RowsAffected)
assert.Equal(t, mysqlResult.InsertID, vitessResult.InsertID)
}

func TestRedactDupError(t *testing.T) {
mcmp, closer := start(t)
defer closer()
Expand Down
10 changes: 10 additions & 0 deletions go/test/endtoend/vtgate/queries/dml/sharded_schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,13 @@ create table lkp_mixed_idx
keyspace_id varbinary(20),
primary key (lkp_key)
) Engine = InnoDB;

create table last_insert_id_test
(
id bigint NOT NULL AUTO_INCREMENT,
user_id bigint,
reason varchar(20),
sharding_key varchar(20),
primary key (id),
unique (user_id, sharding_key)
)
12 changes: 11 additions & 1 deletion go/test/endtoend/vtgate/queries/dml/unsharded_schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,19 @@ create table u_tbl
primary key (id)
) Engine = InnoDB;

create table last_insert_id_test_seq
(
id int default 0,
next_id bigint default null,
cache bigint default null,
primary key (id)
) comment 'vitess_sequence' Engine = InnoDB;

insert into user_seq(id, next_id, cache)
values (0, 1, 1000);
insert into auto_seq(id, next_id, cache)
values (0, 666, 1000);
insert into mixed_seq(id, next_id, cache)
values (0, 1, 1000);
values (0, 1, 1000);
insert into last_insert_id_test_seq(id, next_id, cache)
values (0, 1, 1000);
17 changes: 16 additions & 1 deletion go/test/endtoend/vtgate/queries/dml/vschema.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
"hash": {
"type": "hash"
},
"xxhash": {
"type": "xxhash"
},
"num_vdx": {
"type": "consistent_lookup_unique",
"params": {
Expand Down Expand Up @@ -188,6 +191,18 @@
"name": "hash"
}
]
},
"last_insert_id_test": {
"auto_increment": {
"column": "id",
"sequence": "uks.last_insert_id_test_seq"
},
"column_vindexes": [
{
"column": "sharding_key",
"name": "xxhash"
}
]
}
}
}
}
13 changes: 12 additions & 1 deletion go/vt/vtgate/engine/insert.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,20 @@ func (ins *Insert) executeInsertQueries(
return nil, vterrors.Aggregate(errs)
}

// If this insert used auto increment values from a sequence, we need to set the `last_insert_id` value.
if insertID != 0 {
result.InsertID = insertID
if result.RowsAffected > 0 {
// If at least one row was affected, we set the `last_insert_id` value to the lowest reserved sequence id.
//
// This does not match the behaviour of MySQL in case where no new rows where inserted (but one or more rows were updated
// via `ON DUPLICATE KEY UPDATE`), where the `last_insert_id` value is set to the `auto_increment` column value.
result.InsertID = insertID
} else {
// If no rows were inserted or updated, clear the `last_insert_id` value.
result.InsertID = 0
}
}

Comment on lines +177 to +190
Copy link
Member

Choose a reason for hiding this comment

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

this is not true. I tried it on mysql

mysql> insert into at(col) values (20);
Query OK, 1 row affected (0.00 sec)

mysql> select * from at where col = 20;
+----+------+
| id | col  |
+----+------+
| 10 |   20 |
+----+------+
1 row in set (0.00 sec)

mysql> insert into at(col) values (20);
ERROR 1062 (23000): Duplicate entry '20' for key 'at.col'

mysql> insert into at(col) values (20) on duplicate key update col = 40;
Query OK, 2 rows affected (0.00 sec)

mysql> select * from at where col = 40;
+----+------+
| id | col  |
+----+------+
| 10 |   40 |
+----+------+
1 row in set (0.00 sec)

mysql> select @@last_insert_id;
+------------------+
| @@last_insert_id |
+------------------+
|               10 |
+------------------+
1 row in set (0.00 sec)

If you follow through this sample test. The last_insert_id remained the old value of 10 which was the last insert id when 20 for col was inserted into the table.

Copy link
Member

Choose a reason for hiding this comment

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

We should never reset the Insert ID value. I am not sure of the case when it is done.

Copy link
Member

Choose a reason for hiding this comment

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

another case where row_affected is 0

mysql> insert into at(col) select 10 from dual where 1!=1;
Query OK, 0 rows affected (0.01 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> select @@last_insert_id;
+------------------+
| @@last_insert_id |
+------------------+
|               10 |
+------------------+
1 row in set (0.00 sec)

return result, nil
}

Expand Down