Skip to content
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

BatchSaveChanges should support operation.AllowConcurrency = false #585

Closed
jzabroski opened this issue May 28, 2024 · 6 comments
Closed
Assignees

Comments

@jzabroski
Copy link

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

  • EF version: [EF Classic 6 v6.4.4]
  • EF Extensions version: [EFE Core v7.18.4]
  • Database Server version: [SQL Server 2019]
  • Database Provider version (NuGet): [latest .NET 4.8 System.Data dependency]
@JonathanMagnan JonathanMagnan self-assigned this May 29, 2024
@JonathanMagnan
Copy link
Member

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
Copy link
Member

Hello @jzabroski,

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

Let me know if you need further assistance.

Best regards,

Jon

@jzabroski
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

@jzabroski
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants