Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Messenger] Don't lock tables or start transactions #40376

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Mar 5, 2021

Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

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 PDO behaves differently in PHP 7.4 compared to PHP 8.

Scenario Command Is executed in transaction Method that calls PostgreSqlConnection::getTriggerSql()
A messenger:setup-transports No setup()
B doctrine:schema:create No getExtraSetupSqlForTable()
C doctrine:migration:diff Yes by default, but it can be configured getExtraSetupSqlForTable()

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:

We don't need COMMIT because the transaction will commit itself when we close the connection.

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 @fbourigault suggestion 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 removes LOCK TABLE and 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 TABLE in your initial version of this class in #35485, could you share some knowledge if this is a good or bad idea?

@dunglas
Copy link
Member

dunglas commented Mar 5, 2021

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
Copy link
Member

fabpot commented Mar 5, 2021

Thank you @Nyholm.

@fabpot fabpot merged commit 9978ba8 into symfony:5.2 Mar 5, 2021
@Nyholm
Copy link
Member Author

Nyholm commented Mar 5, 2021

Thank you for merging. I’m sorry that I did not find this issue yesterday.

@Nyholm Nyholm deleted the messenger-postgress-transaction branch March 5, 2021 20:20
@fabpot fabpot mentioned this pull request Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants