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

[9.x] Use readpast query hint instead of holdlock for sqlsrv database queue #43259

Merged
merged 1 commit into from
Jul 19, 2022
Merged

[9.x] Use readpast query hint instead of holdlock for sqlsrv database queue #43259

merged 1 commit into from
Jul 19, 2022

Conversation

Namoshek
Copy link
Contributor

@Namoshek Namoshek commented Jul 18, 2022

Back in the days, the database queue driver was recommended only for development due to performance and deadlock issues. Quite some time ago, somewhen during the life of Laravel 6, performance improvements were introduced for the database queue driver in combination with MySQL >= 8.0.1 and PostgreSQL >= 9.5. This improvement was later extended to cover MariaDB >= 10.6.0 as well. But until this day, there is no special handling for the sqlsrv database driver and deadlocks are a frequent issue, even though SQL Server supports a similar feature. This PR therefore adds the special lock flag readpast for the sqlsrv driver.

The speed improvement of this change is quite significant, especially when running a lot of workers.

Old behavior:
Currently, the database queue driver with sqlsrv as database driver is using the default update lock flags with(rowlock,updlock,holdlock), which are defined in the SqlServerGrammar.

The default update lock flags are used because of return true in the DatabaseQueue::getLockForPopping() method which are passed to Builder::lock($bool) in DatabaseQueue::getNextAvailableJob().

New behavior:
With this change, we now pass with(rowlock,updlock,readpast) instead of with(rowlock,updlock,holdlock) when retrieving the next job to process.

The meaning of READPAST according to the official documentation is very similar to the other DBMS:

Specifies that the Database Engine not read rows that are locked by other transactions. When READPAST is specified, row-level locks are skipped but page-level locks are not skipped. That is, the Database Engine skips past the rows instead of blocking the current transaction until the locks are released. For example, assume table T1 contains a single integer column with the values of 1, 2, 3, 4, 5. If transaction A changes the value of 3 to 8 but has not yet committed, a SELECT * FROM T1 (READPAST) yields values 1, 2, 4, 5. READPAST is primarily used to reduce locking contention when implementing a work queue that uses a SQL Server table. A queue reader that uses READPAST skips past queue entries locked by other transactions to the next available queue entry, without having to wait until the other transactions release their locks.

The READPAST flag cannot be combined with HOLDLOCK.

Test coverage:
This PR does not include any tests since concurrency is extremely hard to test and there do not seem to be any SQL Server tests for the queue system so far. The locking behavior of SQL Server can be tested manually with the following two snippets which are to be executed in this order concurrently:

BEGIN TRANSACTION;
SELECT TOP(1) * FROM jobs WITH(ROWLOCK,UPDLOCK,HOLDLOCK); -- or READPAST instead of HOLDLOCK
WAITFOR DELAY '00:00:10';
COMMIT;
-- Test 1: HOLDLOCK causes the query to block and wait for the open transaction
SELECT TOP(1) * FROM jobs WITH(ROWLOCK,UPDLOCK,HOLDLOCK);

-- Test 2: READPAST causes the query to return the first not-blocked row
SELECT TOP(1) * FROM jobs WITH(ROWLOCK,UPDLOCK,READPAST);

@taylorotwell taylorotwell merged commit 80fe7bd into laravel:9.x Jul 19, 2022
@Namoshek Namoshek deleted the feature/database-queue-concurrency-sqlsrv branch July 19, 2022 14:30
@GrahamCampbell GrahamCampbell changed the title Use readpast query hint instead of holdlock for sqlsrv database queue [9.x] Use readpast query hint instead of holdlock for sqlsrv database queue Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants