Expose transaction savepoints in ADO.NET

[roji]

The SQL Standard defines the concept of savepoints, which allow one to partially roll back a transaction; multiple savepoints may be defined, but only within a transaction. These seem to be well-supported in a uniform way across databases - although the SQL syntax is a bit quirky across databases. SqlClient and Npgsql already expose methods for savepoints on their DbTransaction implementations, but there's probably value in exposing these on DbTransaction.

• PostgreSQL. Exposed by Npgsql: Save, Rollback, Release and async counterparts.
• SQL Server. Exposed by SqlClient: Save, Rollback, no Release (apparently not supported by SQL Server), no async methods.
• Sqlite. Supports SAVE/ROLLBACK/RELEASE, but not exposed in Sqlite. Tracking issue: Sqlite: add transaction savepoint API efcore#20228
• MySQL. Supports SAVE/ROLLBACK/RELEASE, but not exposed in MySqlConnector. Tracking issue: Add transaction savepoint API mysql-net/MySqlConnector#775

API Proposal

class DbTransaction
{
    public virtual Task SaveAsync(string savepointName, CancellationToken cancellationToken = default)
        => throw new NotSupportedException();
    public virtual Task RollbackAsync(string savepointName, CancellationToken cancellationToken = default)
        => throw new NotSupportedException();
    public virtual Task ReleaseAsync(string savepointName, CancellationToken cancellationToken = default)
        => Task.CompletedTask;

    public virtual void Save(string savepointName) => throw new NotSupportedException();
    public virtual void Rollback(string savepointName) => throw new NotSupportedException();
    public virtual void Release(string savepointName) {}

    public virtual bool AreSavepointsSupported => false; // Alternate naming: SupportsSavepoints
}

Notes

• RELEASE seems supported everywhere except for SQL Server. However, it has no user-visible behavior - it only frees up database resources associated with the savepoint - so it seems correct to have this be a no-op by default (and on SQL Server), as opposed to the other methods which throw if not supported.
• Release should generally not throw - if the transaction has already completed or the connection has been broken, it should silently return (similar to DbTransaction.Dispose).
• Instead of the above, we could have a CreateSavepoint that returns a disposable DbSavepoint, which has a single Rollback method on it (Dispose releases). Comments:
  • Pro: Better naming (the current Save/Rollback/Release don't even have Savepoint in the method name).
  • Pro: It would "codify" the fact that releasing doesn't throw (since Dispose in general doesn't).
  • Pro: Aligns more cleanly with the transaction API (e.g. DbConnection.BeginTransaction)
  • Con: New API where Save/Rollback/Release already exists in SqlClient and Sqlite.
  • Con: New type (DbSavepoint, conceptual complexity) which is unlikely to add provider-specific functionality.
  • Con: Additional allocation
• All these methods need to do a database roundtrip (i.e. synchronous completion not really possible, except for Sqlite), but they aren't very fine-grained operations, so making them return ValueTask seems unnecessary. For now I'm proposing they return Task (like CommitAsync and RollbackAsync), but guidance may have changed here (/cc @stephentoub)
• At least in theory, savepoint support may vary across database versions/editions, so ideally the AreSavepointsSupported should throw InvalidOperationException if accessed with closed connection. We could introduce a non-virtual user-facing property to check for this, calling a protected virtual property implemented by the provider, but we can't know if a transaction has completed (or has been disposed). So it's probably better to leave this to the provider, documenting the correct behavior (even when the specific provider always supports savepoints).
• To support this, ADO providers would need to cross-target to .NET 5. /cc @David-Engel @cheenamalhotra @bgrainger @bricelam

Edit history

Date	Modification
18/3	API proposal
24/3	Renamed parameter name from savePointName to savepointName
26/3	Made AreSavepointsSupported virtual
21/7	Added alternate naming Supports Savepoint, describe alternative API (CreateSavepoint instead of Save)

[roji]

/cc @David-Engel @cheenamalhotra

[roji]

Added API proposal with notes

/cc @David-Engel @cheenamalhotra @bgrainger @bricelam @ajcvickers @stephentoub @terrajobst @mgravell

[bgrainger]

RELEASE ... has no user-visible behavior ... so it seems correct to have this be a no-op by default

There can be user-visible side-effects (e.g., raising an error if that savepoint name doesn't exist, making it an error in the future to roll back to that savepoint name after it's released).

My inclination is to make it throw NotSupportedException by default (since it won't be supported by older providers running on top of the new base class) and allow providers to override it as a noop if they choose.

[bgrainger]

Since DB terminology universally uses "savepoint" (not "save point"; see docs linked from OP), the parameter names should be savepointName. That's also consistent with AreSavepointsSupported.

[roji]

Sorry for the delayed response @bgrainger, these are busy times.

There can be user-visible side-effects (e.g., raising an error if that savepoint name doesn't exist, making it an error in the future to roll back to that savepoint name after it's released).

Right - release can be visible for code depending on an error being thrown when the API is used incorrectly etc. I don't have very strong feelings here, but I don't think these negative cases typically inform API decisions in a significant way.

My inclination is to make it throw NotSupportedException by default [...]

To be concrete, all databases I know of which support savepoints support Release, except SQL Server where this would be a no-op; so we're mainly discussing whether SqlClient would inherit the no-op behavior or if it would have to override for that behavior. This would have very little actual consequences either way.

[...] (since it won't be supported by older providers running on top of the new base class) and allow providers to override it as a noop if they choose.

Well, anyone using the new savepoint API on older providers is very likely to call Save/Rollback before Release, at which point they'd get the NotSupportedException anyway.

Since DB terminology universally uses "savepoint" (not "save point"; see docs linked from OP), the parameter names should be savepointName. That's also consistent with AreSavepointsSupported.

Sounds good. The naming was taken from SqlClient's current naming, but the new DbTransaction methods can have different parameter names (though this may get slightly confusing for people using named parameters). FWIW Npgsql already uses a different parameter name here (name). Amended the proposal.

[bgrainger]

This would have very little actual consequences either way.

Agreed, and I'm happy with the current proposal.

Amended the proposal.

Thanks.

One final thing: shouldn't this be virtual?

public bool AreSavepointsSupported => false;

[roji]

One final thing: shouldn't this be virtual?

Absolutely, thanks for catching it (amended).

I think this is ready for review - though am wondering if return a Task rather than ValueTask is still the right thing here (/cc @stephentoub). Am marking this as ready for review.

[roji]

PR submitted: #34561. This is pretty much a simple copy-paste of the proposal above, but @ajcvickers @bgrainger always good to have another pair eyes to make sure things are good.

am wondering if return a Task rather than ValueTask is still the right thing here

After discussion with @stephentoub, leaving these methods to return a Task as the allocation isn't likely to matter for methods which are almost certain to do a database network roundtrip, and which don't return a value.

@stephentoub @terrajobst I'm really hoping we can get this in for preview4 as EF Core needs to build some functionality over it.

[roji]

@FransBouma just realized I had forgotten to cc you on this, in case you're interested.

[FransBouma]

@roji Looks good to me :) Yeah 'Release' isn't really that interesting, as in general you want to go back to a given point in time in the transaction. It's a niche feature tho. I support it through my ORM API, and use reflection to obtain the Save method on the ADO.NET provider so having a generic interface for this is great, saves me the trouble for reflecting over it :)

[roji]

I support it through my ORM API, and use reflection to obtain the Save method on the ADO.NET provider

Great, that's the perfect example for why this is needed in System.Data :)

[terrajobst]

@ajcvickers @roji let me know if you still want to do this for .NET 5 and I'll schedule a review.

[roji]

@terrajobst yes, if possible I'd still like to get this in for 5...

[terrajobst]

Video

• Looks good, but we'd prefer SupportsSavepoints
• We considered exposing a new type DbSavepoint but we felt it's overkill for a niche feature like this
• The System.SqlClient and Microsoft.SqlClient APIs should be updated to override these new base methods (instead of just hiding them) as well as SupportsSavepoints.

namespace System.Data.Common
{
    public class DbTransaction
    {
        public virtual Task SaveAsync(string savepointName, CancellationToken cancellationToken = default);
        public virtual Task RollbackAsync(string savepointName, CancellationToken cancellationToken = default);
        public virtual Task ReleaseAsync(string savepointName, CancellationToken cancellationToken = default);

        public virtual void Save(string savepointName);
        public virtual void Rollback(string savepointName);
        public virtual void Release(string savepointName);

        public virtual bool SupportsSavepoints { get; }
    }
}

[roji]

PR: #34561