Skip to content

Better PDO::inTransaction #4996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ext/mysqlnd/mysqlnd_libmysql_compat.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
#define mysql_field_tell(r) mysqlnd_field_tell((r))
#define mysql_init(a) mysqlnd_connection_init((a), false)
#define mysql_insert_id(r) mysqlnd_insert_id((r))
#define mysql_get_server_status(r) mysqlnd_get_server_status((r))
#define mysql_kill(r,n) mysqlnd_kill((r), (n))
#define mysql_list_dbs(c, wild) mysqlnd_list_dbs((c), (wild))
#define mysql_list_processes(c) mysqlnd_list_processes((c))
Expand All @@ -80,6 +81,7 @@
#define mysql_stmt_param_count(s) mysqlnd_stmt_param_count((s))
#define mysql_stmt_num_rows(s) mysqlnd_stmt_num_rows((s))
#define mysql_stmt_insert_id(s) mysqlnd_stmt_insert_id((s))
#define mysql_stmt_server_status(s) mysqlnd_stmt_server_status((s))
#define mysql_stmt_close(s) mysqlnd_stmt_close((s))
#define mysql_stmt_bind_param(s,b) mysqlnd_stmt_bind_param((s), (b))
#define mysql_stmt_bind_result(s,b) mysqlnd_stmt_bind_result((s), (b))
Expand Down
11 changes: 10 additions & 1 deletion ext/pdo_mysql/mysql_driver.c
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,15 @@ static void pdo_mysql_request_shutdown(pdo_dbh_t *dbh)
}
/* }}} */

/* {{{ pdo_mysql_in_transaction */
static int pdo_mysql_in_transaction(pdo_dbh_t *dbh)
{
pdo_mysql_db_handle *H = (pdo_mysql_db_handle *)dbh->driver_data;
PDO_DBG_ENTER("pdo_mysql_in_transaction");
PDO_DBG_RETURN(!!(mysql_get_server_status(H->server) & SERVER_STATUS_IN_TRANS));
}
/* }}} */

/* {{{ mysql_methods */
static const struct pdo_dbh_methods mysql_methods = {
mysql_handle_closer,
Expand All @@ -544,7 +553,7 @@ static const struct pdo_dbh_methods mysql_methods = {
pdo_mysql_check_liveness,
NULL,
pdo_mysql_request_shutdown,
NULL
pdo_mysql_in_transaction
};
/* }}} */

Expand Down
68 changes: 68 additions & 0 deletions ext/pdo_mysql/tests/pdo_mysql_inTransaction.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
--TEST--
MySQL PDO class inTransaction
--SKIPIF--
<?php
if (!extension_loaded('pdo') || !extension_loaded('pdo_mysql')) die('skip not loaded');
require __DIR__ . '/config.inc';
require __DIR__ . '/../../../ext/pdo/tests/pdo_test.inc';
PDOTest::skip();
?>
--FILE--
<?php
require __DIR__ . '/../../../ext/pdo/tests/pdo_test.inc';

const BEGIN = ['BEGIN', 'START TRANSACTION'];
const END = ['COMMIT', 'ROLLBACK'];

$db = PDOTest::test_factory(__DIR__ . '/common.phpt');
// $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); // mysql does not support
for ($b = 0; $b < count(BEGIN); $b++) {
for ($e = 0; $e < count(END); $e++) {
foreach (['exec', 'query', 'execute'] as $w) {
foreach ([BEGIN[$b], END[$e]] as $command) {
switch ($w) {
case 'exec':
$db->exec($command);
break;
case'query':
$db->query($command)->execute();
break;
case 'execute':
/* EMULATE_PREPARES = QUERY */
$db->prepare($command)->execute();
break;
default:
assert(0);
}
var_dump($db->inTransaction());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does consequent calls hit the server again and again?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does consequent calls hit the server again and again?

Do you mean that whether this call requires network IO? The MySQL server will tell the client if it is in a transaction in every response by a flag, so we can get the latest status no matter what command is executed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that whether this call requires network IO?

yes

The MySQL server will tell the client if it is in a transaction in every response by a flag, so we can get the latest status no matter what command is executed

so does this query this local flag or did it require network IO every time?

Copy link
Member Author

@twose twose Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not required, before sending a new request, the state of the transaction will not change, so the state we got from the last response is reliable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twose Thanks, I already hit this several times, your PR looks very good, 👍

}
}
}
}

?>
--EXPECT--
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)
bool(true)
bool(false)