Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Messenger] Reduce lock time when using MySQL for transport#60207
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
[Messenger] Reduce lock time when using MySQL for transport#60207
Uh oh!
There was an error while loading.Please reload this page.
Conversation
introduce a algorithm in `Connection` for mysql platforms to minimize exclusive locking
carsonbot commentedApr 13, 2025
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
| try { | ||
| $this->driverConnection->delete($this->configuration['table_name'], ['delivered_at' => '9999-12-31 23:59:59']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this made an exclusive lock on more than a row or record level. this is problematic since it blocks all other processes
| private function getMessageForMySQLPlatform(): ?array | ||
| { | ||
| $possibleIdsToClaim = $this->createAvailableMessagesQueryBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fetch only ids till a message is claimed to not load all the payloads unnecessary (they can be huge)
| return null; | ||
| } | ||
| $messageData = $this->createQueryBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
load the data only for the claimed message
| $claimed = $this->driverConnection->createQueryBuilder() | ||
| ->update($this->configuration['table_name']) | ||
| ->set('delivered_at', ':now') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this will invoke an exclusive lock on row/record level to ensure we wont have race conditions and multiple workers handling the same message.
either we can update the message, that means the message id has not been updated (delivered_at)
or we wont find the message to update and go on with the next message id we can try
| $ids = $this->selectMessageIdsToDelete(); | ||
| $this->driverConnection->createQueryBuilder() | ||
| ->delete($this->configuration['table_name']) | ||
| ->where('id IN (:ids)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
row/record level exclusive lock to not interfere with the selecting part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
would it work to replace this by a subquery instead of doing a roundtrip to get the ids?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
id rather not use subqueries. they are very often a cause of performance flaws
maybe someone competent enough in other platforms like oracle or postgres can decide if it should be adapted there aswell 🤷♂️ |
what do you think? shall we approach to merge this? then the tests must be fixed... should this be first controlled with a flag to have an opt in? to not possibly break anything? |
src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Connection.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Connection.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| } | ||
| if (!isset($claimedId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the var should be declared before
| if (!isset($claimedId)) { | |
| if (null ===$claimedId) { |
| $ids = $this->selectMessageIdsToDelete(); | ||
| $this->driverConnection->createQueryBuilder() | ||
| ->delete($this->configuration['table_name']) | ||
| ->where('id IN (:ids)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
would it work to replace this by a subquery instead of doing a roundtrip to get the ids?
| Types::STRING, | ||
| Types::STRING, | ||
| ]) | ||
| ->setMaxResults(5_000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what if there are more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no problem. each time one message gets claimed 5k are being deleted.
src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Connection.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| @@ -668,46 +668,41 @@ public function testGeneratedSql(AbstractPlatform $platform, string $expectedSql | |||
| $connection->get(); | |||
| } | |||
| public static function providePlatformSql(): iterable | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it'd be nice to reduce the diff on this file, doable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
tbh no. i changed the algorithm and the queries. i tried to, but that was the best i could do here on the unit test
…ection.phpCo-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
…ection.phpCo-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
…ection.phpCo-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
introduce a variable instead of checking isset.
| } | ||
| } | ||
| if (!null === $claimedId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if (!null ===$claimedId) { | |
| if (null ===$claimedId) { |
Typo i gues
| $claimedId = null; | ||
| foreach ($possibleIdsToClaim as $id) { | ||
| if (null === $claimedId = $this->claimMessage($id)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if (null===$claimedId =$this->claimMessage($id)) { | |
| if (null!==$claimedId =$this->claimMessage($id)) { |
Uh oh!
There was an error while loading.Please reload this page.
i experienced alot of lock wait time and some deadlocks on my database when working with multiple workers handling the same queue. so i fixed it in my application with a decorater. i want to contribute back to symfony.
introduce a algorithm in
Connectionfor mysql platforms to minimize exclusive locking[TODO list]