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

Long running operations causes SQL timeout #6688

Merged
merged 6 commits into from
Oct 21, 2019

Conversation

Shazwazza
Copy link
Contributor

Moves read/write locking and isolation level to the responsibility of the sql provider based on the db type and changes Sql server to use ReadCommitted for normal SQL calls and RepeatableRead for when working with distributed locks.

This is a trimmed down version of this PR #6029

Changes

  • Adds methods to ISqlSyntaxProvider for dealing with read/write locks and for returning the provider's own default isolation mode
  • Changes SQL Server default isolation mode to ReadCommitted and then ensures we only use RepeatableRead when creating/reading the distributed locks using the ReadLock/WriteLock methods. This means that long running operations/transactions are not going to block all reads from the database which have resulted in timeout errors
  • No changes to SQLCE - this will continue to use RepeatableRead since it does not support isolation mode per query

Testing

  • Ensures all tests pass
  • Code review
  • Test Umbraco with SQLCE and SQL Server - only needs some simple tests, installing with starter kit, creating content types and content should be all that is needed

foreach (var lockId in lockIds)
{
db.Execute(@"SET LOCK_TIMEOUT 1800;");
var i = db.Execute(@"UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = value*-1 WHERE id=@id", new { id = lockId });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if by accident the value has been set to zero this does not lock - hence the original CASE

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zpqrtbnk, is there ways this can happen other than people manipulating the database them self? I wonder why we handle this special case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it was "assume garbage in" but not critical - you decide

@nul800sebastiaan
Copy link
Member

Will this help for #6735 as well?

@bergmania
Copy link
Member

@nul800sebastiaan probably not.. This fixes unintended locking.. Slow queries are still slow.

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

Successfully merging this pull request may close these issues.

5 participants