From #3137883-72: Deprecate passing a StatementInterface object to Connection::query

We can also open a follow-up to explore the possible performance improvements of using RETURNING id

See https://stackoverflow.com/questions/2944297/postgresql-function-for-last...

CommentFileSizeAuthor
#8 3185399-8.patch4.46 KBalexpott
#8 4-8-interdiff.txt795 bytesalexpott
#4 3185399-4.patch4.31 KBalexpott
#4 3-4-interdiff.txt849 bytesalexpott
#3 3185399-3.patch4.33 KBalexpott
#3 2-3-interdiff.txt1.45 KBalexpott
#2 3185399-2.patch3.98 KBalexpott

Comments

mondrake created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3185399-2.patch3.98 KB

This seems nice.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2-3-interdiff.txt1.45 KB
new 3185399-3.patch4.33 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3-4-interdiff.txt849 bytes
new 3185399-4.patch4.31 KB

Less variables.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Unless I'm mistaken, this is reducing PgSql test run times by 3 to 4 minutes?

mondrake’s picture

mondrake
Location 🇮🇹
commented

I do not think we need a CR here since this is an implementation detail.

daffie’s picture

daffie commented
Status: Reviewed & tested by the community » Needs work

I had my doubts about the if we needed the try-catch part, but I saw in the patch from comment #2 that it did not have the try-catch part and the testbot did not liked that patch.
There is already a lot of inserts being run by the testbot, therefore I do not see the need for extra tests.
To me it is almost RTBC, I have just 1 minor nitpick:

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
@@ -149,14 +129,27 @@ public function __toString() {
+        $query .= ' RETURNING ' . $table_information->serial_fields[0];

We could add a comment that adding this to the insert statement will have the effect that the insert query will return the last inserted id. As this is not standard SQL.

@mondrake: Good find!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 4-8-interdiff.txt795 bytes
new 3185399-8.patch4.46 KB

@daffie fwiw I found this in reviewing #3137883-69: Deprecate passing a StatementInterface object to Connection::query

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All changes in the patch look good to me.
For me it is RTBC.

@alexpott: Then my apologies, its your good find!

  • catch committed 340ff83 on 9.2.x 
    Issue #3185399 by alexpott, mondrake, daffie: Use RETURNING to determine...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 340ff83 and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.