Skip to content

[Messenger] Improve deadlock handling on ack() and reject() #54105

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 5, 2024
Merged

[Messenger] Improve deadlock handling on ack() and reject() #54105

merged 1 commit into from
Apr 5, 2024

Conversation

jwage
Copy link
Contributor

@jwage jwage commented Feb 28, 2024 •
edited
Loading

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #54103
License MIT

We started getting this deadlock recently. It has happened only twice so far under high load.

SQLSTATE[40P01]: Deadlock detected: 7 ERROR:  deadlock detected
DETAIL:  Process 221664 waits for ShareLock on transaction 59539641; blocked by process 221671.
Process 221671 waits for AccessExclusiveLock on tuple (77,27) of relation 16455 of database 16385; blocked by process 221605.
Process 221605 waits for ShareLock on transaction 59539646; blocked by process 221606.
Process 221606 waits for AccessExclusiveLock on tuple (69,16) of relation 16455 of database 16385; blocked by process 221664.
HINT:  See server log for query details.
CONTEXT:  while deleting tuple (69,16) in relation "messenger_messages"
Process 221664 waits for ShareLock on transaction 59539641;
blocked by process 221671.

Here are the queries for each process:

Process 221664 waits for ShareLock on transaction 59539641;
blocked by process 221671.

221671
SELECT m.* FROM messenger_messages m WHERE (m.queue_name = $1) AND (m.delivered_at is null OR m.delivered_at < $2) AND (m.available_at <= $3) ORDER BY available_at ASC LIMIT 1 FOR UPDATE

221605
SELECT m.* FROM messenger_messages m WHERE (m.queue_name = $1) AND (m.delivered_at is null OR m.delivered_at < $2) AND (m.available_at <= $3) ORDER BY available_at ASC LIMIT 1 FOR UPDATE

221606
SELECT m.* FROM messenger_messages m WHERE (m.queue_name = $1) AND (m.delivered_at is null OR m.delivered_at < $2) AND (m.available_at <= $3) ORDER BY available_at ASC LIMIT 1 FOR UPDATE

221664
DELETE FROM messenger_messages WHERE id = $1

Open for discussion if this is the right way to handle this or not.

TODO:

@carsonbot carsonbot added this to the 6.4 milestone Feb 28, 2024
@OskarStark OskarStark changed the title [Doctrine Messenger] Improve deadlock handling on ack() and reject(). [Messenger][Doctrine] Improve deadlock handling on ack() and reject() Feb 29, 2024
@jwage jwage requested a review from stof March 4, 2024 17:41
@carsonbot carsonbot changed the title [Messenger][Doctrine] Improve deadlock handling on ack() and reject() Improve deadlock handling on ack() and reject() Mar 4, 2024
@carsonbot carsonbot changed the title Improve deadlock handling on ack() and reject() [Messenger] Improve deadlock handling on ack() and reject() Mar 5, 2024
@jwage
Copy link
Contributor Author

jwage commented Mar 5, 2024

After discussing this with Nicolas, I am going to remove the skip_locked option. We will always use FOR UPDATE SKIP LOCKED. In addition to that, I will make a change to add a delay to the retries and a jitter since we are getting deadlocks caused by contention, immediately retrying with no delay may cause more contention and cause us to repeatedly get deadlocks.

bendavies
bendavies reviewed Mar 6, 2024
View reviewed changes
src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Connection.php
@@ -21,6 +21,7 @@
use Doctrine\DBAL\Platforms\MySQLPlatform;
use Doctrine\DBAL\Platforms\OraclePlatform;
use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
use Doctrine\DBAL\Query\ForUpdate\ConflictResolutionMode;
Copy link
Contributor

Choose a reason for hiding this comment

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

as far as i can see, this was added in dbal 3.8, but we support versions prior to that
https://github.com/symfony/doctrine-messenger/blob/6.4/composer.json#L20C10-L20C23

Copy link
Contributor

@bendavies bendavies Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

may have to manually construct the sql, as is done below with the method_exists(QueryBuilder::class, 'forUpdate') checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will see. The sql for each platform is not as simple as just appending SKIP LOCKED. And in fact now that you point it out, I suspect this code might be broken for other platforms when you are using older versions of doctrine/dbal where the forUpdate() abstraction does not exist yet.

@fabpot
Copy link
Member

fabpot commented Apr 5, 2024

Thank you @jwage.

@fabpot fabpot merged commit 3b9f6b3 into symfony:6.4 Apr 5, 2024
6 of 9 checks passed
This was referenced Apr 29, 2024
Merged
Merged
@jwage jwage deleted the fix-doctrine-messenger-deadlock branch October 13, 2024 21:08
@psihius psihius mentioned this pull request Oct 22, 2024
Open
@renovate renovate bot mentioned this pull request Jan 2, 2025
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alcohol alcohol alcohol left review comments

@sebastianbergmann sebastianbergmann sebastianbergmann left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@jdreesen jdreesen jdreesen left review comments

@bendavies bendavies bendavies left review comments

@fabpot fabpot fabpot approved these changes

@dunglas dunglas dunglas approved these changes

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Bug Messenger Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.