OCI8 transactions are not isolated

[BenMorel]

Bug Report

Q	A
BC Break	no
Version	2.12.0

While creating a failing test case for LockMode::NONE on SQL Server & SQL Anywhere in #4415, I noticed that OCI8 fails as well for an unknown reason:

https://github.com/doctrine/dbal/pull/4415/checks?check_run_id=1371509666

The behaviour is the following, given two connections, C1 and C2:

C1: CREATE TABLE users (id NUMBER(10) NOT NULL)
C1: SET TRANSACTION ISOLATION LEVEL READ COMMITTED
C2: SET TRANSACTION ISOLATION LEVEL READ COMMITTED
C1: "START TRANSACTION"
C2: "START TRANSACTION"
C1: INSERT INTO users (id) VALUES (1)
C2: SELECT id FROM users ⚠️ returns the record inserted by C1! ⚠️
C1: "COMMIT"
C2: "COMMIT"
C1: DROP TABLE users

It looks the test works fine on PDO_OCI, so this may be a bug with OCI8.

Possible causes I can think of:

• Transactions are not actually started?
• Wrong isolation level set?
• TestUtil::getConnection() returns the same connection twice?

[morozov]

I think this is by design from the OCI standpoint but Doctrine doesn't support this use case. From the documentation:

If two handles need to be transactionally isolated from each other, use oci_new_connect() instead.

[morozov]

diff --git a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php
index 8e043ff54..d64b73dc2 100644
--- a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php
+++ b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php
@@ -62,7 +62,7 @@ class OCI8Connection implements ConnectionInterface, ServerInfoAwareConnection
     ) {
         $dbh = $persistent
             ? @oci_pconnect($username, $password, $db, $charset, $sessionMode)
-            : @oci_connect($username, $password, $db, $charset, $sessionMode);
+            : @oci_new_connect($username, $password, $db, $charset, $sessionMode);

         if ($dbh === false) {
             throw OCI8Exception::fromErrorInfo(oci_error());
diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php b/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php
index e91dd2a82..39e5cf385 100644
--- a/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php
+++ b/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php
@@ -24,11 +24,6 @@ class NoneTest extends DbalFunctionalTestCase
     {
         parent::setUp();

-        if ($this->connection->getDriver() instanceof OCI8\Driver) {
-            // https://github.com/doctrine/dbal/issues/4417
-            self::markTestSkipped('This test fails on OCI8 for a currently unknown reason');
-        }
-
         if ($this->connection->getDatabasePlatform() instanceof SQLServerPlatform) {
             // Use row versioning instead of locking on SQL Server (if we don't, the second connection will block when
             // attempting to read the row created by the first connection, instead of reading the previous version);

$ phpunit -c oci8.phpunit.xml tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php
PHPUnit 9.4.0 by Sebastian Bergmann and contributors.

.                                                                                   1 / 1 (100%)

Time: 00:00.854, Memory: 16.00 MB

OK (1 test, 1 assertion)

[BenMorel]

Thanks, @morozov. Should oci_new_connect() be used by default instead, then?

[morozov]

Should oci_new_connect() be used by default instead, then?

I would think twice. It may cause a performance impact in terms of establishing a DB connection and, more importantly, using prepared statements.

I'm open to adding another connection flag that would use oci_new_connect() internally. But in order to make it the default, we need to do some performance testing.

[BenMorel]

TBH, I'd rather make that the default and add an option to use oci_connect() instead. So that by default, all connections created by the DBAL are transactionally isolated, which is the case if I'm not mistaken, for all other drivers.

[morozov]

It can be done in the next major since it's a breaking change.

[beberlei]

It could be done with an opt-in flag passed as $params to switch from the old to the new.

But I don't think this causes performance problems as it would require someone to use two Doctrine Connection objects or connect with exactly the same params outside of Doctrine, where not having isolation from might even be considered dangerous from Doctrine POV.

[morozov]

It could be done with an opt-in flag passed as $params to switch from the old to the new.

Yes, my BC argument was about changing the default.

But I don't think this causes performance problems as it would require someone to use two Doctrine Connection objects or connect with exactly the same params outside of Doctrine

Not sure how this is relevant to Doctrine. AFAIK, Oracle Instant Client maintains the connections within itself. Hence, the same connection with the same credentials can be reused including all the prepared statements, etc. If they cannot be reused, it may have its side effects.

I believe introducing a flag now and changing the default later is fine.