Skip to content
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

Add mysqli_rollback_to #7818

Open
KapitanOczywisty opened this issue Dec 23, 2021 · 4 comments
Open

Add mysqli_rollback_to #7818

KapitanOczywisty opened this issue Dec 23, 2021 · 4 comments

Comments

@KapitanOczywisty
Copy link

KapitanOczywisty commented Dec 23, 2021

Description

In PHP 5.5 mysqli::savepoint and mysqli::release_savepoint were added, but not rollback_to. Maybe there is some database engine where savepoints have multiple purposes, but in mariadb the only use of savepoints is to call query ROLLBACK TO name manually.

For completion: existing rollback with $name is doing nothing (only adding comment) at least in mariadb/mysql.

Method signature

class mysqli { /* ... */
  public function rollback_to(string $name) : bool;
}

Sample test sequence

mysqli_query($link, 'DROP TABLE IF EXISTS test');
mysqli_query($link, 'CREATE TABLE test(id INT) ENGINE = InnoDB');

mysqli_begin_transaction($link);
mysqli_query($link, 'INSERT INTO test(id) VALUES (1)');
mysqli_savepoint($link, 'foo');
mysqli_query($link, 'INSERT INTO test(id) VALUES (2)');
mysqli_rollback_to($link, 'foo');
mysqli_commit($link);
$res = mysqli_query($link, "SELECT * FROM test");
var_dump($res->num_rows); // int(1)
@cmb69
Copy link
Member

cmb69 commented Dec 23, 2021

existing rollback with $name is doing nothing (only adding comment)

I wonder why that is only a comment. Wasn't ROLLBACK TO available on all supported servers when that had been implemented?

Anyhow, I think it makes sense to introduce this functionality; not sure about the details. It might be considered to re-purpose mysqli::rollback() for this.

@KapitanOczywisty
Copy link
Author

KapitanOczywisty commented Dec 23, 2021

I have no idea why this is implemented as a comment, maybe for debugging or to be similar to other api or maybe named transactions were planed at the time. Though I wasn't able to find anything similar being used in the wild. Also seems like ROLLBACK TO was available at that time, but there is distinction to mention:

ROLLBACK and ROLLBACK TO are in fact different statements - the first one is reverting whole transaction, the second one is only reverting to the named savepoint. ROLLBACK have options [NO] CHAIN and [NO] RELEASE ($flag argument) which ROLLBACK TO does not. It's probably better to keep them as separate functions.

Edit: if nobody knows what is the purpose of that comment maybe it's better to deprecate $name in begin_transaction/commit/rollback. In the worst case, somebody will tell us what this was for, after seeing deprecation warning 😏

@kamil-tekiela
Copy link
Member

kamil-tekiela commented May 20, 2022

I wouldn't support adding a new function that abstracts a very simple SQL command. There's no reason to do it in standard mysqli/mysqlnd. The only reason why we even have transaction-related methods in mysqli is because of this plugin: https://php-legacy-docs.zend.com/manual/php4/en/book.mysqlnd-ms I doubt anyone is actively using or maintaining this plugin, so we can't even ask them if such function would be useful to them. I don't have any other justification for adding this function.

Regarding deprecation, I wouldn't mind that, but a similar problem arises here. Do we know of any plugins actually using this param? PHP doesn't seem to use it nor does mysqlnd out of the box. Have any mysqlnd plugins ever made any use of it?

@KapitanOczywisty
Copy link
Author

mysqli::savepoint and mysqli::release_savepoint are pretty useless without mysqli::rollback_to, so deprecate former 2 or add rollback_to. And the reason to add rollback_to is to have exactly the same escaping/handling of the $name. This is like adding array_push without array_pop.

$name parameter in begin_transaction/commit/rollback is a separate issue, and as I've mentioned: deprecate that and see if anybody will notice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants