Feature: [pdo_firebird] Transaction management optimization #12741

SakiTakamachi · 2023-11-21T08:22:31Z

take2 of #12657

About Firebird transaction

Firebird is a full transactional database, so the DB itself does not support autocommit mode. (Strictly, there is an autocommit mode, but it is a different concept from the "autocommit" that we are used to with MySQL and others.)

Therefore, a transaction must have started before any operation is performed, and autocommit mode had to be emulated in php.

I made sure that a transaction always exists when in autocommit mode. Since the in_transacntion function does not work as expected, I have introduced H->in_manually_txn to determine whether a transaction is being manually manipulated.

There are two types of commit/rollback

(I'm not talking about two-phase commit. This change does not take into account two-phase commit.)

There are isc_commit_retaining which starts a transaction again in the same context immediately after committing, and isc_commit_transaction which closes the transaction as is.

Similarly, there are two types of rollback.

Due to the default value of the transaction isolation level, autocommit mode may obtain unintended results.
Regarding this, it would be too large to include support for transaction isolation levels in this PR, so I will leave it as is for now. (I'll address it with the following changes, I'll probably create a PR soon)

SakiTakamachi · 2023-11-21T11:52:03Z

@Girgias
This is related to #12545
Could you please review it when you have time?

SakiTakamachi · 2023-11-21T13:02:24Z

I forgot to use constants in a few places, so I'll fix them.

SakiTakamachi · 2023-11-21T13:31:18Z

done.

SakiTakamachi · 2023-11-22T00:52:09Z

When working with multiple instances, I found that keeping a transaction always started can lead to non-intuitive behavior depending on the isolation level. I'll think about it some more.

Girgias

I agree with the tests changes to remove the commit() method calls.

However, the C code is rather confusing, I'm struggling to see if functions prefixed by firebird_ are from the PHP driver or actual FireBird/Interbase APIs.

I think it would be better first, maybe in a different PR to rename the PHP driver related function to use the more typical php_ prefix. So the PHP driver functions would be prefixed by php_firebird_driver_ would hopefully make it easier to understand what is going on.

Also, I'm not a fan of a leading _ to mark the function "private" if it is static and in a .c file then it is private and limited to that file anyway.

Girgias · 2023-11-22T03:35:42Z

ext/pdo_firebird/firebird_driver.c

 {
 	pdo_firebird_db_handle *H = (pdo_firebird_db_handle *)dbh->driver_data;

-	if (isc_commit_transaction(H->isc_status, &H->tr)) {
-		firebird_error(dbh);
+	if (dbh->auto_commit && H->tr && !firebird_commit_transaction(dbh, FB_TXN_RELEASE)) {


Is this firebird_commit_transaction() function part of the FireBird API or the PHP Driver?

No, it's a replacement of _firebird_begin_transaction with an h file. It's quite difficult to understand....
As you pointed out, I should think about the naming a little more.

Girgias · 2023-11-22T03:36:39Z

ext/pdo_firebird/php_pdo_firebird_int.h

+extern bool _firebird_commit_transaction(pdo_dbh_t *dbh, bool retain);
+#define firebird_commit_transaction(d,r) _firebird_commit_transaction(d, r)


Suggested change

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_commit_transaction(pdo_dbh_t *dbh, bool retain);

Why not just this?

I misunderstood what the naming convention should be and it became complicated.

Girgias · 2023-11-22T03:37:49Z

ext/pdo_firebird/tests/ddl2.phpt

+$result = $dbh->query("SELECT * FROM test_ddl2");
+foreach ($result as $r) {
+    var_dump($r);
+}


Suggested change

$result = $dbh->query("SELECT * FROM test_ddl2");

foreach ($result as $r) {

var_dump($r);

}

$result = $dbh->query("SELECT * FROM test_ddl2");

var_dump($result);

As this is an empty array just having this be the expectation is clearer IMHO

SakiTakamachi · 2023-11-22T03:55:23Z

However, the C code is rather confusing, I'm struggling to see if functions prefixed by firebird_ are from the PHP driver or actual FireBird/Interbase APIs.

I think it would be better first, maybe in a different PR to rename the PHP driver related function to use the more typical php_ prefix. So the PHP driver functions would be prefixed by php_firebird_driver_ would hopefully make it easier to understand what is going on.

Also, I'm not a fan of a leading _ to mark the function "private" if it is static and in a .c file then it is private and limited to that file anyway.

Looking back on it, it seems quite difficult to understand...
First, as you suggested, I'll prepare a separate PR to clean up the existing naming conventions, and then start working on this PR again. thank you.

SakiTakamachi · 2023-11-22T04:31:00Z

@Girgias

There are other things that I'm confused about.

Firebird's transaction isolation level is repeatable read by default, so as it stands now, when in autocommit mode, it cannot read any changes committed from other transactions since new pdo().

To address this, I came up with three ideas:

At the beginning of handle_doer, reset the transaction
Set the transaction isolation level for autocommit mode to read-committed
Abandon the current implementation where transactions always exist, and use transactions in autocommit mode only in sections where transactions are necessary.

Personally, I think 2 has the lowest communication cost, but if you don't mind, could you give me your opinion?

Girgias · 2023-11-22T04:42:22Z

I'm not totally sure about what you mean, but shouldn't the isc_commit_retaining() calls be replaced to isc_commit_transaction() in auto commit mode? I feel this would possibly solve the issues?

SakiTakamachi · 2023-11-22T04:48:55Z

In that case, the transaction will remain closed and it will have to begin again at some point.(In autocommit mode)

I think it's probably easier to understand if I explain the reproduction case using php code, so please wait for a moment.

SakiTakamachi · 2023-11-22T05:38:13Z

I put together some patterns

Cases where the current implementation causes problems

<?php
$db = new PDO(/**/);
$db_other = new PDO(/**/);

$db_other->exec('CREATE TABLE test (val INT)');

$db->query('SELECT * FROM test');

output:

Fatal error: Uncaught PDOException: SQLSTATE[42S02]: Base table or view not found: -204 Dynamic SQL Error SQL error code = -204 Table unknown TEST At line 1, column 15 in <path to file>:7
Stack trace:
#0 /mount/ft.php(7): PDO->exec('SELECT * FROM t...')
#1 {main}
  thrown in <path to file> on line 7

Replace `isc_commit_retaining` with `isc_commit_transaction`

replace( in firebird_handle_doer):

firebird_commit_transaction(dbh, FB_TXN_RETAIN);

to

firebird_commit_transaction(dbh, FB_TXN_RELEASE);
_firebird_begin_transaction(dbh);

<?php
$db = new PDO(/**/);
$db_other = new PDO(/**/);

$db_other->exec('CREATE TABLE test (val INT)');

$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
$db->query('SELECT * FROM test');
var_dump($db->query('SELECT * FROM test'));

output:

Warning: PDO::exec(): SQLSTATE[42S02]: Base table or view not found: -204 Dynamic SQL Error SQL error code = -204 Table unknown TEST At line 1, column 15 in <path to file> on line 8

Warning: PDO::exec(): SQLSTATE[42S02]: Base table or view not found: -204 Dynamic SQL Error SQL error code = -204 Table unknown TEST At line 1, column 15 in <path to file> on line 9
bool(false)

When the isolation level is set to read committed

<?php
$db = new PDO(/**/);
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
$db->setAttribute(PDO::FB_TRANSACTION_ISOLATION_LEVEL, PDO::FB_READ_COMMITTED); // Implemented for verification

$db_other = new PDO(/**/);
$db_other->exec('CREATE TABLE test (val INT)');
$db_other->exec('INSERT INTO test VALUES (1)');

$r = $db->query('SELECT * FROM test');
foreach ($r as $row) {
    var_dump($row);
}

output:

array(2) {
  ["VAL"]=>
  int(1)
  [0]=>
  int(1)
}

Girgias · 2023-11-22T05:59:20Z

Right I see the issue, apparently the transaction isolation levels are an ANSI/ISO SQL spec, so those should possibly be added to PDO as a whole and not just limited to PDO_FireBird.

Also https://github.com/FirebirdSQL/jaybird-manual/blob/master/src/docs/asciidoc/chapters/transactions/transactions.adoc#auto-commit-mode was useful for me to understand some of the issues. (also I will note I am not a database expert and this is new to me)

SakiTakamachi · 2023-11-22T06:14:34Z

Thank you for taking a closer look!

apparently the transaction isolation levels are an ANSI/ISO SQL spec, so those should possibly be added to PDO as a whole and not just limited to PDO_FireBird.

For DBs other than firebird, transaction isolation levels can be set on a per-session basis or in global settings.

example MySQL:

mysql> SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;

https://dev.mysql.com/doc/refman/8.0/en/set-transaction.html

Therefore, non-Firebird DBs can manipulate transaction isolation levels by executing instructions directly in SQL.

Firebird cannot set the transaction isolation level "only" like MySQL.

Firebird specifies the isolation level when starting a transaction. If omitted, it becomes the default value, but the default value cannot be changed (as far as I know).

This is a change that I plan to next this pull request, but for these reasons, in the case of firebird, it is necessary to maintain the transaction isolation level setting value on the php side. So I think that the attribute value will be for only firebird.

SakiTakamachi · 2023-11-22T11:13:48Z

I think it would be better first, maybe in a different PR to rename the PHP driver related function to use the more typical php_ prefix. So the PHP driver functions would be prefixed by php_firebird_driver_ would hopefully make it easier to understand what is going on.

#12750

Done!

SakiTakamachi · 2023-11-22T12:11:18Z

I rebased on #12750 and make naming easier to understand.
Since the changes in #12750 are included, it may be best to leave it alone until #12750 is merged.

Girgias · 2023-11-22T17:16:33Z

ext/pdo_firebird/php_pdo_firebird_int.h

+#define FB_TXN_RETAIN 1
+#define FB_TXN_RELEASE 0


Possibly make this a C enum?

Done, I changed it.

SakiTakamachi · 2023-11-23T03:16:43Z

I rebased on master

Girgias · 2023-11-24T01:43:08Z

ext/pdo_firebird/firebird_driver.c

+	if (dbh->auto_commit && H->tr && !php_firebird_commit_transaction(dbh, FB_TXN_RELEASE)) {
+		return false;
+	}


I'm not sure what this if block checks for.

If I understand properly, does this check that the current "active" FireBird transcation is empty? Anycase having comments would be helpful

Girgias · 2023-11-24T01:44:32Z

ext/pdo_firebird/php_pdo_firebird_int.h

+enum {
+	FB_TXN_RELEASE,
+	FB_TXN_RETAIN
+};


Requires a typedef and this type to be passed to php_firebird_commit_transaction instead of bool, or am I misunderstanding something here?

I thought it would be difficult to understand if it was just a bool value, so I made it a constant.

php_firebird_commit_transaction(dbh, true) // retain php_firebird_commit_transaction(dbh, false) // not retain

Maybe I'm overthinking it a bit, and maybe just passing a bool is fine...

I will try to stop using constants (Enums).

You can write:

php_firebird_commit_transaction(dbh, /* retain */ retain_var_or_const);

To imitate named arguments

It's a good idea! Thank you!

Girgias · 2023-11-24T01:45:43Z

ext/pdo_firebird/firebird_driver.c

 {
 	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 (dbh->auto_commit && H->tr && !php_firebird_commit_transaction(dbh, FB_TXN_RELEASE)) {


Suggested change

if (dbh->auto_commit && H->tr && !php_firebird_commit_transaction(dbh, FB_TXN_RELEASE)) {

php_firebird_commit_transaction(dbh, FB_TXN_RELEASE)

I'm confused, FB_TXN_RELEASE is it's own enum and not a bool, so this is slightly confusing

Girgias · 2023-11-24T01:46:21Z

ext/pdo_firebird/firebird_driver.c

+/* 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;
+
+	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;
+}
+/* }}} */


Can you add comments to explain why the two different behaviours are needed?

SakiTakamachi · 2023-11-24T02:41:52Z

Added some comments. Also, if we try to change the autocommit mode while a transaction has been manually started, an error will now occur.

SakiTakamachi · 2023-11-24T02:56:05Z

ext/pdo_firebird/firebird_driver.c

+	if (dbh->auto_commit && H->tr) {
+		if (!php_firebird_commit_transaction(dbh, /* release */ false)) {
+			return false;
+		}
+	}
+
+	if (!php_firebird_begin_transaction(dbh)) {
 		return false;
 	}


The reason I don't set this to retain is because I want to change the isolation level when I introduce it in the next PR.

SakiTakamachi · 2023-11-24T04:04:24Z

After thinking about it, I shouldn't be retaining when returning from a manual transaction to autocommit, so I'll fix that. Along with that, there is no place to use retain for rollback, so delete it.

SakiTakamachi · 2023-11-24T04:13:46Z

done

SakiTakamachi · 2023-11-24T05:18:46Z

added some tests

Girgias

Looks sensible to me now, thanks for tackling this and all the explanations

SakiTakamachi · 2023-11-27T03:49:15Z

Thank you!

github-actions bot added Extension: pdo (core) Extension: pdo_firebird labels Nov 21, 2023

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_manage branch 2 times, most recently from ad40a7b to 5df9266 Compare November 21, 2023 09:41

SakiTakamachi marked this pull request as ready for review November 21, 2023 09:58

SakiTakamachi mentioned this pull request Nov 21, 2023

See if this fixes Firbird parallel issues #12545

Closed

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_manage branch from 5df9266 to 0765b34 Compare November 21, 2023 13:31

SakiTakamachi marked this pull request as draft November 22, 2023 00:52

Girgias reviewed Nov 22, 2023

View reviewed changes

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_manage branch 3 times, most recently from 89b6b3d to 5d69e85 Compare November 22, 2023 12:08

Girgias reviewed Nov 22, 2023

View reviewed changes

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_manage branch 2 times, most recently from 095188a to 0bc6851 Compare November 23, 2023 03:09

SakiTakamachi marked this pull request as ready for review November 23, 2023 04:30

Girgias reviewed Nov 24, 2023

View reviewed changes

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_manage branch from 0bc6851 to 4764236 Compare November 24, 2023 02:39

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_manage branch from 4764236 to 751c796 Compare November 24, 2023 02:43

SakiTakamachi commented Nov 24, 2023

View reviewed changes

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_manage branch 2 times, most recently from ec2f6a4 to 278593f Compare November 24, 2023 05:18

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_manage branch 4 times, most recently from 16962f7 to 7d907fc Compare November 25, 2023 12:52

[pdo_firebird] Transaction management optimization

c6881cb

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_manage branch from 7d907fc to c6881cb Compare November 26, 2023 06:07

Girgias approved these changes Nov 27, 2023

View reviewed changes

Girgias merged commit dfaf798 into php:master Nov 27, 2023

SakiTakamachi deleted the feature/pdo_firebird_transaction_manage branch November 28, 2023 04:42

SakiTakamachi mentioned this pull request Jan 21, 2024

Firebird PDO begintransaction fails after query #8735

Closed

		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_commit_transaction(pdo_dbh_t *dbh, bool retain);
	#define firebird_commit_transaction(d,r) _firebird_commit_transaction(d, r)
	extern bool firebird_commit_transaction(pdo_dbh_t *dbh, bool retain);

	if (dbh->auto_commit && H->tr && !php_firebird_commit_transaction(dbh, FB_TXN_RELEASE)) {
	php_firebird_commit_transaction(dbh, FB_TXN_RELEASE)

Feature: [pdo_firebird] Transaction management optimization #12741

Feature: [pdo_firebird] Transaction management optimization #12741

Conversation

SakiTakamachi commented Nov 21, 2023 • edited Loading

About Firebird transaction

There are two types of commit/rollback

SakiTakamachi commented Nov 21, 2023

SakiTakamachi commented Nov 21, 2023

SakiTakamachi commented Nov 21, 2023

SakiTakamachi commented Nov 22, 2023 • edited Loading

Girgias 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

SakiTakamachi commented Nov 22, 2023

SakiTakamachi commented Nov 22, 2023

Girgias commented Nov 22, 2023

SakiTakamachi commented Nov 22, 2023

SakiTakamachi commented Nov 22, 2023 • edited by Girgias Loading

Girgias commented Nov 22, 2023

SakiTakamachi commented Nov 22, 2023 • edited Loading

SakiTakamachi commented Nov 22, 2023

SakiTakamachi commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SakiTakamachi commented Nov 23, 2023

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

SakiTakamachi commented Nov 24, 2023

Choose a reason for hiding this comment

SakiTakamachi commented Nov 24, 2023

SakiTakamachi commented Nov 24, 2023

SakiTakamachi commented Nov 24, 2023

Girgias left a comment

Choose a reason for hiding this comment

SakiTakamachi commented Nov 27, 2023

SakiTakamachi commented Nov 21, 2023 •

edited

Loading

SakiTakamachi commented Nov 22, 2023 •

edited

Loading

SakiTakamachi commented Nov 22, 2023 •

edited by Girgias

Loading

SakiTakamachi commented Nov 22, 2023 •

edited

Loading