-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Use savepoints to roll back to before SaveChanges for user-managed transactions #20176
Comments
I was able to reproduce this and another issue in a PoC. |
@Lobosque thanks for digging into this and submitting the repro. After some investigation the root cause seems to be an EF Core issue rather than anything Npgsql-related. Our update pipeline does batching, so multiple updates get sent in a single DbCommand - e.g. an INSERT and an UPDATE. If the UPDATE triggers a failure (optimistic concurrency in this case), that failure doesn't prevent other batched statements (e.g. the INSERT) from getting executed. Since the update pipeline doesn't roll back the transaction in case of an exception, any statement that happened to get batched with the problematic statement will get committed. The only reason this is visible on Npgsql and not on Pomelo MySQL (or SQL Server) is that EF Core's default batching size is 4, and since there are only two statements here they do not get batched (so the UPDATE gets sent alone, and the optimistic concurrency exception prevents the INSERT from ever getting sent). Npgsql overrides this and sets the batching size to 2; if you set the minimum batching size to 2 (see sample below), MySQL and SQL Server behave in the same way. @AndriySvyryd @ajcvickers shouldn't we be rolling back the transaction on any exception during update? Simplified repro: class Program
{
static void Main()
{
using (var ctx1 = new BlogContext())
{
ctx1.Database.EnsureDeleted();
ctx1.Database.EnsureCreated();
ctx1.Blogs.Add(new Blog { Name = "InitialName" });
ctx1.SaveChanges();
}
using var ctx = new BlogContext();
using var transaction = ctx.Database.BeginTransaction();
try
{
ctx.Blogs.Add(new Blog { Name = "SomeUnrelatedBlog" });
var blog = ctx.Blogs.Find(1);
blog.Name = "AlteredName";
// Modify value in another connection, to trigger optimistic concurrency exception below
using (var ctx2 = new BlogContext())
{
var blog2 = ctx2.Blogs.Find(1);
blog2.Name = "PreemptedName";
ctx2.SaveChanges();
}
ctx.SaveChanges();
throw new Exception("Should not be here");
}
catch (DbUpdateConcurrencyException)
{
transaction.Commit();
if (ctx.Blogs.Count() != 1)
throw new Exception("Did not roll back");
}
}
}
public class BlogContext : DbContext
{
public DbSet<Blog> Blogs { get; set; }
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
=> optionsBuilder.UseSqlServer(..., o => o.MinBatchSize(2)) // SQL Server defaults to min batch size of 4, which hides this
}
public class Blog
{
public int Id { get; set; }
[ConcurrencyCheck]
public string Name { get; set; }
} |
I agree that rolling it back would be the most pragmatic solution even though it might be a small breaking change. |
@AndriySvyryd that makes sense but for my specific case would also need to be able to deal with savepoints to handle it well. |
@Lobosque you should not require savepoints to deal with this. Assuming we implement the transaction rollback being proposed here, the pattern would be to catch the exception and restart the entire transaction. One note: this issue isn't really specific to batching. Since an optimistic concurrency exception stops the update process, all updates that have been previously updated (not just in the current batch) currently also remain committed; and since updates are reordered by EF Core, it's very hard to tell what got committed and what didn't. I wonder what EF6 did here. |
Note that it's possible to manually use savepoints today to achieve what you want: you can call GetDbTransaction to get the DbTransaction, cast it down to NpgsqlTransaction and use the savepoint API. This allows you to create a savepoint before calling SaveChanges, and roll it back if SaveChanges throws. Note that I've confirmed with @ajcvickers, and the change tracker state is only modified if SaveChanges completes successfully; so if an exception is thrown you can safely retry to call SaveChanges without worrying about the change tracker state. |
Discussion from triage:
|
Note for team. I've been playing with this on SQL Server. First, when EF controls the transaction, then the normal optimistic concurrency pattern works fine. Specifically, the first SaveChanges can fail, this can be fixed, and the whole of SaveChanges can be run again without explicitly doing anything. In other words, at least on SQL Server in this case, the transaction is effectively rolled back anyway. public static class ThreeOneApp
{
public static void Main()
{
using (var context = new SomeDbContext())
{
context.Database.EnsureDeleted();
context.Database.EnsureCreated();
context.AddRange(
new Customer {Name = "Olive"},
new Customer {Name = "Smokey"},
new Customer {Name = "Baxter"},
new Customer {Name = "Alice"},
new Customer {Name = "Toast"},
new Customer {Name = "Mac"});
context.SaveChanges();
}
using (var context = new SomeDbContext())
{
//context.Database.BeginTransaction();
var all = context.Set<Customer>().ToList();
foreach (var customer in all)
{
customer.Name += "+";
}
context.AddRange(
new Bug {Id = 1, Name = "Retrovirus"},
new Bug {Id = 2, Name = "Rotavirus"});
context.Database.ExecuteSqlRaw(@"UPDATE [Customer] SET [Name] = '-------' WHERE [Id] = 3");
try
{
context.SaveChanges();
throw new Exception("Bang!");
}
catch (DbUpdateConcurrencyException e)
{
var entry = context.Entry(context.Find<Customer>(3));
entry.OriginalValues.SetValues(entry.GetDatabaseValues());
}
context.SaveChanges();
//context.Database.CurrentTransaction.Commit();
}
}
}
public class SomeDbContext : DbContext
{
private static readonly ILoggerFactory
Logger = LoggerFactory.Create(x => x.AddConsole()); //.SetMinimumLevel(LogLevel.Debug));
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<Customer>();
modelBuilder.Entity<Bug>();
}
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
=> optionsBuilder
.UseLoggerFactory(Logger)
.EnableSensitiveDataLogging()
.UseSqlServer(Your.SqlServerConnectionString);
}
public class Bug
{
[DatabaseGenerated(DatabaseGeneratedOption.None)]
public int Id { get; set; }
public string Name { get; set; }
}
public class Customer
{
public int Id { get; set; }
public string Name { get; set; }
[Timestamp]
public byte[] RowVersion { get; set; }
} The log output is:
Notice that the explicit key values for the insert are the same each time, but that SQL Server doesn't throw. If I instead comment out the lines to create an explicit transaction, then the retry fails:
|
Notes on Shay's notes:
This is already happening. In my testing, there is nothing that needs to change here.
We're not going to roll back user-initiated transactions to before where SaveChanges started. So we would need savepoints or equivalent to be able to do anything with user-initiated transactions.
We should do this, since it makes the behavior for user-initiated transactions the same as for EF created transactions given a single call to SaveChanges. |
Additional note: we should introduce hooks for managing savepoints in EF Core - this could be important to support cases where the ADO.NET provider hasn't yet implemented support. It would also act as sugar for users to manually manage savepoints more easily, without dropping down to ADO.NET. |
* Add APIs for supporting transaction savepoints. * Support is implemented at the EF level only, no System.Data support yet (this should come soon). * Use savepoints in the update pipeline when a user-managed transaction is used, to roll back to before SaveChanges in case of exception. Part of #20176
* Add APIs for supporting transaction savepoints. * Support is implemented at the EF level only, no System.Data support yet (this should come soon). * Use savepoints in the update pipeline when a user-managed transaction is used, to roll back to before SaveChanges in case of exception. Part of #20176
* Add APIs for supporting transaction savepoints. * Support is implemented at the EF level only, no System.Data support yet (this should come soon). * Use savepoints in the update pipeline when a user-managed transaction is used, to roll back to before SaveChanges in case of exception. Part of #20176
Merged support. Keeping open for further work:
|
In the following code, trying to call
CreateMovementWithTransactionAsync
is not working as expected when trying to get theDatabaseValue
after a failedSaveChanges
attempt.The steps that lead to the problem are described in the comments of the code that follows.
Inside a transaction, we read two rows from the sabe table. the Entity
FinancialAccount
has a[ConcurrencyCheck]
property.We do some manipulation to force
[ConcurrencyCheck]
to throw an exception (all explained in the comments).Since
SaveChanges
threw an exception, we expect theDatabaseValue
to be equalOriginalValue
, and notCurrentValue
.Important: If the transaction is removed, everything works as expected.
Steps to reproduce
Further technical details
EF Core version: 3.1.2
Database provider: Npgsql
Target framework: 3.1.2
Operating system: Windows
IDE: VSCode
The text was updated successfully, but these errors were encountered: