[Messenger] Adds option to force indexes creation in Mysql platform for Doctrine transport #45007

gianiaz · Jan 13, 2022

Q	A
Branch?	4.4
Bug fix?	maybe
New feature?	yes
Deprecations?	no
Tickets	#42868
License	MIT
Doc PR	TBA

After upgrading to 5.4 I've had a migration on messenger_messages caused by #42345.

Considering that:

1- doctrine_transport is often used for failed messages
2- with large tables we have degraded performance without indexes
3- get/findAll/count methods in Connection.php are using createAvailableMessagesQueryBuilder

symfony/src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

Lines 304 to 312 in 9f5238d

    
           private function createAvailableMessagesQueryBuilder(): QueryBuilder 
        
           { 
        
               $now = new \DateTime(); 
        
               $redeliverLimit = (clone $now)->modify(sprintf('-%d seconds', $this->configuration['redeliver_timeout'])); 
        
               return $this->createQueryBuilder() 
        
                   ->where('m.delivered_at is null OR m.delivered_at < ?') 
        
                   ->andWhere('m.available_at <= ?') 
        
                   ->andWhere('m.queue_name = ?')

method, that adds where conditions on available_at and queue_name are used.

I would like to specify that in my conditions a migration for removing indexes should not be created.

Can I help in porting it in 5.4 or you have your flow for porting?

Thank you

…or Doctrine transport

carsonbot · Jan 13, 2022

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 6.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

carsonbot · Jan 14, 2022

Hey!

I think @nikophil has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

94noni · Jan 16, 2022

1- doctrine_transport is often used for failed messages

indeed, and sometime even as main backend for messenger as generaly all project do have a database but not a redis nor rabbitmq etc

gianiaz · Jan 21, 2022

Can I do something to push forward this PR?

Ci has failed for memory reason, not for code

tifabien · Feb 2, 2022

2- with large tables we have degraded performance without indexes

Confirming that with a large dataset in the table, this is just not usable...

nicolas-grekas · Feb 4, 2022

I'd prefer if this were not configurable.
What about reverting #42345 and instead handle the deadlock exception as a simple UNACK? (aka requeue the message)?
/cc @jeroennoten

nicolas-grekas · Feb 4, 2022

/cc @PalTamasWBA also in case you can help us make the best move here.

Jean85 · Feb 4, 2022

What about reverting #42345 and instead handle the deadlock exception as a simple UNACK? (aka requeue the message)?

Wouldn't that make it even worse? Requeueing would mean even more write attempts on a table which is already under stress due to locks.

Basically, whenever this transport is used in a high-concurrency environment (which IMO should be discouraged), it starts exhibiting the needs of #42345; if you instead use it as a failed queue (which is what I and @gianiaz do), it's the opposite, the indexes become a must.

[EDIT] IMHO this is a sensible approach; maybe you can decide to reverse the default and push for discouraging its use in high-concurrency environments, and suggest putting the value to false otherwise.

nicolas-grekas · Feb 4, 2022

The issue is not the transport, but MySQL, since eg PostgreySQL doesn't exhibit the behavior.
What happens right now when the deadlock exception is triggered? How can we resume operation after it?
Eg couldn't we just retry in the worker until this works?

Jean85 · Feb 7, 2022

What happens right now when the deadlock exception is triggered?

An exception is raised, so probably the handling fails, and the retry/failed mechanisms of Messenger kicks in.

How can we resume operation after it?

IMO we should proceed with a failed handling, and this PR acts as a differentiator between two main possible situations:

we're in a normal queue, so not having the indexes on MySQL solves the issue of locks and the situation allows it (high concurrency but low/zero quantity of stored messages)
we're in a failed queue, so concurrency is never an issue, and having the indexes is better, because we have the opposite situation (low/no concurrency, high quantity of stored messages)

Eg couldn't we just retry in the worker until this works?

We definitely could. But depending on the context, if you leave the indexes you create a cascading failure scenario, with the retry/failed mechanism that makes the locking issue worse. So IMHO #42345 is still a valid approach, just not everywhere, and this PR provides the means to avoid that.

nicolas-grekas · Mar 30, 2022

I'm proposing #45888 instead, can you please have a look?

…ks using soft-delete (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Messenger] Add mysql indexes back and work around deadlocks using soft-delete | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #42868 | License | MIT | Doc PR | - #42345 removed some indexes because they create too many deadlocks when running many concurrent consumers. Yet, as reported in the linked issue, those indexes are useful when processing large queues (typically the failed messages queue). #45007 is proposing to add an option to force the indexes back, but I don't like it because it requires ppl to learn about the issue. I looked for a more seamless solution and here is my proposal. Instead of possibly triggering the deadlock during `ack()`/`reject()`, I propose to use a soft-delete there, and do the real delete in `get()`. This makes ack/reject safe because they don't alter any indexes anymore (the delete was), and this moves deadlock exceptions to the same function that creates the locks. This allows the retry mechanism in `DoctrineReceiver` to recover from at most 3 consecutive deadlock exceptions. There can be more, and in this case, the consumer will stop. But this should be much less likely. (yes, I did create a reproducer to work on this issue ;) ) Commits ------- 12271a4 [Messenger] Add mysql indexes back and work around deadlocks using soft-delete

gianiaz added 2 commits January 13, 2022 10:23

[Messenger] Adds option to force indexes creation in Mysql platform f…

66f08be

…or Doctrine transport

[Messenger] Adds entry in CHANGELOG.md

ce6eedf

carsonbot added Status: Needs Review Feature Messenger labels Jan 13, 2022

carsonbot added this to the 4.4 milestone Jan 13, 2022

gianiaz added 2 commits January 13, 2022 10:53

[Messenger] Code style

09f6fd3

[Messenger] Code style on ConnectionTest.php

17ec023

gianiaz mentioned this pull request Jan 13, 2022

[Messenger] Adds option for forcing index creation on messenger_messages symfony/doctrine-messenger#4

Open

ctippler mentioned this pull request Feb 18, 2022

[Bug]: Processing of messenger_messages becomes very slow pimcore/pimcore#11486

Closed

nicolas-grekas mentioned this pull request Mar 30, 2022

[Messenger] Add mysql indexes back and work around deadlocks using soft-delete #45888

Merged

nicolas-grekas closed this Mar 30, 2022

dipa-damco mentioned this pull request Aug 4, 2023

Deadlock pimcore/pimcore#15688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Messenger] Adds option to force indexes creation in Mysql platform for Doctrine transport #45007

[Messenger] Adds option to force indexes creation in Mysql platform for Doctrine transport #45007

Uh oh!

gianiaz commented Jan 13, 2022 •

edited by nicolas-grekas

Loading

Uh oh!

carsonbot commented Jan 13, 2022

Uh oh!

carsonbot commented Jan 14, 2022

Uh oh!

94noni commented Jan 16, 2022

Uh oh!

gianiaz commented Jan 21, 2022

Uh oh!

tifabien commented Feb 2, 2022

Uh oh!

nicolas-grekas commented Feb 4, 2022 •

edited

Loading

Uh oh!

nicolas-grekas commented Feb 4, 2022

Uh oh!

Jean85 commented Feb 4, 2022 •

edited

Loading

Uh oh!

nicolas-grekas commented Feb 4, 2022

Uh oh!

Jean85 commented Feb 7, 2022 •

edited

Loading

Uh oh!

nicolas-grekas commented Mar 30, 2022

Uh oh!

Uh oh!

	private function createAvailableMessagesQueryBuilder(): QueryBuilder
	{
	$now = new \DateTime();
	$redeliverLimit = (clone $now)->modify(sprintf('-%d seconds', $this->configuration['redeliver_timeout']));

	return $this->createQueryBuilder()
	->where('m.delivered_at is null OR m.delivered_at < ?')
	->andWhere('m.available_at <= ?')
	->andWhere('m.queue_name = ?')

Search code, repositories, users, issues, pull requests...

Uh oh!

[Messenger] Adds option to force indexes creation in Mysql platform for Doctrine transport #45007

[Messenger] Adds option to force indexes creation in Mysql platform for Doctrine transport #45007

Uh oh!

Conversation

gianiaz commented Jan 13, 2022 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonbot commented Jan 13, 2022

Uh oh!

carsonbot commented Jan 14, 2022

Uh oh!

94noni commented Jan 16, 2022

Uh oh!

gianiaz commented Jan 21, 2022

Uh oh!

tifabien commented Feb 2, 2022

Uh oh!

nicolas-grekas commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Feb 4, 2022

Uh oh!

Jean85 commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Feb 4, 2022

Uh oh!

Jean85 commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Mar 30, 2022

Uh oh!

Uh oh!

gianiaz commented Jan 13, 2022 •

edited by nicolas-grekas

Loading

nicolas-grekas commented Feb 4, 2022 •

edited

Loading

Jean85 commented Feb 4, 2022 •

edited

Loading

Jean85 commented Feb 7, 2022 •

edited

Loading