Should savepoints be released after rolling back?

[arsdragonfly]

Bug Description

SQLx SQLite driver uses ROLLBACK TO for rolling back savepoint here. Per SQLx semantics it should also immediately RELEASE the savepoint to remove it from the transaction stack; otherwise savepoint leak would occur. See SQLite docs.

[arsdragonfly]

see rusqlite for how to handle it properly

[abonander]

I started writing a reply refuting this report when I realized there was an important distinction I was missing.

On the surface, it doesn't appear necessary to RELEASE savepoints after rolling back. There's nothing in the linked docs that explicitly suggests that is necessary. On the contrary, see the emphasized part:

The ROLLBACK TO command reverts the state of the database back to what it was just after the corresponding SAVEPOINT. Note that unlike that plain ROLLBACK command (without the TO keyword) the ROLLBACK TO command does not cancel the transaction. Instead of cancelling the transaction, the ROLLBACK TO command restarts the transaction again at the beginning. All intervening SAVEPOINTs are canceled, however.

If we look at the SQLite code that interprets transaction bytecode instructions, we find this block that destroys savepoints in both the RELEASE and the ROLLBACK TO case: https://github.com/sqlite/sqlite/blob/6e53f67c63771b72d5419265038f428c34fa99d5/src/vdbe.c#L3897-L3904

It is clear here that savepoints are stored in a linked list, and this routine is pruning the list from the front (the most recent savepoint) backwards toward the designated savepoint.

However, what it doesn't do is release the actual savepoint that was rolled back to.

Because of SQLx's API design, the savepoint cannot be accessed again and beginning another nested transaction will create a new, redundant savepoint. Even though we reuse savepoint names, this merely shadows the old savepoint. This effectively leaks the savepoint.

If this is the point you were trying to make, you could have gone into a little more detail here and saved me a bunch of digging, although I might have done it anyway as you'll see shortly.

Savepoints cannot be permanently leaked though, because they're destroyed anyway when the parent transaction/savepoint is committed or rolled back, so while this is suboptimal, it's unlikely to cause any real issues unless many, many savepoints are created and then rolled back to in the same transaction.

---

While looking at Postgres to compare what it does, I realized that we likely have the same bug in all the database drivers, because both MySQL and Postgres have the same semantics as SQLite in this case: ROLLBACK TO rolls back to a savepoint but does not release it.

In MySQL, this doesn't strictly result in redundant savepoints because creating a savepoint with the same name as an existing one overwrites it: https://dev.mysql.com/doc/refman/8.4/en/savepoint.html

If the current transaction has a savepoint with the same name, the old savepoint is deleted and a new one is set.

So the savepoint is only leaked until you call .begin() again.

Postgres documents its behavior as an explicit deviation from the standard (see "Compatibility"): https://www.postgresql.org/docs/current/sql-savepoint.html

SQL requires a savepoint to be destroyed automatically when another savepoint with the same name is established. In PostgreSQL, the old savepoint is kept, though only the more recent one will be used when rolling back or releasing. (Releasing the newer savepoint with RELEASE SAVEPOINT will cause the older one to again become accessible to ROLLBACK TO SAVEPOINT and RELEASE SAVEPOINT.) Otherwise, SAVEPOINT is fully SQL conforming.

This sounds identical to what SQLite does.

Digging through the Postgres source code, there's an argument to be made for not releasing savepoints unless necessary, because it requires non-trivial work: https://github.com/postgres/postgres/blob/a43567483c617fb046c805b61964d5168c9a0553/src/backend/access/transam/xact.c#L5096-L5205

This is on top of the work that was already done to roll back to the savepoint: https://github.com/postgres/postgres/blob/a43567483c617fb046c805b61964d5168c9a0553/src/backend/access/transam/xact.c#L5211-L5362

Same as in SQLite, releasing a savepoint is non-trivial, though most of the work is also done by ROLLBACK TO.

---

At the end of the day, I believe most of the use-cases for savepoints involve rolling back to them and then trying identical changes again from that same point. Thus, it doesn't make a whole lot of sense to release a savepoint just to create another.

Instead, I think we should treat an immediate begin() following a rollback() on a savepoint to be a no-op and reuse the same savepoint that already existed. Ultimately, that's less work for the database and is more in-line with how savepoints were intended to be used.

The question becomes, what do we do if the user doesn't call begin() again before executing another statement? If we don't release the savepoint then, it's still effectively leaked. If we do release the savepoint, that's arguably executing a command the user didn't expect.

I think in this case it's fine to release the savepoint at the beginning of the next command, because to the user, the side-effects are almost the same. It may just retain resources for a little longer.

[arsdragonfly]

SQLite doesn't look that problematic tbh (other than the hypothetical situation that we do a ton of savepoints, but that is fine per se IMO) but if PostgreSQL is doing non-trivial stuff when releasing the savepoint, I would interpret that as an indication that not releasing the savepoint might cause weird things beyond our naive awareness to happen (e.g. pg_current_xact_id_if_assigned() might yield unintended value, TX might be stuck in a read-write state (the comment above this LOC was backwards, i.e. parent is RO and child is RW). Those non-trivial work exist for a good reason and I don't think it's our job to cut corners.
Regarding fusing begin() and rollback(), I'd say non-moving rollback_to() would be more ergonomic for both users and driver implementers.