[pdo_firebird] Transaction management optimization

Author: SakiTakamachi

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

@@ -33,6 +33,7 @@
 
 static int php_firebird_alloc_prepare_stmt(pdo_dbh_t*, const zend_string*, XSQLDA*, isc_stmt_handle*,
 	HashTable*);
+static bool php_firebird_rollback_transaction(pdo_dbh_t *dbh, bool retain);
 
 const char CHR_LETTER = 1;
 const char CHR_DIGIT = 2;
@@ -526,17 +527,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)) {
-				php_firebird_error(dbh);
-			}
+			php_firebird_commit_transaction(dbh, /* release */ false);
 		} else {
-			if (isc_rollback_transaction(H->isc_status, &H->tr)) {
-				php_firebird_error(dbh);
-			}
+			php_firebird_rollback_transaction(dbh, /* release */ false);
 		}
 	}
+	H->in_manually_txn = 0;
 
 	if (isc_detach_database(H->isc_status, &H->db)) {
 		php_firebird_error(dbh);
@@ -702,9 +700,10 @@ 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)) {
-		php_firebird_error(dbh);
+	if (dbh->auto_commit && !H->in_manually_txn) {
+		if (!php_firebird_commit_transaction(dbh, /* retain */ true)) {
+			ret = -1;
+		}
 	}
 
 free_statement:
@@ -756,8 +755,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) /* {{{ */
+/* php_firebird_begin_transaction */
+static bool php_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;
@@ -809,28 +808,105 @@ static bool firebird_handle_begin(pdo_dbh_t *dbh) /* {{{ */
 }
 /* }}} */
 
-/* called by PDO to commit a transaction */
-static bool firebird_handle_commit(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 (isc_commit_transaction(H->isc_status, &H->tr)) {
-		php_firebird_error(dbh);
+	/**
+	 * If in autocommit mode and in transaction, we will need to close the transaction once.
+	 */
+	if (dbh->auto_commit && H->tr) {
+		if (!php_firebird_commit_transaction(dbh, /* release */ false)) {
+			return false;
+		}
+	}
+
+	if (!php_firebird_begin_transaction(dbh)) {
 		return false;
 	}
+	H->in_manually_txn = 1;
+	return true;
+}
+/* }}} */
+
+/* php_firebird_commit_transaction */
+bool php_firebird_commit_transaction(pdo_dbh_t *dbh, bool retain) /* {{{ */
+{
+	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
+
+	/**
+	 * `retaining` keeps the transaction open without closing it.
+	 *
+	 * firebird needs to always have a transaction open to emulate autocommit mode,
+	 * and in autocommit mode it keeps the transaction open.
+	 *
+	 * Same as close and then begin again, but use retain to save overhead.
+	 */
+	if (retain) {
+		if (isc_commit_retaining(H->isc_status, &H->tr)) {
+			php_firebird_error(dbh);
+			return false;
+		}
+	} else {
+		if (isc_commit_transaction(H->isc_status, &H->tr)) {
+			php_firebird_error(dbh);
+			return false;
+		}
+	}
+	return true;
+}
+/* }}} */
+
+/* called by PDO to commit a transaction */
+static bool firebird_handle_manually_commit(pdo_dbh_t *dbh) /* {{{ */
+{
+	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
+	if (!php_firebird_commit_transaction(dbh, dbh->auto_commit)) {
+		return false;
+	}
+	H->in_manually_txn = 0;
+	return true;
+}
+/* }}} */
+
+/* php_firebird_rollback_transaction */
+static bool php_firebird_rollback_transaction(pdo_dbh_t *dbh, bool retain) /* {{{ */
+{
+	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
+
+	/**
+	 * `retaining` keeps the transaction open without closing it.
+	 *
+	 * firebird needs to always have a transaction open to emulate autocommit mode,
+	 * and in autocommit mode it keeps the transaction open.
+	 *
+	 * Same as close and then begin again, but use retain to save overhead.
+	 */
+	if (retain) {
+		if (isc_rollback_retaining(H->isc_status, &H->tr)) {
+			php_firebird_error(dbh);
+			return false;
+		}
+	} else {
+		if (isc_rollback_transaction(H->isc_status, &H->tr)) {
+			php_firebird_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)) {
-		php_firebird_error(dbh);
+	if (!php_firebird_rollback_transaction(dbh, dbh->auto_commit)) {
 		return false;
 	}
+	H->in_manually_txn = 0;
 	return true;
 }
 /* }}} */
@@ -848,16 +924,6 @@ static int php_firebird_alloc_prepare_stmt(pdo_dbh_t *dbh, const zend_string *sq
 		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)) {
 		php_firebird_error(dbh);
@@ -898,21 +964,31 @@ static bool pdo_firebird_set_attribute(pdo_dbh_t *dbh, zend_long attr, zval *val
 					return false;
 				}
 
+				if (H->in_manually_txn) {
+					/* change auto commit mode with an open transaction is illegal, because
+						we won't know what to do with it */
+					const char *msg = "Cannot change autocommit mode while a transaction is already open";
+					php_firebird_error_with_info(dbh, "HY000", strlen("HY000"), msg, strlen(msg));
+					return false;
+				}
+
 				/* ignore if the new value equals the old one */
 				if (dbh->auto_commit ^ bval) {
-					if (dbh->in_txn) {
-						if (bval) {
-							/* turning on auto_commit with an open transaction is illegal, because
-							   we won't know what to do with it */
-							const char *msg = "Cannot enable auto-commit while a transaction is already open";
-							php_firebird_error_with_info(dbh, "HY000", strlen("HY000"), msg, strlen(msg));
-							return false;
-						} else {
-							/* close the transaction */
-							if (!firebird_handle_commit(dbh)) {
-								break;
+					if (bval) {
+						/* change to auto commit mode */
+						/* If the transaction is not started, start it. */
+						if (!H->tr) {
+							if (!php_firebird_begin_transaction(dbh)) {
+								return false;
+							}
+						}
+					} else {
+						/* change to not auto commit mode */
+						/* close the transaction if exists */
+						if (H->tr) {
+							if (!php_firebird_commit_transaction(dbh, /* release */ false)) {
+								return false;
 							}
-							dbh->in_txn = false;
 						}
 					}
 					dbh->auto_commit = bval;
@@ -1058,22 +1134,35 @@ static void pdo_firebird_fetch_error_func(pdo_dbh_t *dbh, pdo_stmt_t *stmt, zval
 }
 /* }}} */
 
+/* {{{ firebird_in_manually_transaction */
+static bool pdo_firebird_in_manually_transaction(pdo_dbh_t *dbh)
+{
+	/**
+	 * we can tell if a transaction exists now by checking H->tr,
+	 * but which will always be true in autocommit mode.
+	 * So this function checks if there is currently a "manually begun transaction".
+	 */
+	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;
+	return 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,
 	pdo_firebird_set_attribute,
 	NULL, /* last_id not supported */
 	pdo_firebird_fetch_error_func,
 	pdo_firebird_get_attribute,
 	NULL, /* check_liveness */
 	NULL, /* get driver methods */
 	NULL, /* request shutdown */
-	NULL, /* in transaction, use PDO's internal tracking mechanism */
+	pdo_firebird_in_manually_transaction,
 	NULL /* get gc */
 };
 /* }}} */
@@ -1154,6 +1243,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 = php_firebird_begin_transaction(dbh);
+	}
+
 	if (!ret) {
 		firebird_handle_closer(dbh);
 	}

ext/pdo_firebird/firebird_statement.c

@@ -180,8 +180,7 @@ static int pdo_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 && !php_firebird_commit_transaction(stmt->dbh, /* retain */ true)) {
 			break;
 		}
 

ext/pdo_firebird/php_pdo_firebird_int.h

@@ -66,7 +66,6 @@ typedef struct {
 } pdo_firebird_error_info;
 
 typedef struct {
-
 	/* the result of the last API call */
 	ISC_STATUS isc_status[20];
 
@@ -75,6 +74,7 @@ typedef struct {
 
 	/* the transaction handle */
 	isc_tr_handle tr;
+	bool in_manually_txn;
 
 	/* date and time format strings, can be set by the set_attribute method */
 	char *date_format;
@@ -93,7 +93,6 @@ typedef struct {
 
 
 typedef struct {
-
 	/* the link that owns this statement */
 	pdo_firebird_db_handle *H;
 
@@ -122,7 +121,6 @@ typedef struct {
 
 	/* the output SQLDA */
 	XSQLDA out_sqlda; /* last member */
-
 } pdo_firebird_stmt;
 
 extern const pdo_driver_t pdo_firebird_driver;
@@ -136,6 +134,8 @@ extern void php_firebird_set_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char
 #define php_firebird_error_with_info(d,e,el,m,ml) php_firebird_set_error(d, NULL, e, el, m, ml)
 #define php_firebird_error_stmt_with_info(s,e,el,m,ml) php_firebird_set_error(s->dbh, s, e, el, m, ml)
 
+extern bool php_firebird_commit_transaction(pdo_dbh_t *dbh, bool retain);
+
 enum {
 	PDO_FB_ATTR_DATE_FORMAT = PDO_ATTR_DRIVER_SPECIFIC,
 	PDO_FB_ATTR_TIME_FORMAT,

ext/pdo_firebird/tests/bug_47415.phpt

@@ -16,8 +16,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);
@@ -32,8 +30,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

@@ -18,7 +18,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";
 
@@ -33,7 +32,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

@@ -14,7 +14,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 = ?";
@@ -29,7 +28,6 @@ $stmth1->execute(array('A', 'B'));
 $rows = $stmth1->fetchAll(); // <------- segfault
 var_dump($rows);
 
-$dbh->commit();
 unset($stmth1);
 unset($stmth2);
 unset($stmt);