asyncer-io / r2dbc-mysql

Reactive Relational Database Connectivity for MySQL. The official successor to mirromutth/r2dbc-mysql(dev.miku:r2dbc-mysql).
https://r2dbc.io
Apache License 2.0
196 stars 21 forks source link

Add `RETURNING` support for MariaDB 10.5.1+ #201

Closed mirromutth closed 8 months ago

mirromutth commented 8 months ago

Motivation:

Add RETURNING clause support for MariaDB 10.5.1+ .

See also #177 .

Modification:

Add RETURNING clause in MySqlStatement#returnGeneratedValues and query message packets.

Result:

For MariaDB 10.5.1 and above, MySqlStatement#returnGeneratedValues will use RETURNING clause instead of LAST_INSERT_ID.

Notice:

For MariaDB 10.5.0 and below, MySqlStatement#returnGeneratedValues will still use LAST_INSERT_ID due to these versions do not support RETURNING clause.

mirromutth commented 8 months ago

@jchrys

Currently, it is simplify to add RETURNING following original SQL statement without checking.

Perhaps the statement parser needs to be rewritten to support checking whether the statement supports RETURNING (INSERT/REPLACE/DELETE) or already includes RETURNING, like r2dbc-mariadb does?

jchrys commented 8 months ago

@jchrys

Currently, it is simplify to add RETURNING following original SQL statement without checking.

Perhaps the statement parser needs to be rewritten to support checking whether the statement supports RETURNING (INSERT/REPLACE/DELETE) or already includes RETURNING, like r2dbc-mariadb does?

I think the current change is suitable for merging, as MariaDB will naturally return a syntax error in case of incorrect RETURNING usage. I agree that enhancing the parser, akin to what's done in r2dbc-mariadb, better be next step.

mirromutth commented 8 months ago

Columns should not be quoted, it can be an expression. e.g. INSERT INTO ... RETURNING CURRENT_TIMESTAMP.

If we quoted it, like in the example above it would be INSERT INTO ... RETURNING `CURRENT_TIMESTAMP`, which would be confusing. User wants the result of the CURRENT_TIMESTAMP expression, not a column named CURRENT_TIMESTAMP.

Keep columns raw and let ORM/QueryBuilder handle it.

mirromutth commented 8 months ago

@jchrys It is foreseeable that this PR will change the behavior of returnGenerateValues when connecting to MariaDB 10.5.1+, maybe we should merge it with the next minor version?

For example, if a user is using returnGenerateValues without arguments, we will return a Result with one and only one row, which contains a LAST_INSERT_ID column. This modification will change the number of result rows (maybe more, maybe 0 by INSERT ... SELECT ... RETURNING ...), and the row metadata will contain all data columns in the table.

jchrys commented 8 months ago

@jchrys It is foreseeable that this PR will change the behavior of returnGenerateValues when connecting to MariaDB 10.5.1+, maybe we should merge it with the next minor version?

For example, if a user is using returnGenerateValues without arguments, we will return a Result with one and only one row, which contains a LAST_INSERT_ID column. This modification will change the number of result rows (maybe more, maybe 0 by INSERT ... SELECT ... RETURNING ...), and the row metadata will contain all data columns in the table.

I agree. 😄