Firebird PDO begintransaction fails after query

[digitaldutch]

Description

The following code:

<?php
// Step 1: Perform a query
$sql = "SELECT 1 FROM anytable ROWS 1";
$statement = $pdo->prepare($sql);
$statement->execute();
$result = $statement->fetch(PDO::FETCH_ASSOC);

// Step 2: Perform a query in transaction
$pdo->beginTransaction();   // Fatal error on this line: Uncaught PDOException: There is already an active transaction
$statement = $pdo->prepare($sql);
$statement->execute();
$result = $statement->fetch(PDO::FETCH_ASSOC);
$pdo->commit();

Resulted in this output:
Fatal error: Uncaught PDOException: There is already an active transaction in ...

But I expected this output instead:
No exception. This works fine with mysql PDO. The manual says:

Turns off autocommit mode. While autocommit mode is turned off, changes made to the database via the PDO object instance are not committed until you end the transaction by calling PDO::commit(). Calling PDO::rollBack() will roll back all changes to the database and return the connection to autocommit.

It does not turn off autocommit. The workaround is to turn off autocommit manually before calling begintransaction and switching it on again after commit or rollback:

$pdo->setAttribute(PDO::ATTR_AUTOCOMMIT, false);
$pdo->beginTransaction();

// Do database stuff

$pdo->commit();
$pdo->setAttribute(PDO::ATTR_AUTOCOMMIT, true);

PHP Version

PHP 7.4 & 8.1.6

Operating System

No response

[cmb69]

The Firebird driver implicitly starts a transaction if auto_commit is enabled:

php-src/ext/pdo_firebird/firebird_driver.c

Lines 795 to 803 in c019421

	/* start a new transaction implicitly if auto_commit is enabled and no transaction is open */
	if (dbh->auto_commit && !dbh->in_txn) {
	/* dbh->transaction_flags = PDO_TRANS_READ_UNCOMMITTED; */
	if (!firebird_handle_begin(dbh)) {
	return 0;
	}
	dbh->in_txn = true;
	}

That is not the case for the MySQL driver (and most/all other drivers). Since Firebird apparently needs an explicit transaction (otherwise all queries will fail), that behavior makes some sense, and at least we cannot easily change it for BC reasons.

It might make sense to introduce a in_transaction handler for Firebird, which would then auto-commit any implicitly started transaction; a quick draft:

 ext/pdo_firebird/firebird_driver.c      | 18 +++++++++++++++++-
 ext/pdo_firebird/php_pdo_firebird_int.h |  4 +++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/ext/pdo_firebird/firebird_driver.c b/ext/pdo_firebird/firebird_driver.c
index 7dc40f510d..bd26032caf 100644
--- a/ext/pdo_firebird/firebird_driver.c
+++ b/ext/pdo_firebird/firebird_driver.c
@@ -800,6 +800,7 @@ static int firebird_alloc_prepare_stmt(pdo_dbh_t *dbh, const zend_string *sql,
 			return 0;
 		}
 		dbh->in_txn = true;
+		H->auto_txn = true;
 	}
 
 	/* allocate the statement */
@@ -856,6 +857,7 @@ static bool firebird_handle_set_attribute(pdo_dbh_t *dbh, zend_long attr, zval *
 								break;
 							}
 							dbh->in_txn = false;
+							H->auto_txn = false;
 						}
 					}
 					dbh->auto_commit = bval;
@@ -1011,6 +1013,20 @@ static void pdo_firebird_fetch_error_func(pdo_dbh_t *dbh, pdo_stmt_t *stmt, zval
 }
 /* }}} */
 
+static bool firebird_in_transaction(pdo_dbh_t *dbh)
+{
+	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
+	if (dbh->in_txn && dbh->auto_commit && H->auto_txn) {
+		if (isc_commit_transaction(H->isc_status, &H->tr)) {
+			RECORD_ERROR(dbh);
+		} else {
+			dbh->in_txn = false;
+			H->auto_txn = false;
+		}
+	}
+	return dbh->in_txn;
+}
+
 static const struct pdo_dbh_methods firebird_methods = { /* {{{ */
 	firebird_handle_closer,
 	firebird_handle_preparer,
@@ -1026,7 +1042,7 @@ static const struct pdo_dbh_methods firebird_methods = { /* {{{ */
 	NULL, /* check_liveness */
 	NULL, /* get driver methods */
 	NULL, /* request shutdown */
-	NULL, /* in transaction, use PDO's internal tracking mechanism */
+	firebird_in_transaction,
 	NULL /* get gc */
 };
 /* }}} */
diff --git a/ext/pdo_firebird/php_pdo_firebird_int.h b/ext/pdo_firebird/php_pdo_firebird_int.h
index 243163b8a0..6f948cf4bb 100644
--- a/ext/pdo_firebird/php_pdo_firebird_int.h
+++ b/ext/pdo_firebird/php_pdo_firebird_int.h
@@ -83,7 +83,9 @@ typedef struct {
 	/* prepend table names on column names in fetch */
 	unsigned fetch_table_names:1;
 
-	unsigned _reserved:29;
+	unsigned auto_txn:1;
+
+	unsigned _reserved:28;
 
 } pdo_firebird_db_handle;

That feels a bit hackish, though.

Anyhow, I think we need to document the existing behavior, and maybe leave it as is.

The workaround is to turn autocommit before calling the transaction:

That is not even necessary. It is totally sufficient to just start the transaction before the first ::prepare(). Or you could even ::commit() the implicit transaction just before you start the explicit transaction. If you want to be extra careful, you could do:

if ($pdo->inTransaction()) {
    $pdo->commit();
}
$pdo->beginTransaction();

[digitaldutch]

The reason I, and I assume others, start a transaction after other prepares/statements is to be able to rollback that part.

I have asked the Firebird news group about this issue and Mark Rotteveel, the maintainer of the Firebird JDBC driver says:

However, yes it sounds like a bug, something they confirmed in your bug
report. The "Firebird driver" mentioned by cmb69 is pdo-firebird
(https://github.com/php/php-src/tree/master/ext/pdo_firebird).

The problem is that Firebird itself doesn't have a suitable auto-commit
mode that automatically starts and completes transactions when
necessary, so a driver must handle starting and completingtransactions
for auto-commit itself, and the way pdo-firebird does this, doesn't
handle the requirements documented for PDO's beginTransaction.

If I read it correctly: missing suitable auto-commit support in the Firebird engine forces the driver to handle transactions properly.

Looks like it is probably wise to handle the transactions in the PDO driver to make begintransaction do what the documentation is saying "Turns off autocommit mode". And if that is not possible to update the documentation.

[cmb69]

Okay, I applied the patch above to "master", ran the test suite, and got seven fails. Six of these are caused because implicit transactions are explicitly committed, e.g.

php-src/ext/pdo_firebird/tests/bug_47415.phpt

Lines 10 to 20 in e9e3a14

	<?php
	require 'testdb.inc';
	$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_SILENT);
	@$dbh->exec('DROP TABLE testz');
	$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
	$dbh->exec('CREATE TABLE testz (idx int NOT NULL PRIMARY KEY, txt VARCHAR(20))');
	$dbh->exec('INSERT INTO testz VALUES(0, \'String0\')');
	$dbh->commit();

Apparently, the author of these tests was well aware of PDO_Firebird's implicit transactions.
And worse, automatically closing such implicit transactions will obviously break code which deliberately makes use of that. One may argue that one shouldn't commit transactions which have not been explicitly started, but given the long standing behavior, the change would still be a BC break.

Particularly interesting was that pdo_017.phpt fails with the patch. Having a closer look at the test, I was surprised that it didn't fail without the patch, and found that it is just skipped because "no working transactions". That is obviously nonsense, but again caused by the quirky implicit transactions of PDO_Firebird. So maybe despite the BC break we should do something about that.

However, having a closer look at that test showed a serious flaw in PDO_Firebird, namely that transactions are not (necessarily?) rolled back. Consider:

<?php

$pdo = new PDO("firebird:dbname=C:\\test.fdb", "sysdba", "masterkey");

// $pdo->exec("DROP TABLE gh8735"); // uncomment if table already exists
$pdo->exec("CREATE TABLE gh8735 (name CHAR(10))");
$pdo->exec("INSERT INTO gh8735 VALUES ('one')");
$pdo->commit(); // firebird quirk

$pdo->beginTransaction();
$pdo->exec("INSERT INTO gh8735 VALUES ('two')");
$pdo->rollback();

$stmt = $pdo->query("SELECT * from gh8735");
var_dump($stmt->fetchAll(PDO::FETCH_COLUMN));

That outputs:

array(2) {
  [0]=>
  string(10) "one       "
  [1]=>
  string(10) "two       "
}

Am I missing something, or is ::rollback() so broken with PDO_Firebird?

[digitaldutch]

Disclaimer: My c and driver knowledge are non existent. The following is from some testing:

Committing the implicit transactions seems superfluous. We can just leave them out in autocommit mode. As a side note, the JDBC driver does require it. Maybe the original author took that as an example.

I also noticed that implicit transactions do not rollback. It just does nothing. Like commit.

Rollback does work when starting a manual transaction:

$pdo->setAttribute(PDO::ATTR_AUTOCOMMIT, false);
$pdo->beginTransaction();
$pdo->exec('INSERT INTO testz VALUES(10, \'sometext\')');
$pdo->rollBack();
$pdo->setAttribute(PDO::ATTR_AUTOCOMMIT, true);

Probably a dumn suggestion:
If the driver just does what the work-around does above: Switch off PDO::ATTR_AUTOCOMMIT in beginTransaction() if it is on. And commit() and rollback() switch it on again if it is off.

Would that not fix the problem? beginTransaction(), commit() and rollback() do what the manual says and no old code is broken.

[digitaldutch]

I have inspected the pdo_firebird driver a bit (never too old to learn) and came to a few conclusions and a proposal.

When starting a manual transaction by calling beginTransaction(), auto_commit must be switched off otherwise every time a statement is executed it gets committed automatically. Rollback won't work..

The work-around by switching it off and on myself works fine. This should ideally happen in the driver.

Rollback is not broken. Only when in auto_commit mode every statement gets committed right away. So a rollback of an implicitly started transaction does not do anything because there is nothing left to rollback.

Proposed changes:

1. In the firebird_handle_doer() function, which begins a transaction, we should check if it is called manually using PDO::beginTransaction or by auto_commit from other code in the driver. Not sure how to do that in C. If called manually auto_commit must be disabled. We should probably set a auto_commit_manually_disabled parameter to true. This parameter should be false at driver initialization.

2. In firebird_handle_rollback() auto_commit must be enabled at the end if auto_commit_manually_disabled is true.

3. In firebird_handle_commit() auto_commit must be enabled at the end if auto_commit_manually_disabled is true.

Writing in pseudo code, because the last time I programmed in C++ (not even C) was 28 years ago.

[cmb69]

I'm afraid that regardless how we fix this issue, we would potentially break code which manually commits a automatically started transaction. So we likely can't do this for any of the stable branches (a minor BC break due to a bug fix might be acceptable for a new minor version, though).

But anyway, since I see some unexplainable strange behavior (last part of #8735 (comment)), I'm not going to pursue this (at least for now). Anybody is welcome to provide a PR, though!