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

arthurschreiber · 2024-04-11T11:01:59Z

Description

This pull request fixes the behaviour of LAST_INSERT_ID when inserting into a sharded table that uses a sequence to fill an auto increment column.

In case an INSERT ... ON DUPLICATE KEY UPDATE ... statement does neither insert nor update any rows in the target table, Vitess should set LAST_INSERT_ID to 0.

Note: I think this will only work on connections that don't specify the CLIENT_FOUND_ROWS flag. 🤔 I will add some additional test cases to verify this.

Related Issue(s)

Fixes #15696

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

… behaviour when performing `INSERT ... ON DUPLICATE KEY UPDATE ...` statements. Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

…ged. Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

vitess-bot · 2024-04-11T11:02:02Z

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

codecov · 2024-04-11T11:07:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.42%. Comparing base (0912690) to head (7bee257).
Report is 571 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15699      +/-   ##
==========================================
+ Coverage   68.40%   68.42%   +0.02%     
==========================================
  Files        1556     1556              
  Lines      195121   195124       +3     
==========================================
+ Hits       133468   133511      +43     
+ Misses      61653    61613      -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

harshit-gangal · 2024-05-08T12:14:32Z

I tried this on mysql and looks like it retained the old value and not reset to 0.

mysql> create table at(id bigint auto_increment, col bigint, unique key(col), primary key(id));
Query OK, 0 rows affected (0.03 sec)

mysql> insert into at(col) values (1), (2), (3), (4);
Query OK, 4 rows affected (0.01 sec)
Records: 4  Duplicates: 0  Warnings: 0

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

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

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

mysql> select * from at;
+----+------+
| id | col  |
+----+------+
|  1 |    1 |
|  2 |    2 |
|  4 |    4 |
|  3 |   10 |
+----+------+
4 rows in set (0.01 sec)

harshit-gangal · 2024-05-08T13:38:01Z

go/test/endtoend/vtgate/queries/dml/insert_test.go

+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))


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

harshit-gangal · 2024-05-08T13:48:00Z

go/vt/vtgate/engine/insert.go

+	// 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
+		}
 	}
+


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.

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

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)

github-actions · 2024-06-08T01:30:33Z

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.