Don't call last_insert_id if not needed #1403

tyt2y3 · 2023-01-18T12:21:00Z

src/executor/insert.rs

nitnelave · 2023-01-18T12:27:41Z

src/executor/insert.rs

-            let last_insert_id = db.execute(statement).await?.last_insert_id();
-            ValueTypeOf::<A>::try_from_u64(last_insert_id).ok()
+
+    enum QueryOrExecResult {


It's quite awkward to have an enum where you already know the type it has in each branch. Can we refactor otherwise? To get rid of the unreachable.

I think the alternative is that we split the implementation into two versions.

nitnelave · 2023-01-18T12:30:22Z

src/executor/insert.rs

-            None => return Err(DbErr::UnpackInsertId),
-        },
-    };
+        Some(value_tuple) => Ok(FromValueTuple::from_value_tuple(value_tuple)),


It seems to be we just have to add a db.execute(statement).await? here rather than introduce an entirely new type.

So, if we add the execute statement in the Some branch, we can inline the top of the function in the None branch, further reducing the amount of work done in case we provide the primary key. Each branch would have its own singular await point.

What's the rationale for reducing the number of await points?

nitnelave · 2023-01-18T12:37:42Z

src/executor/insert.rs

+                match insert_result {
+                    QueryOrExecResult::Query(row) => {
+                        let cols = PrimaryKey::<A>::iter()
+                            .map(|col| col.to_string())


That might be a bigger change, but I feel that try_get_many should accept a slice of AsRef<str> and IdenStatic should implement AsRef<str> so we don't have to do this whole conversion at all.

I agree, but that might be another PR

tyt2y3 · 2023-01-18T12:53:24Z

I don't see issues/1357 being run, but I assure it works on my machine(TM)

tyt2y3 · 2023-01-18T13:10:18Z

Refactored. How about now?

nitnelave · 2023-01-18T13:12:33Z

src/executor/insert.rs

-            })?;
-            res.try_get_many("", cols.as_ref()).ok()
+
+    let last_insert_id = if db.support_returning() {


That's much better! Do we absolutely care about RecordNotInserted if the primary key is provided? How can we detect it if the db doesn't support returning?

If we don't especially care about it, we could rewrite it as :

match (primary_key, db.support_returning()) { (Some(value_tuple), _) => {db.execute(); Ok(FromValueTuple)}, (None, true) =>{db.query_all() ... }, (None, false) => {res = db.execute(); res.last_insert_id()}, }

WDYT?

Yes, I think we care. Because that's the case when we inserted a duplicate record?

How can we detect it if the db doesn't support returning

Now it's a compile-time hardcoded constant, with SQLite being false. Ideally, SQLite support should be determined by its version detected runtime.

No, I mean how can we detect that the record was not inserted if the db doesn't support returning?

Umm, I see what you mean

Could we use https://stackoverflow.com/questions/6551903/getting-the-number-of-affected-rows-for-a-sqlite-statement-using-the-c-api ?

If we can't detect that if the db doesn't support returning, then the question is should we align the behaviour or not?

For sqlite, by calling sqlite3_changes() (or by selecting changes() in the query), we can get the number of rows affected. With that, we can see if it's 0 (didn't insert anything) or >0 (inserted at least one thing). That wouldn't catch the case where we insert >1 value but only some failed, but for single value inserts, it would already be helpful.

I think the code as of "Refactor" can be released in 0.10 and further changes should go into 0.11

Sounds good. Just FYI, I tested select changes(); on SQlite3, it seems to do what we want: after a single insert, it'll return 1 if the insert was successful, and 0 otherwise. And it's scoped to the DB connection as well, so it should be usable even in async contexts.

But anyway, inserting a single duplicate row returns an error, so probably the await? should trigger a return, no?

Yes. It should be a UNIQUE CONSTRAINT ERROR unless if we specified ON CONFLICT DO NOTHING.

nitnelave · 2023-01-18T14:28:04Z

src/executor/insert.rs

+    let last_insert_id = match (primary_key, db.support_returning()) {
+        (Some(value_tuple), _) => {
+            let res = db.execute(statement).await?;
+            if res.rows_affected() == 0 {


oh, if we have that, that's great!

nitnelave

Looks great!

billy1624 · 2023-01-19T03:16:56Z

src/executor/insert.rs

-    let last_insert_id_opt = match db.support_returning() {
-        true => {
+
+    let last_insert_id = match (primary_key, db.support_returning()) {


Nice rewrite!!

billy1624 · 2023-01-19T03:17:52Z

src/error.rs

+    #[error("None of the records are being inserted")]
+    RecordNotInserted,


I think this is a breaking change that should goes into 0.11.x

It's actually never released in 0.10

Backportting #1021 and #1403 on 0.10.x

tyt2y3 added 3 commits January 18, 2023 19:06

Don't call last_insert_id if not needed

e5b69eb

[issues] test case

f50dc1d

Refactor

07d5f78

tyt2y3 mentioned this pull request Jan 18, 2023

Don't call auto_increment for columns where it's not set #1394

Closed

tyt2y3 changed the title ~~Issues/1357~~ Don't call last_insert_id if not needed Jan 18, 2023

nitnelave suggested changes Jan 18, 2023

View reviewed changes

Refactor 2

a465d1e

nitnelave reviewed Jan 18, 2023

View reviewed changes

Use rows_affected when DB does not support returning

03207fb

nitnelave approved these changes Jan 18, 2023

View reviewed changes

tyt2y3 force-pushed the issues/1357 branch from 6ec7dae to 03207fb Compare January 18, 2023 14:15

tyt2y3 requested a review from nitnelave January 18, 2023 14:20

nitnelave reviewed Jan 18, 2023

View reviewed changes

nitnelave approved these changes Jan 18, 2023

View reviewed changes

tyt2y3 mentioned this pull request Jan 18, 2023

Backportting #1021 and #1403 on 0.10.x #1404

Merged

Add testcase

5cdcdbf

billy1624 approved these changes Jan 19, 2023

View reviewed changes

tyt2y3 force-pushed the issues/1357 branch from 6ec9337 to 5cdcdbf Compare January 19, 2023 03:18

billy1624 assigned tyt2y3 Jan 19, 2023

tyt2y3 added a commit that referenced this pull request Jan 19, 2023

Merge pull request #1404 from SeaQL/1357-backport

7d856e7

Backportting #1021 and #1403 on 0.10.x

tyt2y3 merged commit 0557e7b into master Jan 19, 2023

tyt2y3 deleted the issues/1357 branch January 19, 2023 06:09

tyt2y3 mentioned this pull request Feb 3, 2023

using insert_many and on_conflict ignore, panics on empty unwrap #1193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't call last_insert_id if not needed #1403

Don't call last_insert_id if not needed #1403

tyt2y3 commented Jan 18, 2023

nitnelave Jan 18, 2023

tyt2y3 Jan 18, 2023 •

edited

nitnelave Jan 18, 2023

nitnelave Jan 18, 2023

nitnelave Jan 18, 2023

tyt2y3 Jan 18, 2023

tyt2y3 commented Jan 18, 2023

tyt2y3 commented Jan 18, 2023

nitnelave Jan 18, 2023

tyt2y3 Jan 18, 2023 •

edited

nitnelave Jan 18, 2023

tyt2y3 Jan 18, 2023

nitnelave Jan 18, 2023

tyt2y3 Jan 18, 2023 •

edited

nitnelave Jan 18, 2023

tyt2y3 Jan 18, 2023

nitnelave Jan 18, 2023

tyt2y3 Jan 18, 2023

nitnelave Jan 18, 2023

nitnelave left a comment

billy1624 Jan 19, 2023

billy1624 Jan 19, 2023

tyt2y3 Jan 19, 2023 •

edited

		#[error("None of the records are being inserted")]
		RecordNotInserted,

Don't call last_insert_id if not needed #1403

Don't call last_insert_id if not needed #1403

Conversation

tyt2y3 commented Jan 18, 2023

Choose a reason for hiding this comment

tyt2y3 Jan 18, 2023 • edited

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

Choose a reason for hiding this comment

tyt2y3 commented Jan 18, 2023

tyt2y3 commented Jan 18, 2023

Choose a reason for hiding this comment

tyt2y3 Jan 18, 2023 • edited

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

tyt2y3 Jan 18, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitnelave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyt2y3 Jan 19, 2023 • edited

Choose a reason for hiding this comment

tyt2y3 Jan 18, 2023 •

edited

tyt2y3 Jan 18, 2023 •

edited

tyt2y3 Jan 18, 2023 •

edited

tyt2y3 Jan 19, 2023 •

edited