Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Don't lock tables or start transactions#40376
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dunglas commentedMar 5, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
IIRC it was to prevent data to be added to the table while the trigger is being created but I think that it's an edge case. (I'm not even sure if it can happen). +1 on my side for this patch. |
fabpot commentedMar 5, 2021
Thank you@Nyholm. |
Nyholm commentedMar 5, 2021
Thank you for merging. I’m sorry that I did not find this issue yesterday. |
I was so sure my PR#40336 fixed the doctrine-messenger issue once and for all. But it had a very silent bug..
This has been tricky to debug and find because the
PDObehaves differently in PHP 7.4 compared to PHP 8.PostgreSqlConnection::getTriggerSql()messenger:setup-transportssetup()doctrine:schema:creategetExtraSetupSqlForTable()doctrine:migration:diffgetExtraSetupSqlForTable()PR#40055 fixed scenario C on PHP 8, but that also broke scenario B on PHP 7.4 and PHP 8.
In PR#40336 I was wrong claiming:
The result was the we removed all the errors messages from the 3 scenarios. But scenario B will produce some SQL that is actually never committed. IE it will silently fail.
I've been trying to figure out a good solution to how or when to start a transaction. I tried out@fbourigaultsuggestion but that would be the same fix as#40055.
We need a transaction because the SQL includes a
LOCK TABLE, however, I cannot see a strict need for it. This PR removesLOCK TABLEand all transaction juggling. It all seams to work.I would be happy to get thorough feedback on this PR so we can end the chapter of constantly adding bugs to this part of the component.
@dunglas, you added
LOCK TABLEin your initial version of this class in#35485, could you share some knowledge if this is a good or bad idea?