rebus-org / Rebus.SqlServer

:bus: Microsoft SQL Server transport and persistence for Rebus
https://mookid.dk/category/rebus
Other
43 stars 42 forks source link

It should be possible to configure SQL server transaction level #4

Closed mookid8000 closed 7 years ago

mookid8000 commented 7 years ago

The Rebus.SqlServer implementation has a IsolationLevel.ReadCommitted in DbConnectionFactoryProvider that cannot be changed from the outside. In our application we need it to be IsolationLevel.Snapshot to work with the remaining code base. Please make it possible to change it.

mookid8000 commented 7 years ago

@JornWildt originally raised the issue here

mookid8000 commented 7 years ago

@JornWildt how does row-level locks work with snapshot isolation level?

When the SQL transport issues a query to fetch the next message, it does so with the ROWLOCK and READPAST hints in order to avoid having two consumers snatch the same message.

Wouldn't that be broken if snapshot isolation was used?

JornWildt commented 7 years ago

You could be right. Googling a bit (https://msdn.microsoft.com/en-us/library/ms187373.aspx):

ROWLOCK Specifies that row locks are taken when page or table locks are ordinarily taken. When specified in transactions operating at the SNAPSHOT isolation level, row locks are not taken unless ROWLOCK is combined with other table hints that require locks, such as UPDLOCK and HOLDLOCK.

So ROWLOCK would need HOLDLOCK (to keep the lock pending during the transaction).

READPAST can only be specified in transactions operating at the READ COMMITTED or REPEATABLE READ isolation levels. When specified in transactions operating at the SNAPSHOT isolation level, READPAST must be combined with other table hints that require locks, such as UPDLOCK and HOLDLOCK.

So, apparently, a HOLDLOCK would solve it.

JornWildt commented 7 years ago

But, thinking about it, as long as Rebus uses a different connection than the rest of the application and only touches its own tables, then the isolation level does not have to mach that of the rest of the application.

I do although consider asking for a feature that would allow Rebus and the rest of the application to share a connection (basically being able to inject an SqlConnection provider). That would make more efficient use of transactions when both Rebus and the application itself runs on the same database (which would be our case). But I see that doing so could break rebus as it expects a specific transaction isolation level.

Not sure about what is best here :-)

mookid8000 commented 7 years ago

READPAST can only be specified in transactions operating at the READ COMMITTED or REPEATABLE READ isolation levels

READPAST is necessary in order for multiple consumers to read messages in parallel – otherwise, the first consumer would grab the lock on a row, and then the other consumers would be blocked by that lock.

The conclusion must be that SNAPSHOT is not suitable for the transport.

mookid8000 commented 7 years ago

But, thinking about it, as long as Rebus uses a different connection than the rest of the application and only touches its own tables, then the isolation level does not have to mach that of the rest of the application.

Exactly!

mookid8000 commented 7 years ago

I do although consider asking for a feature that would allow Rebus and the rest of the application to share a connection (basically being able to inject an SqlConnection provider). That would make more efficient use of transactions when both Rebus and the application itself runs on the same database (which would be our case). But I see that doing so could break rebus as it expects a specific transaction isolation level.

Not sure about what is best here :-)

All of the different UseSqlServer(...) extensions have overloads that accept a Func<Task<IDbConnection>> allowing for providing a shared connection.

mookid8000 commented 7 years ago

There's a problem though if you reply on Rebus to create the necessary database schema for you – if the connection you provide uses SNAPSHOT isolation, the schema generation will fail, as SQL Server does not allow you to e.g. CREATE TABLE on a connection using SNAPSHOT

JornWildt commented 7 years ago

You kind of ignored the rest of the quote:

READPAST ... When specified in transactions operating at the SNAPSHOT isolation level, READPAST must be combined with other table hints that require locks, such as UPDLOCK and HOLDLOCK.

So I would expect READPAST to work with snapshot isolation (as intented in Rebus - ignoring locked records) when READPAST is used with HOLDLOCK (making sure locks are actually detected and skipped).

Anyway - its not the most important issue for me. Leaving it "as it" works too.

mookid8000 commented 7 years ago

You kind of ignored the rest of the quote:

Ah sorry 😁 I was too fast there!

mookid8000 commented 7 years ago

Back to your original conclusion:

So, apparently, a HOLDLOCK would solve it.

Apparently, yes. I will try that immediately and see if it affects the transport in any negative way – I mean, if it doesn't make a difference under normal READ COMMITTED operation, then we might as well allow for the transport to work with SNAPSHOT isolation too.

mookid8000 commented 7 years ago

Weird!! 🤔

When I add the HOLDLOCK hint, I get this error:

System.Data.SqlClient.SqlException (0x80131904): You can
only specify the READPAST lock in the READ COMMITTED or
REPEATABLE READ isolation levels.

even though I am still using READ COMMITTED....!

mookid8000 commented 7 years ago

If I change the transport's connection to use SNAPSHOT isolation I get the same error:

System.Data.SqlClient.SqlException (0x80131904): You can
only specify the READPAST lock in the READ COMMITTED or
REPEATABLE READ isolation levels.
mookid8000 commented 7 years ago

hmm, I forgot about this one 😄 how's it going?

JornWildt commented 7 years ago

Not an issue since Rebus uses its own SQL connection and does not interact with any business data (and vice versa). As long as these are kept separate there is no problem.

mookid8000 commented 7 years ago

👍