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

[jwage]

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:

• Should there be a retry delay/exponential backoff/jitter? Retrying the failed delete that deadlocked immediately may not help.
• Should skip_locked even be an option or should we always use skip locked?
• Should we add SKIP LOCKED to the FOR UPDATE? It will reduce contention further. I was looking at how SolidQueue in Ruby On Rails handles this and it appears they use SKIP LOCKED FOR UPDATE https://github.com/basecamp/solid_queue/blob/fe57349a126efc381fe0adf4c1ec444bd8a4f53f/app/models/solid_queue/record.rb#L11

[jwage]

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]

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

[bendavies]

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

[jwage]

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]

Thank you @jwage.