Feature: Add transaction isolation level and mode settings to `pdo_firebird` #12815

SakiTakamachi · 2023-11-28T14:07:24Z

take2 of #12664

About Firebird transaction isolation levels

Firebird's transaction isolation level can only be set at the beginning of a transaction. Therefore, I saved the user's settings on the php side and used them when starting a transaction.
Default is repeatable lead.

Firebird's `read committed` has three types of behavior

Load committed data. It works the same as MySQL etc.
If there is in-doubt data that has not yet been committed, wait until it is committed or rolled back.
If there is in-doubt data that has not yet been committed, it immediately raises an error as a deadlock.

For this pull request, I adopted 1, which is a common read committed.

It is possible to switch between 2 and 3 by attribute value, but I have decided that there is no need to implement it yet as I do not know the demand.

About access mode

There are writable and read-only modes. Like isolation levels, this can only be set at the beginning of a transaction. In the same way, the setting value is retained on the php side.
Default is writable.

Auto commit mode

Firebird's autocommit mode is emulated by PHP, which actually always has a transaction open. If a transaction in autocommit mode is run at a high isolation level, it will not be able to read any changes made by other transactions after new PDO().

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

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

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

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

Therefore, I made the autocommit mode to always work with read committed, regardless of the user's configured value.

ext/pdo_firebird/firebird_driver.c

SakiTakamachi · 2023-11-28T14:33:39Z

ext/pdo_firebird/firebird_driver.c

+	) {
+		H->txn_isolation_level = txn_isolation_level;
+	} else {
+		H->txn_isolation_level = PDO_FB_REPEATABLE_READ;


Should I make it an error?

I took a closer look, and the ret value is a mess. I don't know what it want to do by returning -1.

I would like to leave this as is for now and fix it all together in a separate pull request.

Can this case ever happen? The attribute should be controlled by the driver and only return valid values. So I would probably make this an assertion.

This value is set by users, so any value that is an int will come here. Therefore, it is quite possible that values other than the three constant values will be passed.

factory may need validation and errors as same as set attr.

I would possibly throw a ValueError as trying to use an invalid transaction isolation mode is a programming error.

Both the constructor and set attr have been fixed to cause a value error.
b7d932f

However, I am a little confused about whether it would be better to write zend_restore_error_handling in pdo core.

Ah, zend_value_error is better. I'll fix it.

ext/pdo_firebird/firebird_driver.c

SakiTakamachi · 2023-11-28T14:41:28Z

@Girgias

Could you please review it when you have time?

Girgias · 2023-12-01T10:11:34Z

ext/pdo_firebird/firebird_driver.c

@@ -822,7 +827,7 @@ static bool firebird_handle_manually_begin(pdo_dbh_t *dbh) /* {{{ */
 		}
 	}

-	if (!php_firebird_begin_transaction(dbh)) {
+	if (!php_firebird_begin_transaction(dbh, /* manually */ false)) {


Suggested change

if (!php_firebird_begin_transaction(dbh, /* manually */ false)) {

if (!php_firebird_begin_transaction(dbh, /* auto commit mode */ false)) {

Fixed on e1fd726

retain comment was also difficult to understand, so I fixed it as well.

Girgias · 2023-12-01T10:12:46Z

ext/pdo_firebird/firebird_driver.c

+						pdo_raise_impl_error(dbh, NULL, "HY000",
+							"Transaction isolation level must be PDO::FB_READ_COMMITTED, PDO::FB_REPEATABLE_READ, or PDO::PDO_FB_SERIALIZABLE");
+						return false;


I'd throw a ValueError instead, as this is a Firebird specific attribute.

Girgias · 2023-12-01T10:24:19Z

ext/pdo_firebird/firebird_driver.c

-			*ptpb++ = isc_tpb_write;
-			dbh->transaction_flags &= ~(PDO_TRANS_ACCESS_MODE^PDO_TRANS_READWRITE);
+	/* isc_xxx are all 1 byte. */
+	char tpb[5] = { isc_tpb_version3 }, *ptpb = tpb + 1;


Suggested change

char tpb[5] = { isc_tpb_version3 }, *ptpb = tpb + 1;

/* tpb means "Transaction Parameter Buffer" */

char tpb[] = { isc_tpb_version3 };

char *ptpb = tpb + 1;

The tpb might not be consistent across versions?

I don't really get how the pointer is being initialized and also why it is updated twice sometimes, as it will overwrite the table?

If this is intended, then using tpb[1] = isc_tpb_read_committed is possibly easier to understand. But comments around the Interbase/Firebird API would be helpful as this is confusing.

If it is not an API with the FB_ prefix, it seems safe to assume that there is no difference between versions. Therefore, there are no differences between versions in the tpb specifications.

I think I can write it a little more clearly, so I'll try rewriting it.

I tried to rewrite it a little easier to understand.
efa0d98

The setting add more 1 byte for read committed, because firebird's read committed supports multiple modes of behavior.

isc_tpb_rec_version is the mode for reading from committed data.
The opposite setting is isc_tpb_no_rec_version. This is a mode in which uncommitted data waits for it to be committed (or rolled back) and then read.

Detailed documentation on these:
https://docwiki.embarcadero.com/InterBase/2020/en/Specifying_Isolation_Level

FYI, when reading some times within a transaction, the phenomenon where different data is read due to updates by other transactions is called "Phantom Read".
(I deleted the comment about Phantom Read because it might have been difficult to understand.)

@Girgias
Using strlen on char arrays doesn't seem to work well on windows. (It may be wrong to use strlen for arrays in the first place.)
I rewrote it for now and the test passed, but which one seems better, the one using the pointer?

I think this version is clearer

Girgias · 2023-12-01T10:25:38Z

ext/pdo_firebird/firebird_driver.c

+	) {
+		H->txn_isolation_level = txn_isolation_level;
+	} else {
+		H->txn_isolation_level = PDO_FB_REPEATABLE_READ;


Can this case ever happen? The attribute should be controlled by the driver and only return valid values. So I would probably make this an assertion.

SakiTakamachi · 2023-12-01T14:24:01Z

I made a mistake. It's late today, so I'll fix it tomorrow.

SakiTakamachi · 2023-12-02T08:24:39Z

~~done~~

~~oops....~~

Passed!

Girgias · 2023-12-04T16:07:21Z

ext/pdo_firebird/firebird_driver.c

+			*/
+			case PDO_FB_READ_COMMITTED:
+				tpb[2] = isc_tpb_read_committed;
+				/* Not wait to commit indeterminate data. This option required only with `read committed`.*/


Suggested change

/* Not wait to commit indeterminate data. This option required only with `read committed`.*/

/* Do not wait to commit indeterminate data. This option only required with `read committed`. */

Not sure what you mean by "indeterminate" ?

I don't know why the sentence is like this....Maybe I was tired.

It should have been a comment like this, I'll fix it.

Ignore indeterminate data from other transactions.

Indeterminate data from other transactions means data that has not been committed or rolled back in other transactions.

done

04235aa

Girgias · 2023-12-04T16:08:43Z

ext/pdo_firebird/firebird_driver.c

+	) {
+		H->txn_isolation_level = txn_isolation_level;
+	} else {
+		H->txn_isolation_level = PDO_FB_REPEATABLE_READ;


I would possibly throw a ValueError as trying to use an invalid transaction isolation mode is a programming error.

…B_TRANSACTION_ISOLATION_LEVEL.

Girgias

Makes sense to me, can you add an entry to UPGRADING/NEWS? :)

Girgias · 2023-12-05T04:22:10Z

ext/pdo_firebird/firebird_driver.c

+						zend_value_error("PDO::FB_TRANSACTION_ISOLATION_LEVEL must be a valid transaction isolation level "
+							"(PDO::FB_READ_COMMITTED, PDO::FB_REPEATABLE_READ, or PDO::FB_SERIALIZABLE)");


This is just a possible follow-up suggestion to extract the common error message into a function so that they don't go out of sync.

SakiTakamachi · 2023-12-05T06:21:24Z

@Girgias
Thanks! I added to UPGRADING/NEWS on af3de5e

SakiTakamachi · 2023-12-07T23:00:57Z

Thanks!

github-actions bot added the Extension: pdo_firebird label Nov 28, 2023

SakiTakamachi commented Nov 28, 2023

View reviewed changes

ext/pdo_firebird/firebird_driver.c Show resolved Hide resolved

Girgias reviewed Dec 1, 2023

View reviewed changes

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_isolation_level_and_mode_2 branch 2 times, most recently from cdca462 to ac8516a Compare December 2, 2023 07:54

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_isolation_level_and_mode_2 branch 2 times, most recently from b887eea to 55c7be2 Compare December 2, 2023 12:02

Girgias reviewed Dec 4, 2023

View reviewed changes

SakiTakamachi added 2 commits December 5, 2023 09:14

Added transaction isolation level and access mode

00dbd49

be consistent in comments

4967d08

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_isolation_level_and_mode_2 branch 2 times, most recently from a70544b to 2cebb13 Compare December 5, 2023 00:15

Made transaction isolation level settings easier to understand

b3caa01

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_isolation_level_and_mode_2 branch from 2cebb13 to b3caa01 Compare December 5, 2023 00:58

change to raise a value error if an invalid value is passed to PDO::F…

04235aa

…B_TRANSACTION_ISOLATION_LEVEL.

SakiTakamachi force-pushed the feature/pdo_firebird_transaction_isolation_level_and_mode_2 branch from b7d932f to 04235aa Compare December 5, 2023 01:59

Girgias approved these changes Dec 5, 2023

View reviewed changes

[ci skip] NEWS / UPGRADING

af3de5e

Girgias merged commit 834cb64 into php:master Dec 7, 2023

SakiTakamachi deleted the feature/pdo_firebird_transaction_isolation_level_and_mode_2 branch December 7, 2023 23:01

SakiTakamachi mentioned this pull request Dec 20, 2023

Implement PDO driver specific sub-classes #12804

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add transaction isolation level and mode settings to `pdo_firebird` #12815

Feature: Add transaction isolation level and mode settings to `pdo_firebird` #12815

SakiTakamachi commented Nov 28, 2023 •

edited

Loading

SakiTakamachi Nov 28, 2023

SakiTakamachi Nov 29, 2023 •

edited

Loading

Girgias Dec 1, 2023

SakiTakamachi Dec 1, 2023

Girgias Dec 4, 2023

SakiTakamachi Dec 5, 2023

SakiTakamachi Dec 5, 2023

SakiTakamachi Dec 5, 2023

SakiTakamachi commented Nov 28, 2023

Girgias Dec 1, 2023

SakiTakamachi Dec 1, 2023

Girgias Dec 1, 2023

Girgias Dec 1, 2023

SakiTakamachi Dec 1, 2023

SakiTakamachi Dec 1, 2023 •

edited

Loading

SakiTakamachi Dec 1, 2023

SakiTakamachi Dec 2, 2023 •

edited

Loading

Girgias Dec 4, 2023

Girgias Dec 1, 2023

SakiTakamachi commented Dec 1, 2023

SakiTakamachi commented Dec 2, 2023 •

edited

Loading

Girgias Dec 4, 2023

SakiTakamachi Dec 4, 2023 •

edited

Loading

SakiTakamachi Dec 5, 2023 •

edited

Loading

Girgias Dec 4, 2023

Girgias left a comment

Girgias Dec 5, 2023

SakiTakamachi commented Dec 5, 2023 •

edited

Loading

SakiTakamachi commented Dec 7, 2023

	if (!php_firebird_begin_transaction(dbh, /* manually */ false)) {
	if (!php_firebird_begin_transaction(dbh, /* auto commit mode */ false)) {

-	char tpb[5] = { isc_tpb_version3 }, *ptpb = tpb + 1;
+	/* tpb means "Transaction Parameter Buffer" */
+	char tpb[] = { isc_tpb_version3 };
+	char *ptpb = tpb + 1;

	/* Not wait to commit indeterminate data. This option required only with `read committed`.*/
	/* Do not wait to commit indeterminate data. This option only required with `read committed`. */

		zend_value_error("PDO::FB_TRANSACTION_ISOLATION_LEVEL must be a valid transaction isolation level "
		"(PDO::FB_READ_COMMITTED, PDO::FB_REPEATABLE_READ, or PDO::FB_SERIALIZABLE)");

Feature: Add transaction isolation level and mode settings to pdo_firebird #12815

Feature: Add transaction isolation level and mode settings to pdo_firebird #12815

Conversation

SakiTakamachi commented Nov 28, 2023 • edited Loading

About Firebird transaction isolation levels

Firebird's read committed has three types of behavior

About access mode

Auto commit mode

Choose a reason for hiding this comment

SakiTakamachi Nov 29, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SakiTakamachi commented Nov 28, 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

SakiTakamachi Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SakiTakamachi Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SakiTakamachi commented Dec 1, 2023

SakiTakamachi commented Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

SakiTakamachi Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

SakiTakamachi Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SakiTakamachi commented Dec 5, 2023 • edited Loading

SakiTakamachi commented Dec 7, 2023

Feature: Add transaction isolation level and mode settings to `pdo_firebird` #12815

Feature: Add transaction isolation level and mode settings to `pdo_firebird` #12815

SakiTakamachi commented Nov 28, 2023 •

edited

Loading

Firebird's `read committed` has three types of behavior

SakiTakamachi Nov 29, 2023 •

edited

Loading

SakiTakamachi Dec 1, 2023 •

edited

Loading

SakiTakamachi Dec 2, 2023 •

edited

Loading

SakiTakamachi commented Dec 2, 2023 •

edited

Loading

SakiTakamachi Dec 4, 2023 •

edited

Loading

SakiTakamachi Dec 5, 2023 •

edited

Loading

SakiTakamachi commented Dec 5, 2023 •

edited

Loading