Add support for last_insert_id with MySQL

[mcronce]

I haven't dug in to figure out when exactly it was added to the protocol, but MySQL's (and MariaDB's) OK packet includes last_insert_id[1][2], and it's supported by mysql_common[3] going back as far as the repo's first commit[4]

I was also able to confirm that it does provide the correct value, at least with MariaDB 10.10.2 on my dev machine:

I did find #1011 and #1627 and the workaround (just calling MySQL's LAST_INSERT_ID() function) should work fine for my purpose, but wanted to also make a formal request to add support now that MySQL provides that value in the response :)

I'm not sure what you want as far as API for this, but with some guidance would be happy to take a whack at implementing and make a PR

[1] https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_ok_packet.html
[2] https://mariadb.com/kb/en/ok_packet/
[3] https://docs.rs/mysql_common/0.30.3/mysql_common/packets/struct.OkPacket.html#method.last_insert_id
[4] https://github.com/blackbeam/rust_mysql_common/blob/5c53f9ff25285e9f573b4c17ff4832ca7c672173/src/packets.rs#L348

[weiznich]

Generally speaking I'm open to such an addition as long as it contains documentation and tests and the API is somewhat clear.

It's an interesting question how such a API should look like. I given the provided documentation it looks like that this field is returned as part of the query response. It's not clear for me if it's always set (for example on select queries) or not (only on insert statements?). I do not have a specific proposal how such an API should look like, but I'm open to suggestions. API wise it might be simpler to include the LAST_INSERT_ID sql function instead.

[mcronce]

The issues with using LAST_INSERT_ID() are that it's racy (MySQL tracks it per-connection - even if both are in a transaction, if there are multiple threads/tasks sharing a connection, one can end up getting another's last insert ID) and that it means another roundtrip to the database. All that being said, I just ran a quick test - looking at the responses to SELECT and INSERT queries, it looks like the field is treated as Option<NonZeroU64> - at least one byte is always present, but at least with Wireshark's packet analysis, it hides the field if it's 0.

Looking at what other crates do, mysql and mysql_async both return Option<u64>[1][2], while sqlx opts to just return u64[3]. Personally, I would lean toward making it optional so the semantics are more clear. It also seems like it would be technically possible - since Diesel knows the nature of the query that was run (as long as it wasn't a raw query) - to only return a structure with last_insert_id when an INSERT query was executed; however, you would definitely have a better sense for the feasibility than I would :)

I think what I would propose is

• Add an associated type called something like ExecuteQueryResult to Connection
• For MysqlConnection, make a new MysqlQueryResult struct containing affected_rows and last_insert_id, with functions named the same to retrieve them
• For PgConnection and SqliteConnection, make ExecuteQueryResult a thin wrapper around usize with an affected_rows() function to return the inner usize (IMO this keeps the API more consistent across database back-ends)
• Maybe add a trait with affected_rows() and implement it for all of these, so that code that wants to be generic over the database back end are able to?
• Change RunQueryDsl::execute()'s return type to QueryResult<Conn::ExecuteQueryResult>

This is, obviously, a breaking change, though. Is there anything you'd recommend to to make it non-breaking?

[1] https://docs.rs/mysql/23.0.1/mysql/struct.QueryResult.html#method.last_insert_id
[2] https://docs.rs/mysql_async/0.32.1/mysql_async/struct.QueryResult.html#method.last_insert_id
[3] https://docs.rs/sqlx/0.6.3/sqlx/mysql/struct.MySqlQueryResult.html#method.last_insert_id

[mcronce]

Just add another function - I like it. WDYT about exposing .execute_with_returning_id(MysqlConnection) on InsertStatement? That would allow calling code to replace something along these lines:

diesel::insert_into(whatever::table)
    .values(CreateWhatever{...})
    .execute(db)?;

let id: u64 = diesel::select(last_insert_id()).first(db)?; // last_insert_id() defined by sql_function!()

with

let id = diesel::insert_into(whatever::table)
    .values(CreateWhatever{...})
    .execute_with_returning_id(db)?;

(As long as db is a MysqlConnection, obviously)

[weiznich]

That would be fine as well

[mcronce]

Cool. I'll take a whack at implementing. Thanks for the guidance!

[lopter]

Just wanted to say I also like the suggested API. The MySQL backend feels unusable to me without it, for the reason stated above, combined with the fact that it does not seem that you can use UUID columns either:

the trait `diesel::Queryable<BreakpointsUuidUuid, Mysql>` is not implemented for `Uuid`

[weiznich]

@lopter As written above: We are happy to accept contributions in this area.

Your uuid issues sounds totally unrelated to what's being discussed here. If you have questions about that feel free to open a new discussion that includes all the relevant details to understand your problem. Based on the provided information that is not possible.