zzzprojects / EntityFramework-Extensions

Entity Framework Bulk Operations | Improve Entity Framework performance with Bulk SaveChanges, Insert, update, delete and merge for SQL Server, SQL Azure, SQL Compact, MySQL and SQLite.
https://entityframework-extensions.net
338 stars 56 forks source link

BatchSaveChanges should support operation.AllowConcurrency = false #585

Closed jzabroski closed 1 day ago

jzabroski commented 1 month ago

Description

When saving data to tables with audit logs (temporal tables), using BatchSaveChanges, ZZZ Projects uses ValidFrom/ValidTo/Version columns to determine optimistic concurrency behavior. In practice, since the table is audit logged, any over-written changes would be easy to see in the audit log. Moreover, while BulkSaveChanges does allow this, calling BulkSave generates a MERGE statement that can fail when there is a non-clustered index on the temporal table (this failure is a long-standing issue with SQL Server dating back to SQL Server 2008 pre-R2 that has had various fixes, and currently doesnt work for temporal tables in practice, as almost any useful temporal table will have a non-clustered index on the main table's primary key for navigation purposes). Hence, it would be ideal to have feature parity with BulkSaveChanges operation.AllowConcurrency = false

Currently, I can't write code like this:

try
{
    // DbContext.Entry calls detect changes which becomes when the change tracker is tracking a large number of entities.
    // To avoid this performance issue we will:
    //   * Disable change tracking
    //   * Call detect changes to ensure everything is up to date
    //   * Do our state changes
    //   * Turn back on change tracking
    // We know can do this safely as EF won't modify the state of entity (see Rule 1): 
    // https://blog.oneunicorn.com/2012/03/12/secrets-of-detectchanges-part-3-switching-off-automatic-detectchanges/

    Context.Configuration.AutoDetectChangesEnabled = false;
    Context.ChangeTracker.DetectChanges();
    foreach (var entity in entities)
    {
        var entry = Context.Entry(entity);
        if (entry.State == EntityState.Detached)
        {
            entry.State = entity.IsTransient() ? EntityState.Added : EntityState.Modified;
        }
    }
}
finally
{
    Context.Configuration.AutoDetectChangesEnabled = true;
}
Context.UnderlyingContext.BulkSaveChanges(operation =>
{
    operation.AllowConcurrency = false;
});

The above example will hit System.Data.SqlClient.SqlException: Attempting to set a non-NULL-able column's value to NULL.

I want to write code like this:

try
{
    // DbContext.Entry calls detect changes which becomes when the change tracker is tracking a large number of entities.
    // To avoid this performance issue we will:
    //   * Disable change tracking
    //   * Call detect changes to ensure everything is up to date
    //   * Do our state changes
    //   * Turn back on change tracking
    // We know can do this safely as EF won't modify the state of entity (see Rule 1): 
    // https://blog.oneunicorn.com/2012/03/12/secrets-of-detectchanges-part-3-switching-off-automatic-detectchanges/

    Context.Configuration.AutoDetectChangesEnabled = false;
    Context.ChangeTracker.DetectChanges();
    foreach (var entity in entities)
    {
        var entry = Context.Entry(entity);
        if (entry.State == EntityState.Detached)
        {
            entry.State = entity.IsTransient() ? EntityState.Added : EntityState.Modified;
        }
    }
}
finally
{
    Context.Configuration.AutoDetectChangesEnabled = true;
}
Context.UnderlyingContext.BatchSaveChanges(operation =>
{
    operation.AllowConcurrency = false;
});

Exception

System.Data.Entity.Infrastructure.DbUpdateConcurrencyException: Store update, insert, or delete statement affected an unexpected number of rows (0). Entities may have been modified or deleted since entities were loaded. See http://go.microsoft.com/fwlink/?LinkId=472540 for information on understanding and handling optimistic concurrency exceptions. ---> System.Data.Entity.Core.OptimisticConcurrencyException: Store update, insert, or delete statement affected an unexpected number of rows (0). Entities may have been modified or deleted since entities were loaded. See http://go.microsoft.com/fwlink/?LinkId=472540 for information on understanding and handling optimistic concurrency exceptions. at System.Data.Entity.Core.Mapping.Update.Internal.UpdateTranslator.ValidateRowsAffected(Int64 rowsAffected, UpdateCommand source) at Z.EntityFramework.Extensions.PolyCommandSet.Execute() at ?.?(Action1 ?) --- End of inner exception stack trace --- at ?.?(Action1 ?)

Further technical details

JonathanMagnan commented 1 month ago

Hello @jzabroski ,

After reviewing our code, it could be possible to skip the concurrency check. This means that in the case of a concurrency scenario, the entity will not be updated/deleted as of now but will not throw an error either.

However, we don't want to force the Delete or Update as the BatchSaveChanges currently don't touch the existing SQL generated by EF6; It simply batches all SQL in fewer commands.

If this solution is okay with you, I will ask my developer to look into it.

Best Regards,

Jon

JonathanMagnan commented 1 month ago

Hello @jzabroski,

Since our last conversation, we haven't heard from you.

Let me know if you need further assistance.

Best regards,

Jon

jzabroski commented 1 month ago

Hey,

I would like this but I realized BulkSaveChanges works for me for now as I don't need additional indexes on this table. I plan to test BulkSaveChanges this week and revert back once I'm sure this works the way I want.

JonathanMagnan commented 2 weeks ago

Hello @jzabroski ,

Just to make sure, can we close this issue as you currently seem to not need the solution anymore from what I understand in your last message?

Best Regards,

Jon

jzabroski commented 2 weeks ago

Ill confirm this week. Please keep open one more week.

jzabroski commented 2 days ago

@JonathanMagnan I do not currently need this feature. BulkSaveChanges is enough for now. Feel free to close.