Fix last insert ID consistency accross drivers by deeky666 · Pull Request #2648 · doctrine/dbal

deeky666 · 2017-02-08T09:49:51Z

This one came while trying to fix the last remaining testsuite failure on sqlsrv.
Retrieving the last insert ID for non-sequences is highly unreliable accross the driver implementations (as also stated in the PDO docs). This PR makes all drivers behave the same (which support this feature) with two exceptions (shown later on).

To demonstrate some inconsistency examples:

MySQL drivers reset the last insert ID to 0 after each non-insert query
mysqli and sqlanywhere return int instead of string
sqlsrv returns null instead of 0 if no INSERT was been executed, yet
ibm_db2 returns `` instead of 0 if no INSERT was been executed, yet
sqlsrv only records the last insert ID on prepared statements (not on exec) because of custom implementation via object
pdo_pgsql resets the last insert ID to 0 after dropping the particular table the last insert ID was retrieved from
oci8 always returns false instead of 0 (not supported on Oracle)
Surely others I forgot right now...

Looking at our interface for lastInsertId() drivers always have to return string, no word about exceptions or what to return if not supported or no ID available.
To stick to the contract and finding a reasonable common denominator, I chose MySQL as a base for returning "I don't know the value". MySQL returns 0 in this case. Otherwise MySQL behaves different when it comes to keeping the last insert ID for the connection (resets to 0 while the others don't).

Two exceptions that cannot be made consistent:

SQLite uses a different concept when it comes to reusing rolled back insert IDs (see tests)
pdo_pgsql returns 4294967295 if no insert ID is available yet (weird thing, don't know what PHP intending here)

I tried to cover the most important situations in the tests, hope I did not forget anything.

deeky666 · 2017-02-08T09:54:14Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php

            return $stmt->fetchColumn();
        }

-        return $this->lastInsertId->getId();


I wanted to remove the custom object altogether but unfortunately the property is protected on the connection so I kept it here for BC.

Ocramius · 2017-02-08T09:57:07Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

+
+        // The last insert ID is reset to "0" in certain situations by some implementations,
+        // therefore we keep the previously set insert ID locally.
+        if ($lastInsertId) {


Is an empty string a valid generated identifier? I think this conditional should be checking for '0' !== $lastInsertId

Also introduces the issue in scenarios where sequences actually contain '0' as valid insert ID at some point.

These are just edge cases that should really not be considered, IMO, but it's good to comment on them, at least.

@Ocramius this PR does not touch sequence ID retrieval (which requires passing $name). So this is all about regular "autoincrement" via identity columns. I am not aware of the possibility to have an identity column generate an empty string or 0 (they start at 1, don't they)?

Yes, sequences are indeed not affected, but we still need to consider scenarios where something like an AUTO_INCREMENT started at -10, and then at some point reaches 0. We can't handle that, but we have to at least specify that to users (we only support positive insert identifiers, starting from 1, while all, other scenarios lead to edge cases)

Ocramius · 2017-02-08T10:03:14Z

While the fix is perfectly valid, and our API documents the behavior correctly, people may (!!!) currently rely on the last insert id being an int.

Therefore, this will go only to 2.6 due to regression risks in consumer code.

Ocramius · 2017-02-08T10:05:21Z

I restarted the CI run, but postgresql + PHP nightly seem to have some issues with fetching the lastval

deeky666 · 2017-02-09T12:05:58Z

@Ocramius could you do me the favour and have a look at the current approach (no nitpicking, it's not polished yet) and tell me what you think about it? Concerns, optimizations, different ideas welcome. The only related failing test ist this one which is expected (see remarks in the code). Don't know how to fix this. I guess we need to skip the test?

Ocramius · 2017-02-09T15:55:54Z

lib/Doctrine/DBAL/Driver/LastInsertId.php

+ *
+ * @author Steve Müller <st.mueller@dzh-online.de>
+ */
+class LastInsertId


final? I really suggest that userland does never extend this...

Ocramius · 2017-02-09T15:57:13Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php

            $this->_bindedValues = array_fill(1, $paramCount, null);
        }
+
+        $this->lastInsertId = $lastInsertId;


Should probably create an instance, if none is given

That won't help as we need the reference in the connection. I just wanted to keep BC here, but not sure if that is necessary.

Yes, that's necessary here, sadly.

Ocramius · 2017-02-09T15:59:09Z

lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php

+            return '0';
        }

        $sql    = 'SELECT ' . $name . '.CURRVAL FROM DUAL';


$sql variable is redundant

Ocramius · 2017-02-09T16:03:25Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

 {
+    /**
+     * @var string
+     */


Why not reuse the LastInsertId object that we created?

Because I need to know how to retrieve the last insert ID in the statement object? Simply passing the LastInsertId container won't help in PDOStatement::execute()

Ah I see what you mean. Yeah true I can do that.

Ocramius · 2017-02-09T16:07:09Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

+     *
+     * @return bool
+     */
+    protected function supportsTrackingLastInsertId()


This can most probably be a constant or a protected property. Having it as a method seems excessive, and can lead to (although minor) overhead

Could also be a constant, btw.

Another (better) approach is to make trackLastInsertId empty in subclasses not supporting it. Thoughts?

YES! The last suggestion sounds perfect. I will see if that works.

Ocramius · 2017-02-09T16:17:42Z

lib/Doctrine/DBAL/Driver/PDOPgSql/Connection.php

+     */
+    private $lastInsertIdCallNested = false;
+
+    /**


Please don't keep commented code

I left it in here to demonstrate you what the problem with postgres is. I will of course remove it in the final version ;)

Ocramius · 2017-02-09T16:19:15Z

lib/Doctrine/DBAL/Driver/PDOStatement.php

        try {
-            return parent::execute($params);
+            $result = parent::execute($params);
+


if ($this->connection) {

👍 BTW I am still not using either style consistently. Do you have any reason why to prefer one over the other?

Ocramius · 2017-02-09T16:20:33Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php

-        return sqlsrv_rows_affected($stmt);
+        $affectedRowCount = sqlsrv_rows_affected($stmt);
+
+        $stmt = sqlsrv_query($this->conn, 'SELECT @@IDENTITY');


This seems excessive, as it duplicates queries to the DB. Since we can just SELECT @@IDENTITY anyway, do we need to update the last insert ID?

This is what I was talking about before showing off this approach ("performance impact"). If you have a look at how the different PDO drivers implement lastInsertId() you will see that each of them actually trigger a query. pdo_sqlsrv uses the exact same, pdo_pgsql uses SELECT LASTVAL(), pdo_mysql uses SELECT LAST_INSERT_ID(). However I am not sure but I believe those are session related information calls which do not go through the server at all. Would need to investigate this.

And yes we need to update the ID in exec(), query() and prepare()->execute() as those are the methods you can use to execute queries on each driver.

However I am not sure but I believe those are session related information calls which do not go through the server at all. Would need to investigate this.

This might cause an uproar of DBAs if they actually hit the backend. If only some exotic drivers (cough sqlsrv cough) actually hit the backend, then we should be quite OK.

Since this is an SQLSrv-only problem, this comment can be ignored.

IMO: their problem for relying on SQLSrv.

Ocramius · 2017-02-09T16:23:37Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php

+            return;
+        }
+
+        $statement = sqlsrv_query($this->conn, 'SELECT @@IDENTITY');


Same as above

As I said, this in fact is what would normally be in lastInsertId()

Looks OK then.

…cting

deeky666 · 2017-02-09T20:23:47Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

+        $lastInsertId = $this->fetchLastInsertId();
+
+        // Reactivate exception mode.
+        $this->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);


@Ocramius should we rather evaluate and set the previous error mode here? because in userland you could potentially change our default error mode.

Scratch that - using `PDO#getAttribute()` is simple/easy here - anything against using it?

Can fetchLastInsertId ever fail? If so, this block should be in a finally {}

Done both (restoring + finally)

deeky666 · 2017-02-09T20:26:29Z

lib/Doctrine/DBAL/Driver/PDOStatement.php

     * Protected constructor.
+     *
+     * @param PDOConnection|null $connection
+     *


@Ocramius no decision done here so far. What do you think?

We have to maintain BC in my opinion, otherwise we may break existing manual DIC calls.

deeky666 · 2017-02-09T20:27:52Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php

        $this->sql = $sql;
-
-        if (stripos($sql, 'INSERT INTO ') === 0) {
-            $this->sql .= self::LAST_INSERT_ID_SQL;


@Ocramius do we need to keep the constant for BC?

The constant has to stay, but should be deprecated if we don't use it. This is where private constants will be vital once we switch to PHP 7.1

Ocramius · 2017-02-13T13:04:06Z

lib/Doctrine/DBAL/Driver/LastInsertId.php

+        // The last insert ID is reset to "0" in certain situations by some implementations,
+        // therefore we keep the previously set insert ID locally.
+        if ('0' !== $value) {
+            $this->value = $value;


Should probably apply a string cast, since we are not using PHP7's type hints.

No longer needed, as we can rely on PHP 7.1 now.

No need: handled via addition of a type hint

Ocramius · 2017-02-13T13:05:08Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php

            $this->_bindedValues = array_fill(1, $paramCount, null);
        }
+
+        $this->lastInsertId = $lastInsertId;


Yes, that's necessary here, sadly.

Ocramius · 2017-02-13T13:06:17Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

+        $lastInsertId = $this->fetchLastInsertId();
+
+        // Reactivate exception mode.
+        $this->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);


Scratch that - using `PDO#getAttribute()` is simple/easy here - anything against using it?

Ocramius · 2017-02-13T13:06:51Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

+        $lastInsertId = $this->fetchLastInsertId();
+
+        // Reactivate exception mode.
+        $this->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);


Can fetchLastInsertId ever fail? If so, this block should be in a finally {}

Ocramius · 2017-02-13T13:09:05Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

    }
+
+    /**
+     * Tracks the last insert ID at the current state.


This method should be annotated with @internal

Ocramius · 2017-02-13T13:10:39Z

lib/Doctrine/DBAL/Driver/PDOStatement.php

     * Protected constructor.
+     *
+     * @param PDOConnection|null $connection
+     *


We have to maintain BC in my opinion, otherwise we may break existing manual DIC calls.

Ocramius · 2017-02-13T13:11:06Z

lib/Doctrine/DBAL/Driver/PDOStatement.php

        try {
-            return parent::execute($params);
+            $result = parent::execute($params);
+


This conditional seems wrong: you only track if there's no connection?

This comment is invalid - the connection passed in as constructor is a design decision, and nullability is just a side-effect of BC compliance. This code should never be skipped inside DBAL, but it could be skipped if the class is used directly in userland.

Ocramius · 2017-02-13T13:13:44Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvConnection.php

-        return sqlsrv_rows_affected($stmt);
+        $affectedRowCount = sqlsrv_rows_affected($stmt);
+
+        $stmt = sqlsrv_query($this->conn, 'SELECT @@IDENTITY');


However I am not sure but I believe those are session related information calls which do not go through the server at all. Would need to investigate this.

This might cause an uproar of DBAs if they actually hit the backend. If only some exotic drivers (cough sqlsrv cough) actually hit the backend, then we should be quite OK.

Ocramius · 2017-02-13T13:14:16Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php

        $this->sql = $sql;
-
-        if (stripos($sql, 'INSERT INTO ') === 0) {
-            $this->sql .= self::LAST_INSERT_ID_SQL;


The constant has to stay, but should be deprecated if we don't use it. This is where private constants will be vital once we switch to PHP 7.1

Ocramius · 2017-02-13T13:14:39Z

lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php

+            return;
+        }
+
+        $statement = sqlsrv_query($this->conn, 'SELECT @@IDENTITY');


Looks OK then.

…O drivers

…uced API

… with a hardcoded value

…O drivers

…leak

…2 handles per test)

…uced API

… with a hardcoded value

…O drivers

…2 handles per test)

deeky666 added Bug Drivers labels Feb 8, 2017

deeky666 requested a review from Ocramius February 8, 2017 09:49

deeky666 commented Feb 8, 2017

View reviewed changes

Ocramius approved these changes Feb 8, 2017

View reviewed changes

Ocramius added this to the 2.6 milestone Feb 8, 2017

deeky666 changed the title ~~Fix last insert ID consistency accross drivers~~ [WIP] Fix last insert ID consistency accross drivers Feb 8, 2017

deeky666 mentioned this pull request Feb 9, 2017

Testing of MSSQL on Windows with AppVeyor #2617

Merged

Ocramius suggested changes Feb 9, 2017

View reviewed changes

Ocramius assigned deeky666 Feb 9, 2017

deeky666 added 12 commits February 9, 2017 19:18

make last insert id behaviour consistent accross drivers

ad6a389

fix tearing down functional tests that have been skipped before conne…

96f5b8b

…cting

fix last insert ID tests

b47ab23

new approach (WIP)

f0c227c

let's see what's left to do

9df7e25

update PDOStatement todo

5b19ce5

fix oci8 connection test

3b0d990

make LastInsertId container class final

fadb400

simplify oci8 last insert retrieval

92eea90

use LastInsertId container object in PDO connection

24d89ab

refactor last insert ID extension points in PDO connection

d77e5e1

fix code style

761b218

deeky666 force-pushed the fix-last-insert-id-consistency branch from e2f2eb7 to 761b218 Compare February 9, 2017 20:18

deeky666 commented Feb 9, 2017

View reviewed changes

Ocramius suggested changes Feb 13, 2017

View reviewed changes

morozov added a commit to morozov/dbal that referenced this pull request Mar 31, 2018

doctrine#2648 Renamed set() to register()

e3ee044

morozov added a commit to morozov/dbal that referenced this pull request Mar 31, 2018

doctrine#2648 Replaced duplicated code with variadic argument

b2b20be

morozov added a commit to morozov/dbal that referenced this pull request Mar 31, 2018

doctrine#2648 Temporarily disabled the test causing a dead lock on PD…

09399be

…O drivers

morozov added a commit to morozov/dbal that referenced this pull request Mar 31, 2018

doctrine#2648 Fixed failures on PDO drivers

547f921

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 adding return hints and parameter hints to newly introd…

6682981

…uced API

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 adding type hints to the LastInsertId class

8a5c1a9

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 deprecated LAST_INSERT_ID_SQL class constant

ea7c25e

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 removed nullability of the lastInsertId private property

2c7f130

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 wrap ERRMODE_EXCEPTION restoring in a finally block

64fbf4f

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 restoring original error mode instead of overwriting it…

4ed7334

… with a hardcoded value

morozov added a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 Renamed set() to register()

0bbe3b4

morozov added a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 Replaced duplicated code with variadic argument

204f938

morozov added a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 Temporarily disabled the test causing a dead lock on PD…

122f2b2

…O drivers

morozov added a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 Fixed failures on PDO drivers

f9cf17a

morozov added a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 Close temporary connection to eliminate connection the …

d335808

…leak

morozov added a commit to morozov/dbal that referenced this pull request Apr 2, 2018

doctrine#2648 Close temporary connection to eliminate the leak (1 of …

6fb67c9

…2 handles per test)

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 adding return hints and parameter hints to newly introd…

2082149

…uced API

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 adding type hints to the LastInsertId class

dcce88b

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 deprecated LAST_INSERT_ID_SQL class constant

58c5f1e

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 removed nullability of the lastInsertId private property

b5051ea

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 wrap ERRMODE_EXCEPTION restoring in a finally block

1d34c2c

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 restoring original error mode instead of overwriting it…

07ecd64

… with a hardcoded value

morozov added a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 Renamed set() to register()

a2da1b8

morozov added a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 Replaced duplicated code with variadic argument

9a2cf83

morozov added a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 Temporarily disabled the test causing a dead lock on PD…

cba547e

…O drivers

morozov added a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 Fixed failures on PDO drivers

aee0eb3

morozov added a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 Close temporary connection to eliminate the leak (1 of …

076c1da

…2 handles per test)

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 adding type hints to the LastInsertId class

c88bb64

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#2648 deprecated LAST_INSERT_ID_SQL class constant

b0a2f81

morozov mentioned this pull request Apr 18, 2018

Fix last insert ID consistency across drivers #3074

Closed

5 tasks

Uh oh!

Conversation

deeky666 commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Feb 8, 2017

Uh oh!

Ocramius commented Feb 8, 2017

Uh oh!

deeky666 commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deeky666 Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deeky666 Feb 9, 2017 •

edited

Loading