Everything

php

[Proposal] Add `savepoint()` method to PDO

php.internals

Saki Takamachi

2 years ago

Hi internals,

I'm thinking of adding a `savepoint(string $name)` method to PDO. Similarly, add `releaseSavepoint(string $name)` and change the signature of `rollback()` to `rollback(?string $name = null)`.

Savepoint is a feature defined in SQL99, so it is a feature supported by many databases. Therefore, I thought it would be a good idea to support this with PDO.

If we use `savepoint()` before a transaction has started, it is expected that a `PDOException` will be thrown. Similarly, if we use `releaseSavepoint()` or `rollback('savepoint_name’)` when there is no savepoint, an exception will be thrown.

Only pdo_odbc cannot predict what kind of database will be used, but in the odbc3 API, we can use `supportsSavepoints()` to find out whether the database supports savepoints.

FYI, mysqli already has support for savepoints.

I would like to know your opinion.

Regard.

Saki

Kamil Tekiela

2 years ago

Hi Saki,

Why does this have to be done in PHP? Why can this not be done in SQL?

Also, I am not sure that mysqli implements this properly. It's
probably not the best extension to take an example of.

Regards,
Kamil

Saki Takamachi

2 years ago

Hi Kamil,

> Why does this have to be done in PHP? Why can this not be done in SQL?

Of course, we can also do it with SQL. Indeed, many frameworks do that. However, if we think about it from that perspective, functions such as `beginTransaction()` and `commit()` can also be executed with SQL, except for some drivers.

Savepoint can still be used using SQL, but I'm thinking about adding it as a method for convenience.

> Also, I am not sure that mysqli implements this properly. It’s probably not the best extension to take an example of.

Since there was already an implementation in mysqli, I thought it would be natural to implement it in PDO as well, but based on what you mentioned, I may need to think about this a little more.

Regards.

Saki

Unnamed Person

2 years ago

Hi Saki,

Aside from a few concerns, I think your proposal is very sensible.

I have long felt a disconnect that while we have `beginTransaction()`,
`commit()`, and `rollback()` methods, there's no equivalent
functionality for `SAVEPOINT` in PDO. It seems natural to support
these commonly used transactional features as part of PDO's
functionality.

Having consistent support through PDO would be greatly appreciated
from a user perspective, but I do have concerns regarding the
variations in SQL. The following code is quoted from Laravel:

https://github.com/laravel/framework/blob/v10.43.0/src/Illuminate/Database/Query/Grammars/Grammar.php#L1307-L1310

```php
return 'SAVEPOINT '.$name;
```

While most databases use `SAVEPOINT [name]`, but:

https://github.com/laravel/framework/blob/v10.43.0/src/Illuminate/Database/Query/Grammars/SqlServerGrammar.php#L453-L456

```php
return 'SAVE TRANSACTION '.$name;
```

SQL Server uniquely uses `SAVE TRANSACTION [name]`. There might be
other variations in databases, so full support could be challenging,
which is something to be mindful of.

> change the signature of `rollback()` to `rollback(?string $name = null)`.

This might lead to confusion due to `rollback()` having multiple
functionalities, as `ROLLBACK` and `ROLLBACK TO [name]` target
different rollback operations.

Just as `beginTransaction()` and `savepoint()` are separate, perhaps
having a separate method for rolling back to a savepoint might
maintain consistency, for example `rollbackToSavepoint(string $name)`.

Regards.

Kentaro

Saki Takamachi

2 years ago

Hi Kentaro,

Thank you for your important point.

> While most databases use `SAVEPOINT [name]`, but:
>
> https://github.com/laravel/framework/blob/v10.43.0/src/Illuminate/Database/Query/Grammars/SqlServerGrammar.php#L453-L456
>
> ```php
> return 'SAVE TRANSACTION '.$name;
> ```
>
> SQL Server uniquely uses `SAVE TRANSACTION [name]`. There might be
> other variations in databases, so full support could be challenging,
> which is something to be mindful of.

I think this feature will be implemented for each driver, so it probably won't be a problem for anything other than pdo_odbc. As you say, I think I need to think a little more about pdo_odbc.

(When I looked into odbc3 again, I realized that what I had written was not an API for odbc, sorry. Therefore, it may be difficult to support this feature stably with ODBC.)

I'll see if there's a better way.

>> change the signature of `rollback()` to `rollback(?string $name = null)`.
>
> This might lead to confusion due to `rollback()` having multiple
> functionalities, as `ROLLBACK` and `ROLLBACK TO [name]` target
> different rollback operations.
>
> Just as `beginTransaction()` and `savepoint()` are separate, perhaps
> having a separate method for rolling back to a savepoint might
> maintain consistency, for example `rollbackToSavepoint(string $name)`.

Regarding this, mysqli's `rollback()` is such a specification, so I used that as a reference. However, it may be safer to separate the method itself.

`rollbackTo()` might also be good.

Regards.

Saki

Larry Garfield

2 years ago

On Mon, Feb 5, 2024, at 1:54 PM, Saki Takamachi wrote:

> Hi Kentaro,
>
> Thank you for your important point.
>
>> While most databases use `SAVEPOINT [name]`, but:
>>
>> https://github.com/laravel/framework/blob/v10.43.0/src/Illuminate/Database/Query/Grammars/SqlServerGrammar.php#L453-L456
>>
>> ```php
>> return 'SAVE TRANSACTION '.$name;
>> ```
>>
>> SQL Server uniquely uses `SAVE TRANSACTION [name]`. There might be
>> other variations in databases, so full support could be challenging,
>> which is something to be mindful of.
>
> I think this feature will be implemented for each driver, so it
> probably won't be a problem for anything other than pdo_odbc. As you
> say, I think I need to think a little more about pdo_odbc.
>
> (When I looked into odbc3 again, I realized that what I had written was
> not an API for odbc, sorry. Therefore, it may be difficult to support
> this feature stably with ODBC.)
>
> I'll see if there's a better way.
>
>>> change the signature of `rollback()` to `rollback(?string $name = null)`.
>>
>> This might lead to confusion due to `rollback()` having multiple
>> functionalities, as `ROLLBACK` and `ROLLBACK TO [name]` target
>> different rollback operations.
>>
>> Just as `beginTransaction()` and `savepoint()` are separate, perhaps
>> having a separate method for rolling back to a savepoint might
>> maintain consistency, for example `rollbackToSavepoint(string $name)`.
>
> Regarding this, mysqli's `rollback()` is such a specification, so I
> used that as a reference. However, it may be safer to separate the
> method itself.
>
> `rollbackTo()` might also be good.

I like this proposal.  It's a good incremental improvement to PDO.  I also agree with rollbackTo(), to avoid confusion.

--Larry Garfield

Saki Takamachi

2 years ago

Hi Larry,

> I like this proposal.  It's a good incremental improvement to PDO.  I also agree with rollbackTo(), to avoid confusion.

Thank you, I'm glad to receive your positive feedback.

It is very difficult to implement these in pdo_odbc because the odbc API does not support savepoint.

However, since odbc itself is quite old, it may be a good idea to make only pdo_odbc incompatible with the savepoint method.

Regards.

Saki

Larry Garfield

2 years ago

On Tue, Feb 6, 2024, at 2:28 PM, Saki Takamachi wrote:

> Hi Larry,
>
>> I like this proposal.  It's a good incremental improvement to PDO.  I also agree with rollbackTo(), to avoid confusion.
>
> Thank you, I'm glad to receive your positive feedback.
>
> It is very difficult to implement these in pdo_odbc because the odbc
> API does not support savepoint.
>
> However, since odbc itself is quite old, it may be a good idea to make
> only pdo_odbc incompatible with the savepoint method.

There's been discussion recently about how to deal with incompatibilities between different PDO drivers, especially now that we can have subclasses per driver.  That may be the way to deal with it.  I don't have a major opinion on the approach, other than all incompatibilities should be handled in a consistent way.

--Larry Garfield

Saki Takamachi

2 years ago

> There's been discussion recently about how to deal with incompatibilities between different PDO drivers, especially now that we can have subclasses per driver.  That may be the way to deal with it.  I don't have a major opinion on the approach, other than all incompatibilities should be handled in a consistent way.

I was thinking of an implementation method like `lastInsertId()`, but there is certainly another way to use a subclass.

Personally, I was thinking of implementing, for example, SQL99-compliant (included) functions in PDO Core Class, and vendor-specific original functions in subclasses. However, I don't have a strong opinion on how they should be used, as there is still not much precedent for subclasses.

I agree with you that it should be a consistent implementation. I'm not sure if all incompatible features should be implemented in subclasses, or just vendor-specific features, but at least yes. There should be consistency.

Regards.

Saki