Prepared statements become unusable after calling closeCursor() on IBM DB2, Oracle and MS SQL Server. #2546

morozov · 2016-10-30T03:16:07Z

The adapters above free their statement resources as part of closeCursor(), so the statement cannot be reused after that. The rest of adapters (mysqli, sasql and PDO) keep their statement resources in place during closing cursor and allow reuse after.

The fact that the statement is closed during closeCursor() contradicts the purpose of the method:

Closes the cursor, enabling the statement to be executed again.

deeky666 · 2017-01-14T18:31:52Z

@morozov the patch looks legit. What is the matter with mysqli? Why is it not working? Can we get it behaving correctly somehow?

morozov · 2017-01-14T19:49:00Z

@deeky666 the test will fail on mysqli until #2536 is resolved. Basically, the mysqli driver currently cannot reuse statements for a different reason. That pull request is also waiting for your input.

morozov · 2017-01-14T21:39:13Z

@deeky666 please see the update.

deeky666 · 2017-01-14T22:18:41Z

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

+            $value = $stmt->fetchColumn();
+        } catch (\Exception $e) {
+            // some adapters trigger PHP error or throw adapter-specific exception in case of fetching
+            // from a closed cursor, which still proves that it has been closed


This is an inconsistency then. Maybe we should fix that too. The interface states that false is returned in that case. Having the drivers behave consistently finally would be great. Can you investigate? Thanks!

The current state is:

pdo_sqlite returns false,

mysqli throws Doctrine\DBAL\Driver\Mysqli\MysqliException "Commands out of sync; you can't run this command now",

oci8 returns false,

sqlsrv returns false,

ibm_db2 triggers warning "db2_fetch_array(): Fetch Failure".

Looks like MySQL and DB2 consider this situation logically exceptional, as the cursor is not just empty but is explicitly closed. Maybe so should we in the rest of the drivers.

@morozov it is indeed an exceptional case but we should stick to our interfaces as those are the contracts we provide. In the 3.0 we will rework the interfaces to throw meaningful exceptions as returning false is not a good behaviour in exceptional cases. But for now we should stick to returning false I think. @Ocramius thoughts?

@deeky666 if I understand correctly, there's no clear expectation of how cursors should behave after being closed given the current implementation. The problem with returning false is that it's hard to identify why exactly fetch failed on the application end. "Fetch Failure" and "Commands out of sync;" are too generic and may mean some other exception which we don't want to silently convert to false.

Another question is, do we really have to fix consistency as part of this ticket. It's already big enough and judging by the number resolves ~5 different issues (the number of test cases).

@morozov you are completely right. I feel bad about returning false too but unfortunately we have made up that contract when designing the API.

I'm also aware that this PR is growing big already but in the end it is about fixing the close cursor behaviour across all the drivers. Unfortunately it's quite a rabbit hole but it feels wrong to leave some parts buggy when trying to fix a general problem (which this test case exposes). I am ok with a rather big change here if we get it fixed consistently.

Also relying on exceptions from some drivers but not from others in this scenario is not an issue for us because it is undefined behaviour (as not being defined in the interface).

@deeky666 I see your point. How should we implement handling these warnings/exceptions? Just do string/error code comparison? Or maybe manually track calls to closeCursor() in the drivers in question and return false without calling the actual fetch() to meet the contract.

@morozov We could use a flag $result for those problematic drivers, which is set in execute as soon as we have a result and have fetch() methods check for that flag and return false early. closeCursor will then reset the flag. Does that make sense or am I missing something here?

@deeky666 I was thinking about the opposite: having an $isCursorClosed flag which is set in closeCursor(), reset in execute() and is checked before calling the underlying fetch() implementation. Here, we only want to manually handle the case when the cursor is closed and then fetched. Not the cases when for example the statement is constructed but not yet executed (which would also be covered with your approach). In general, both are about the same. What would you suggest?

@morozov I don't have a strong opinion about either of both options. I am just concerned about what the actual behaviour should be when fetching without having executed the query before. Because IMO the whole API defined in Doctrine\DBAL\Driver\ResultStatement is in theory only available after executing the prepared statement. I think of it as if one would return a Result object (which is in fact a cursor) form the execute() method. So to say an encapsulation of the result cursor. I think APIs like JDBC have similar constucts. So in this case the $result flag would make more sense to me as it is explicitly set when having a result rather than having as $isCursorClosed which does not have a meaning as long as we the prepared statment is not executed. I think I saw something like the $result flag in some driver already...

deeky666 · 2017-01-14T22:22:41Z

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

+        $stmt->execute();
+
+        $id = $stmt->fetchColumn();
+        $this->assertNotEmpty($id);


I don't think we need to assert here again as it already is covered by the test before. So simply executing and immediately closing should be enough.

deeky666 · 2017-01-14T22:23:38Z

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

+        $table->addColumn('id', 'integer');
+        $sm->createTable($table);
+        $this->_conn->insert('stmt_test_no_results', array('id' => 1));
+        $this->_conn->insert('stmt_test_no_results', array('id' => 2));


We only need one row, so please remove this one.

morozov · 2017-01-17T06:24:41Z

@deeky666 please see the update.

morozov · 2017-01-23T22:10:17Z

@deeky666, ping

deeky666

We also need to change sasql_stmt_free_result to sasql_stmt_reset in SQLAnywhereStatement::closeCursor() as reusing the prepared statement won't work otherwise.

Furthermore I would suggest that we introduce more tests for edge case scenarios:

fetch*()
closeCursor()
closeCursor() -> fetch*()
execute() -> closeCursor() -> fetch*()

What do you think? Anything else that comes to mind?

deeky666 · 2017-01-24T15:49:24Z

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

+        $sm = $this->_conn->getSchemaManager();
+        $table = new Table('stmt_test_longer_results');
+        $table->addColumn('param', 'string');
+        $table->addColumn('value', 'text');


Using value errors on DB2 as it is a reserved word. Using val for example works though.

deeky666 · 2017-01-24T15:50:04Z

tests/Doctrine/Tests/DBAL/Functional/StatementTest.php

+
+        $sm = $this->_conn->getSchemaManager();
+        $table = new Table('stmt_test_long_blob');
+        $table->addColumn('data', 'blob', array(


Using data errors on DB2 as it is a reserved word, we need to use something else here.

deeky666 · 2017-01-24T15:52:41Z

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

+        // @link http://php.net/manual/en/pdostatement.closecursor.php
+        // @link https://github.com/php/php-src/blob/php-7.0.11/ext/pdo/pdo_stmt.c#L2075
+        // deliberately do not consider multiple result sets, since doctrine/dbal doesn't support them
+        while (oci_fetch($this->_sth));


We need to work with $result here too, otherwise calling closeCursor() or one of the fetch*() methods before execute() errors:

ORA-24374: define not done before fetch or execute and fetch

PDO returns true for closeCursor() and false for fetch*() methods. We should try to be consistent here.

deeky666 · 2017-01-24T15:55:33Z

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

@@ -150,9 +150,13 @@ public function bindParam($column, &$variable, $type = null, $length = null)
     */
    public function closeCursor()
    {
-        if ($this->stmt) {


If $stmt is not initialized, we need to return true here. This happens if execute() has not been called before. Otherwise sqlsrv_fetch() will error.

deeky666 · 2017-01-24T16:01:25Z

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

@@ -150,9 +150,13 @@ public function bindParam($column, &$variable, $type = null, $length = null)
     */
    public function closeCursor()
    {
-        if ($this->stmt) {


Also reusing prepared statements does currently not work as the binding by reference does not work. We need to modify bindParam() to this:

public function bindParam($column, &$variable, $type = null, $length = null) { if (!is_numeric($column)) { throw new SQLSrvException("sqlsrv does not support named parameters to queries, use question mark (?) placeholders instead."); } $this->values[$column] = $variable; if ($type === \PDO::PARAM_LOB) { $this->params[$column-1] = array(&$this->values[$column], SQLSRV_PARAM_IN, SQLSRV_PHPTYPE_STREAM(SQLSRV_ENC_BINARY), SQLSRV_SQLTYPE_VARBINARY('max')); } else { $this->params[$column-1] =& $this->values[$column]; } }

Also introduces a new private property $values that keeps track of parameter references. Otherwise using different parameters won't work and the driver will always use those from the first binding.

@deeky666 is it about the case like:

public function testReuseStatementWithParameterBoundByReference() { $this->_conn->insert('stmt_test', array('id' => 1)); $this->_conn->insert('stmt_test', array('id' => 2)); $stmt = $this->_conn->prepare('SELECT id FROM stmt_test WHERE id = ?'); $stmt->bindParam(1, $id); $id = 1; $stmt->execute(); $this->assertEquals(1, $stmt->fetchColumn()); $id = 2; $stmt->execute(); $this->assertEquals(2, $stmt->fetchColumn()); }

Then it should be $this->values[$column] =& $variable, same as in MysqliStatement::bindParam(). In any case, DB2 is unhappy about this case because the DBAL does both db2_bind_param() and passes the values of $this->_bindParam to db2_execute():

db2_execute(): Binding Error 2

@morozov basically it is about that yes. But the test case I used was this:

1) Doctrine\Tests\DBAL\Functional\StatementTest::testReuseStatementAfterClosingCursor Failed asserting that 1 matches expected 2. /php/srv/tests/Doctrine/Tests/DBAL/Functional/StatementTest.php:151

But also we are talking about SQL Server here, not DB2.

@deeky666 with your fix testReuseStatementAfterClosingCursor() passes, thank you. Not sure if this is relevant to this issue (I wish it wasn't), but testReuseStatementWithParameterBoundByReference() above still doesn't pass on SQL Server and IBM DB2. It's made up and I don't know if it should pass in the first place (however it does on other DBs).

The difference between the two is that the first binds param by value, and the second binds them by reference.

UPD: I've modified your solution to make testReuseStatementWithParameterBoundByReference() also pass.

@morozov I now figured out the issue with DB2 binding and it is quite nasty -.-. The binding error seems to be related due to $this->_bindParam having referenced variable values so the fix that worked for me is to change the portion in execute() to:

if ($params === null) { ksort($this->_bindParam); $params = array(); foreach ($this->_bindParam as $column => $value) { $params[] = $value; } }

This fixes the binding error but in fact we cannot get db_bind_param() to work at all because it is not actually binding immediately at call time. Instead when calling db2_execute(), the function looks up variables by the names passed to db2_bind_param lazily. And even worse, it expects thos variables to be declared in global scope (WTF). In theory we could create global variables on binding with global keyword but that does not seem to work properly either as I am getting db2_execute(): Binding Error 1 then. So I would suggest changing the code as shown above, because real parameter binding does not work atm anyways.

…db2 and oci drivers

…event string truncation

morozov · 2017-01-25T02:10:52Z

@deeky666 thank you for the detailed review.

deeky666 · 2017-01-27T11:54:02Z

@morozov we are getting closer :) Awesome test cases btw!

…LongBlob()` on pdo_sqlsrv The test is partially skipped due to: 1. Fetching blob as binary string from pdo_sqlsrv requires using `PDOStatement::bindColumn()` and `PDOStatement::fetch(PDO::FETCH_BIND)` which are not supported by the DBAL. 2. Setting encoding on the connection level is not supported: https://msdn.microsoft.com/en-us/library/ff628164(v=sql.105).aspx

…StatementWithParameterBoundByReference()` on ibm_db2

Ocramius · 2017-02-02T20:04:34Z

Random comment, but it has to be said: @morozov thanks for the patience, impressive work!

morozov · 2017-02-02T20:11:11Z

@Ocramius my pleasure. I want to say the same to you guys but I'll wait until this one gets merged :-)

deeky666

simply: AWESOME!

deeky666 · 2017-02-02T20:32:18Z

LGTM! I'll let have @Ocramius have a second look on that and then we can merge. 🎉

morozov · 2017-02-02T22:22:01Z

@deeky666 does it make sense to introduce a libmysql label as it's also an environment attribute (often not taken into account) which the DBAL behavior may depend on?

deeky666 · 2017-02-03T00:42:46Z

@morozov good point. Done. Added libmysql and mysqlnd labels.

Ocramius · 2017-02-04T21:05:13Z

Awesome work! Some minor CS problems, but I'm going to close an eye and fix them post-merge.

Thanks @morozov and @deeky666!

…2.5' into 2.5 Backport #2546 to 2.5.x

Ocramius · 2017-02-04T21:12:36Z

Backported to 2.5 in b713ba7

morozov mentioned this pull request Oct 30, 2016

Rebind row values to the statement after freeing result #2489

Closed

morozov mentioned this pull request Nov 23, 2016

Inconsistent semantics of close cursor #2557

Closed

deeky666 added Bug Drivers ibm_db2 oci8 Prepared Statements sqlsrv labels Jan 14, 2017

deeky666 mentioned this pull request Jan 14, 2017

Bind result variables after each execution in order to prevent string truncation #2536

Closed

morozov force-pushed the close-cursor-frees-statement branch from 5505093 to 923c29d Compare January 14, 2017 21:06

deeky666 reviewed Jan 14, 2017

View reviewed changes

deeky666 suggested changes Jan 14, 2017

View reviewed changes

morozov force-pushed the close-cursor-frees-statement branch from 923c29d to c9b86c2 Compare January 14, 2017 23:52

deeky666 suggested changes Jan 24, 2017

View reviewed changes

morozov added 5 commits January 24, 2017 16:50

[DBAL-2546] Reworked implementation of closeCursor() for mysqli, ibm_…

074a12a

…db2 and oci drivers

[DBAL-2546] Fixed SQLAnywhereStatement::closeCursor()

01d7097

[DBAL-2546] Bind result variables after each execution in order to pr…

d44cc19

…event string truncation

[DBAL-2546] Added regression test for doctrine#433

2772862

[DBAL-2546] Ported tests from doctrine#2487 and doctrine#2489

c19af34

morozov force-pushed the close-cursor-frees-statement branch from 89bc749 to ec8c742 Compare January 25, 2017 01:55

morozov added 2 commits January 24, 2017 20:45

[DBAL-2546] Prevent exception when fetching from closed cursor

d34ef56

[DBAL-2546] Refactored unit tests and added some more for edge cases

bc62b08

morozov force-pushed the close-cursor-frees-statement branch from ec8c742 to bc62b08 Compare January 25, 2017 04:46

morozov added 2 commits February 2, 2017 11:53

[DBAL-2546] Fixed the failure of `Functional\StatementTest::testReuse…

dab3ae1

…StatementWithParameterBoundByReference()` on ibm_db2

morozov force-pushed the close-cursor-frees-statement branch from 2050ed7 to dab3ae1 Compare February 2, 2017 19:56

deeky666 approved these changes Feb 2, 2017

View reviewed changes

deeky666 self-assigned this Feb 2, 2017

deeky666 added mysqli pdo_sqlsrv sqlanywhere labels Feb 2, 2017

deeky666 assigned Ocramius Feb 2, 2017

deeky666 added this to the 2.5.11 milestone Feb 2, 2017

deeky666 added the libmysql label Feb 3, 2017

Ocramius approved these changes Feb 4, 2017

View reviewed changes

Ocramius merged commit 3f8c746 into doctrine:master Feb 4, 2017

Ocramius added a commit that referenced this pull request Feb 4, 2017

Merge branch 'fix/#2546-prepared-statement-close-cursor-invalidation-…

b713ba7

…2.5' into 2.5 Backport #2546 to 2.5.x

Ocramius added a commit that referenced this pull request Feb 4, 2017

#2546 s/callable/Closure to please the ancient PHP version gods

1b1effb

morozov deleted the close-cursor-frees-statement branch February 4, 2017 21:22

deeky666 mentioned this pull request Feb 7, 2017

[DBAL-2637] Fix closing prepared statement cursor with LOB column on Oracle #2645

Merged

morozov mentioned this pull request Jul 12, 2017

MSSQL IDENTITY_INSERT Issues #2752

Open

morozov mentioned this pull request Dec 26, 2017

Enabled testFetchLongBlob() for PDO SQL Server driver #2952

Merged

morozov mentioned this pull request Jan 30, 2018

Statement objects should throw exception in exceptional cases #3005

Closed

sypets mentioned this pull request Jul 26, 2020

Do not call closeCursor() in prepared statements on TYPO3 8.7 TYPO3-Documentation/TYPO3CMS-Reference-CoreApi#955

Merged

morozov mentioned this pull request Oct 30, 2021

Commands out of sync #2430

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2022

Prepared statements become unusable after calling closeCursor() on IBM DB2, Oracle and MS SQL Server. #2546

Prepared statements become unusable after calling closeCursor() on IBM DB2, Oracle and MS SQL Server. #2546

Conversation

morozov commented Oct 30, 2016

deeky666 commented Jan 14, 2017

morozov commented Jan 14, 2017 • edited Loading

morozov commented Jan 14, 2017

Choose a reason for hiding this comment

morozov Jan 14, 2017 • edited Loading

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

deeky666 Jan 16, 2017 • edited Loading

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

morozov commented Jan 17, 2017

morozov commented Jan 23, 2017

deeky666 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Jan 25, 2017

deeky666 commented Jan 27, 2017

Ocramius commented Feb 2, 2017

morozov commented Feb 2, 2017 • edited Loading

deeky666 left a comment

Choose a reason for hiding this comment

deeky666 commented Feb 2, 2017

morozov commented Feb 2, 2017

deeky666 commented Feb 3, 2017

Ocramius commented Feb 4, 2017

Ocramius commented Feb 4, 2017

morozov commented Jan 14, 2017 •

edited

Loading

morozov Jan 14, 2017 •

edited

Loading

deeky666 Jan 16, 2017 •

edited

Loading

morozov commented Feb 2, 2017 •

edited

Loading