Implement savepoint API for Microsoft.Data.Sqlite #22273
Conversation
| @@ -113,6 +113,74 @@ public override void Rollback() | |||
| RollbackInternal(); | |||
| } | |||
|
|
|||
| #if NET5_0 | |||
There was a problem hiding this comment.
I think we should expose the same API on all targets
|
A couple of concerns:
|
| throw new InvalidOperationException(Resources.TransactionCompleted); | ||
| } | ||
|
|
||
| _connection.ExecuteNonQuery("SAVEPOINT " + savepointName); |
There was a problem hiding this comment.
Delimit and escape the identifier. Terminate statements.
SAVEPOINT "The ""SaveChanges"" Savepoint";| throw new InvalidOperationException(Resources.TransactionCompleted); | ||
| } | ||
|
|
||
| _connection.ExecuteNonQuery("ROLLBACK TO " + savepointName); |
There was a problem hiding this comment.
ROLLBACK TO SAVEPOINT (the SQL standard syntax)
| throw new ArgumentNullException(nameof(savepointName)); | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(savepointName)) |
|
Yeah, this does mean that until rc1 is released, users of our daily builds would have to also have SDK daily builds. I can also submit this after rc1 if we're OK with doing it even later (and think we'll get approval). I'm not sure what the risks are of multi-targeting, it's always been straightforward for me. @ajcvickers @bricelam if you're against this, let me know and I'll close. |
|
What happens if we add virtual methods to netstandard 2.0 only? Will it explode on 5.0.0-rc.1? |
|
I feel like adding complications for the community and for daily builds is not wise at this stage. I also don't think this meets the bar to come in after rc1. I think this should just be punted to 6.0 at this point. |
|
And if we add the methods to netstandard2.0, we can still light it up in the EF Core provider, right? |
|
In EF we already support savepoints without going through any System.Data API (RelationalTransaction implements everything). Not sure what happens if we add virtual methods to netstandard2.0, but I'm pretty sure it wouldn't work properly - it's like having the same virtual method signature in a superclass and base class. I can't see any way where polymorphically calling the base virtual method would end up calling the subclass implementation... I guess we can discuss all this for 6.0. |
|
So it would hide the ADO.NET versions, and you'd have to cast to SqliteTransaction to call them? I can live with that. Aren't there already providers in the wild that expose these methods and are in the same boat until they can multi-target net5.0? |
|
I vote for putting these in now and multi-targeting in 6.0. Thoughts @ajcvickers? |
|
I think there's much less value in doing this if we're not implementing the System.Data methods (i.e. multi-targeting). If you're using SqliteTransaction, you're already coding directly against MS.Data.Sqlite, so it's very easy to just execute the commands yourself. Also, this situation with a netstandard2.0 virtual method with the same signature as a net5.0 one doesn't seem totally kosher... |
|
This only adds value at the ADO.NET level, right? EF Core should show no change in behavior. I'm really struggling with any risk right now, since it seems to add such minimal value. |
|
Yeah, given that the EF Core provider supports savepoints without it, I agree we can just punt it. |
|
The world isn't just about EF 😉 The point here was to start actually delivering the new System.Data APIs we're introducing. At the end of the day the commit went into runtime on August 5th, and we can't deliver our own Sqlite provider with support for it in .NET 5.0 (to be released in a while still...). That makes me sad regardless of whether EF uses it or not. |
|
Agreed. It's insane that Preview 8 was an entire month stale by the time it was released. |
|
@roji Agreed, it makes me sad too. However, what we learn from this is that we really need to front-load System.Data work. Like, even more! Also, we should mark the API reviews in dotnet/runtime as |
|
So... I updated it to not depend on the latest SDK. How do we feel about the change now? |
Curious: How does the SDK used to build EF bleed into the daily builds❔ If this is about requiring a repo-local version of the SDK, that requirement exists regardless because global.json lists runtimes that must be added.
Curious: What problems in the RC1 SDK led you to need the older version❔ I see the above discussion but don't get it. |
|
I still think it's more sensible to do this in 6.0. @dotnet/efteam Do others have any thoughts on this? |
|
No user has asked for this, so no-one (except us) would be disappointed if we punt this. |
|
I know it sucks, but I think we should punt this. Sorry. |
|
Replaced by #22869 |
Closes #20228