SaveChanges() deadlocks in overlapping scenarios

[divega]

Steps to reproduce

The original repro steps were provided by @nathana1 and apply directly to PostgreSQL. Later it was confirmed that the issue affects SQL Server when transaction are being used (the default behavior for SaveChanges()) as well and should be reproducible on SQL Server with a subset of these steps:

1. Git clone https://github.com/aspnet/benchmarks.git
2. Run build.ps1 to setup everything up
3. Setup a client as directed here: https://github.com/aspnet/benchmarks#generating-load
4. Modify https://github.com/aspnet/benchmarks/blob/dev/src/Benchmarks/appsettings.json with connection string to your db and add this field: "Database": "postgresql"
5. Modify either https://github.com/aspnet/benchmarks/blob/dev/src/Benchmarks/Data/ApplicationDbContext.cs#L28 to enable batching or https://github.com/aspnet/benchmarks/blob/dev/src/Benchmarks/Data/RawDb.cs#L106 to disable sorting (depending on if you want to look at raw or ef)
6. Make sure the connection string includes Maximum Pool Size=1024 and that there can be enough connections, e.g. posgresql.conf can contain max_connections = 2000.
7. On the web server run: dotnet run urls=http://*:5000 scenarios=update
   • The first time it runs it will populate the database so it might take a little while
8. On the client machine run: wrk -c 512 -d 10 -t 16 http://10.0.1.20:5000/updates/raw?queries=20
   • Substitute ef for raw in url to switch

The issue

The test shows initially a spike of throughput but then quickly the throughput goes down near zero, with most of the requests failing. Closer examination shows that the reason for the failure is deadlocks.

As we understand them is that the test will often generate two or more requests that try to make changes to two or more of the same rows, but the changes are performed in arbitrary order therefore the deadlocks.

Previously in 1.1 we added a feature to EF to allow disabling transactions during SaveChanges() (see #6339) mainly because we observed that this helped with these tests perform much better. However the reason that helped was that each individual statement in the batches generated by EF Core got their own transactions, meaning that locks did not remain held for the whole batch. In this case ordering did not matter.

Proposed solution

Through the raw tests we have found that sorting the SQL statements (https://github.com/aspnet/benchmarks/blob/dev/src/Benchmarks/Data/RawDb.cs#L106) based on primary key values can help achieve a similar performance boost.

The proposal is to extend the topological sort performed by EF Core over individual operations to include an order over the primary key values. Otherwise use this order in MERGE operations for SQL Server.

Once we do this we should consider pulling the feature that allows opting out of transactions in SaveChanges() (see #6339), as there aren't any clear benefits and it adds some complexity to the API.

[divega]

cc @roji

[roji]

FYI for the PostgreSQL angle, the current Npgsql batching implementation doesn't support non-transactional batches; I've opened npgsql/npgsql#1307 to fix this. However, topological ordering is necessary regardless, with PostgreSQL or other providers, for when users choose transactional batches.

[divega]

Assigning this to @AndriySvyryd for 1.1.0-preview1 after talking to @rowanmiller.

[divega]

We need to test a build containing this fix with the ASP.NET benchmarks. @AndriySvyryd can you please work with @nathana1 to get this done?

Then, depending on the outcome of the esting, we should consider removing the ability to opt-out of transactions in SaveChanges() before we ship 1.1.

[AndriySvyryd]

@divega Yes, but Coherence is broken, so it might be a couple of days till we get a build in the feed.