bug #40376 [Messenger] Don't lock tables or start transactions (Nyholm)
This PR was merged into the 5.2 branch.
Discussion
----------
[Messenger] Don't lock tables or start transactions
| 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](https://github.com/symfony/symfony/pull/40336#issuecomment-790622951) 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 https://github.com/symfony/symfony/pull/35485, could you share some knowledge if this is a good or bad idea?
Commits
-------
26061a131d
Dont lock tables or start transactions
This commit is contained in:
commit
9978ba805f
|
@ -51,16 +51,10 @@ class PostgreSqlConnectionTest extends TestCase
|
||||||
$table->addOption('_symfony_messenger_table_name', 'queue_table');
|
$table->addOption('_symfony_messenger_table_name', 'queue_table');
|
||||||
$sql = implode("\n", $connection->getExtraSetupSqlForTable($table));
|
$sql = implode("\n", $connection->getExtraSetupSqlForTable($table));
|
||||||
|
|
||||||
/*
|
|
||||||
* We need to start a transaction for the following commands to work properly:
|
|
||||||
* doctrine:schema:create
|
|
||||||
* messenger:setup-transports
|
|
||||||
* doctrine:migrations:diff and doctrine:migrations:migrate
|
|
||||||
*/
|
|
||||||
$this->assertStringContainsString('BEGIN;', $sql);
|
|
||||||
$this->assertStringContainsString('CREATE TRIGGER', $sql);
|
$this->assertStringContainsString('CREATE TRIGGER', $sql);
|
||||||
|
|
||||||
// We MUST NOT commit, that will mess with the PDO in PHP 8
|
// We MUST NOT use transaction, that will mess with the PDO in PHP 8
|
||||||
|
$this->assertStringNotContainsString('BEGIN;', $sql);
|
||||||
$this->assertStringNotContainsString('COMMIT;', $sql);
|
$this->assertStringNotContainsString('COMMIT;', $sql);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -109,8 +109,6 @@ final class PostgreSqlConnection extends Connection
|
||||||
private function getTriggerSql(): array
|
private function getTriggerSql(): array
|
||||||
{
|
{
|
||||||
return [
|
return [
|
||||||
'BEGIN;',
|
|
||||||
sprintf('LOCK TABLE %s;', $this->configuration['table_name']),
|
|
||||||
// create trigger function
|
// create trigger function
|
||||||
sprintf(<<<'SQL'
|
sprintf(<<<'SQL'
|
||||||
CREATE OR REPLACE FUNCTION notify_%1$s() RETURNS TRIGGER AS $$
|
CREATE OR REPLACE FUNCTION notify_%1$s() RETURNS TRIGGER AS $$
|
||||||
|
|
Reference in New Issue