[WIP] Fix: Corrected and optimized pdo_firebird transaction handling.

ext/pdo/tests/pdo_017.phpt

@@ -8,7 +8,6 @@ $dir = getenv('REDIR_TEST_DIR');
 if (false == $dir) die('skip no driver');
 require_once $dir . 'pdo_test.inc';
 PDOTest::skip();
-if (str_starts_with(getenv('PDOTEST_DSN'), "firebird")) die('xfail firebird driver does not behave as expected');
 
 $db = PDOTest::factory();
 try {

ext/pdo_firebird/firebird_driver.c

@@ -31,8 +31,7 @@
 #include "php_pdo_firebird.h"
 #include "php_pdo_firebird_int.h"
 
-static int firebird_alloc_prepare_stmt(pdo_dbh_t*, const zend_string*, XSQLDA*, isc_stmt_handle*,
-	HashTable*);
+static int firebird_alloc_prepare_stmt(pdo_dbh_t*, const zend_string*, XSQLDA*, isc_stmt_handle*, HashTable*);
 
 const char CHR_LETTER = 1;
 const char CHR_DIGIT = 2;
@@ -475,17 +474,14 @@ static void firebird_handle_closer(pdo_dbh_t *dbh) /* {{{ */
 {
 	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
 
-	if (dbh->in_txn) {
+	if (H->tr) {
 		if (dbh->auto_commit) {
-			if (isc_commit_transaction(H->isc_status, &H->tr)) {
-				RECORD_ERROR(dbh);
-			}
+			firebird_commit_transaction(dbh, false);
 		} else {
-			if (isc_rollback_transaction(H->isc_status, &H->tr)) {
-				RECORD_ERROR(dbh);
-			}
+			firebird_rollback_transaction(dbh, false);
 		}
 	}
+	H->in_manually_txn = 0;
 
 	if (isc_detach_database(H->isc_status, &H->db)) {
 		RECORD_ERROR(dbh);
@@ -647,8 +643,8 @@ static zend_long firebird_handle_doer(pdo_dbh_t *dbh, const zend_string *sql) /*
 	}
 
 	/* commit if we're in auto_commit mode */
-	if (dbh->auto_commit && isc_commit_retaining(H->isc_status, &H->tr)) {
-		RECORD_ERROR(dbh);
+	if (dbh->auto_commit && !H->in_manually_txn) {
+		firebird_commit_transaction(dbh, true);
 	}
 
 free_statement:
@@ -700,8 +696,8 @@ static zend_string* firebird_handle_quoter(pdo_dbh_t *dbh, const zend_string *un
 }
 /* }}} */
 
-/* called by PDO to start a transaction */
-static bool firebird_handle_begin(pdo_dbh_t *dbh) /* {{{ */
+/* _firebird_begin_transaction */
+static bool _firebird_begin_transaction(pdo_dbh_t *dbh) /* {{{ */
 {
 	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
 	char tpb[8] = { isc_tpb_version3 }, *ptpb = tpb+1;
@@ -753,28 +749,90 @@ static bool firebird_handle_begin(pdo_dbh_t *dbh) /* {{{ */
 }
 /* }}} */
 
+/* called by PDO to start a transaction */
+static bool firebird_handle_manually_begin(pdo_dbh_t *dbh) /* {{{ */
+{
+	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
+
+	if (dbh->auto_commit && H->tr && !firebird_commit_transaction(dbh, false)) {
+		return false;
+	}
+
+	if (!_firebird_begin_transaction(dbh)) {
+		return false;
+	}
+
+	H->in_manually_txn = 1;
+	return true;
+}
+/* }}} */
+
+/* firebird_commit_transaction */
+bool _firebird_commit_transaction(pdo_dbh_t *dbh, bool retain) /* {{{ */
+{
+	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
+
+	if (retain) {
+		if (isc_commit_retaining(H->isc_status, &H->tr)) {
+			RECORD_ERROR(dbh);
+			return false;
+		}
+	} else {
+		if (isc_commit_transaction(H->isc_status, &H->tr)) {
+			RECORD_ERROR(dbh);
+			return false;
+		}
+	}
+
+	return true;
+}
+/* }}} */
+
 /* called by PDO to commit a transaction */
-static bool firebird_handle_commit(pdo_dbh_t *dbh) /* {{{ */
+static bool firebird_handle_manually_commit(pdo_dbh_t *dbh) /* {{{ */
 {
 	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
 
-	if (isc_commit_transaction(H->isc_status, &H->tr)) {
-		RECORD_ERROR(dbh);
+	if (!firebird_commit_transaction(dbh, dbh->auto_commit)) {
 		return false;
 	}
+
+	H->in_manually_txn = 0;
+	return true;
+}
+/* }}} */
+
+/* firebird_rollback_transaction */
+bool _firebird_rollback_transaction(pdo_dbh_t *dbh, bool retain) /* {{{ */
+{
+	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
+
+	if (retain) {
+		if (isc_rollback_retaining(H->isc_status, &H->tr)) {
+			RECORD_ERROR(dbh);
+			return false;
+		}
+	} else {
+		if (isc_rollback_transaction(H->isc_status, &H->tr)) {
+			RECORD_ERROR(dbh);
+			return false;
+		}
+	}
+
 	return true;
 }
 /* }}} */
 
 /* called by PDO to rollback a transaction */
-static bool firebird_handle_rollback(pdo_dbh_t *dbh) /* {{{ */
+static bool firebird_handle_manually_rollback(pdo_dbh_t *dbh) /* {{{ */
 {
 	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
 
-	if (isc_rollback_transaction(H->isc_status, &H->tr)) {
-		RECORD_ERROR(dbh);
+	if (!firebird_rollback_transaction(dbh, dbh->auto_commit)) {
 		return false;
 	}
+
+	H->in_manually_txn = 0;
 	return true;
 }
 /* }}} */
@@ -792,16 +850,6 @@ static int firebird_alloc_prepare_stmt(pdo_dbh_t *dbh, const zend_string *sql,
 		return 0;
 	}
 
-	/* 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;
-	}
-
 	/* allocate the statement */
 	if (isc_dsql_allocate_statement(H->isc_status, &H->db, s)) {
 		RECORD_ERROR(dbh);
@@ -844,19 +892,22 @@ static bool firebird_handle_set_attribute(pdo_dbh_t *dbh, zend_long attr, zval *
 
 				/* ignore if the new value equals the old one */
 				if (dbh->auto_commit ^ bval) {
-					if (dbh->in_txn) {
-						if (bval) {
+					if (bval) {
+						if (H->tr && H->in_manually_txn) {
 							/* turning on auto_commit with an open transaction is illegal, because
-							   we won't know what to do with it */
+							we won't know what to do with it */
 							H->last_app_error = "Cannot enable auto-commit while a transaction is already open";
 							return false;
-						} else {
-							/* close the transaction */
-							if (!firebird_handle_commit(dbh)) {
-								break;
-							}
-							dbh->in_txn = false;
 						}
+						if (!H->tr && !_firebird_begin_transaction(dbh)) {
+							return false;
+						}
+					} else {
+						/* close the transaction */
+						if (H->tr && !firebird_commit_transaction(dbh, false)) {
+							return false;
+						}
+						H->in_manually_txn = 0;
 					}
 					dbh->auto_commit = bval;
 				}
@@ -1011,22 +1062,30 @@ static void pdo_firebird_fetch_error_func(pdo_dbh_t *dbh, pdo_stmt_t *stmt, zval
 }
 /* }}} */
 
+/* {{{ firebird_in_manually_transaction */
+static bool firebird_in_manually_transaction(pdo_dbh_t *dbh)
+{
+	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
+	return H->tr && H->in_manually_txn;
+}
+/* }}} */
+
 static const struct pdo_dbh_methods firebird_methods = { /* {{{ */
 	firebird_handle_closer,
 	firebird_handle_preparer,
 	firebird_handle_doer,
 	firebird_handle_quoter,
-	firebird_handle_begin,
-	firebird_handle_commit,
-	firebird_handle_rollback,
+	firebird_handle_manually_begin,
+	firebird_handle_manually_commit,
+	firebird_handle_manually_rollback,
 	firebird_handle_set_attribute,
 	NULL, /* last_id not supported */
 	pdo_firebird_fetch_error_func,
 	firebird_handle_get_attribute,
 	NULL, /* check_liveness */
 	NULL, /* get driver methods */
 	NULL, /* request shutdown */
-	NULL, /* in transaction, use PDO's internal tracking mechanism */
+	firebird_in_manually_transaction,
 	NULL /* get gc */
 };
 /* }}} */
@@ -1107,6 +1166,11 @@ static int pdo_firebird_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /*
 				"HY000", H->isc_status[1], errmsg);
 	}
 
+	H->in_manually_txn = 0;
+	if (dbh->auto_commit && !H->tr) {
+		ret = _firebird_begin_transaction(dbh);
+	}
+
 	if (!ret) {
 		firebird_handle_closer(dbh);
 	}

ext/pdo_firebird/firebird_statement.c

@@ -177,8 +177,7 @@ static int firebird_stmt_execute(pdo_stmt_t *stmt) /* {{{ */
 				;
 		}
 
-		/* commit? */
-		if (stmt->dbh->auto_commit && isc_commit_retaining(H->isc_status, &H->tr)) {
+		if (stmt->dbh->auto_commit && !S->H->in_manually_txn && !firebird_commit_transaction(stmt->dbh, true)) {
 			break;
 		}
 

ext/pdo_firebird/php_pdo_firebird_int.h

@@ -59,6 +59,12 @@ typedef void (*info_func_t)(char*);
 # define PDO_FIREBIRD_HANDLE_INITIALIZER NULL
 #endif
 
+extern bool _firebird_commit_transaction(pdo_dbh_t *dbh, bool retain);
+#define firebird_commit_transaction(d, r)	_firebird_commit_transaction(d, r)
+
+extern bool _firebird_rollback_transaction(pdo_dbh_t *dbh, bool retain);
+#define firebird_rollback_transaction(d, r)	_firebird_rollback_transaction(d, r)
+
 typedef struct {
 
 	/* the result of the last API call */
@@ -69,6 +75,7 @@ typedef struct {
 
 	/* the transaction handle */
 	isc_tr_handle tr;
+	bool in_manually_txn:1;
 
 	/* the last error that didn't come from the API */
 	char const *last_app_error;

ext/pdo_firebird/tests/bug_47415.phpt

@@ -15,8 +15,6 @@ $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
 $dbh->exec('CREATE TABLE test47415 (idx int NOT NULL PRIMARY KEY, txt VARCHAR(20))');
 $dbh->exec('INSERT INTO test47415 VALUES(0, \'String0\')');
 
-$dbh->commit();
-
 $query = "SELECT idx, txt FROM test47415 ORDER by idx";
 $idx = $txt = 0;
 $stmt = $dbh->prepare($query);
@@ -31,8 +29,6 @@ var_dump($stmt->rowCount());
 $stmt = $dbh->prepare('DELETE FROM test47415');
 $stmt->execute();
 
-$dbh->commit();
-
 unset($stmt);
 unset($dbh);
 ?>

ext/pdo_firebird/tests/bug_48877.phpt

@@ -17,7 +17,6 @@ $dbh->exec('CREATE TABLE test48877 (A integer)');
 $dbh->exec("INSERT INTO test48877 VALUES ('1')");
 $dbh->exec("INSERT INTO test48877 VALUES ('2')");
 $dbh->exec("INSERT INTO test48877 VALUES ('3')");
-$dbh->commit();
 
 $query = "SELECT * FROM test48877 WHERE A = :paramno";
 
@@ -32,7 +31,6 @@ var_dump($stmt->rowCount());
 $stmt = $dbh->prepare('DELETE FROM test48877');
 $stmt->execute();
 
-$dbh->commit();
 unset($stmt);
 unset($dbh);
 

ext/pdo_firebird/tests/bug_53280.phpt

@@ -13,7 +13,6 @@ require("testdb.inc");
 
 $dbh->exec('CREATE TABLE test53280(A VARCHAR(30), B VARCHAR(30), C VARCHAR(30))');
 $dbh->exec("INSERT INTO test53280 VALUES ('A', 'B', 'C')");
-$dbh->commit();
 
 $stmt1 = "SELECT B FROM test53280 WHERE A = ? AND B = ?";
 $stmt2 = "SELECT B, C FROM test53280 WHERE A = ? AND B = ?";
@@ -28,7 +27,6 @@ $stmth1->execute(array('A', 'B'));
 $rows = $stmth1->fetchAll(); // <------- segfault
 var_dump($rows);
 
-$dbh->commit();
 unset($stmth1);
 unset($stmth2);
 unset($stmt);

ext/pdo_firebird/tests/bug_62024.phpt

@@ -14,8 +14,6 @@ require("testdb.inc");
 $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
 $dbh->exec("CREATE TABLE test62024 (ID INTEGER NOT NULL, TEXT VARCHAR(10))");
 
-$dbh->commit();
-
 //start actual test
 
 $sql = "insert into test62024 (id, text) values (?, ?)";
@@ -30,15 +28,11 @@ var_dump($res);
 $res = $sttmt->execute($args_err);
 var_dump($res);
 
-$dbh->commit();
-
 
 //teardown test data
 $sttmt = $dbh->prepare('DELETE FROM test62024');
 $sttmt->execute();
 
-$dbh->commit();
-
 unset($sttmt);
 unset($dbh);
 

ext/pdo_firebird/tests/bug_64037.phpt

@@ -17,8 +17,6 @@ $dbh->exec("INSERT INTO test64037 (ID, TEXT, COST) VALUES (1, 'test', -1.0)");
 $dbh->exec("INSERT INTO test64037 (ID, TEXT, COST) VALUES (2, 'test', -0.99)");
 $dbh->exec("INSERT INTO test64037 (ID, TEXT, COST) VALUES (3, 'test', -1.01)");
 
-$dbh->commit();
-
 $query = "SELECT * from test64037 order by ID";
 $stmt = $dbh->prepare($query);
 $stmt->execute();
@@ -31,8 +29,6 @@ var_dump($rows[2]['COST']);
 $stmt = $dbh->prepare('DELETE FROM test64037');
 $stmt->execute();
 
-$dbh->commit();
-
 unset($stmt);
 unset($dbh);