[Messenger] Add support for PostgreSQL LISTEN/NOTIFY #35485

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.

Sign up for GitHub

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

Jump to bottom

Merged

fabpot merged 1 commit into symfony:master from dunglas:messenger-pg-listen-notify

Feb 4, 2020

Member

dunglas commented Jan 27, 2020 •

edited

Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	n/a
License	MIT
Doc PR	todo

PostgreSQL comes with a builtin, performant, scalable and transactional pub/sub system called LISTEN/NOTIFY.
This PR allows to leverage this mechanism when using the Messenger component with Postgres.

When the Postgres is used, workers are notified in real-time when a message is dispatched.
This reduces the latency, and prevents the worker from executing useless SQL queries. Basically, it allows to switch from a polling-based approach to an event-based one.

This patch can be used with all existing installation of Messenger, as long as the underlying DBMS is Postgres. For many (most ?) projects, it allows to get the benefits of using a queue system such as RabbitMQ or Pulsar without having to introduce new services to monitor, replicate or upgrade.

If PostgreSQL is used, LISTEN/NOTIFY is used automatically!

That's all!

It's also possible to configure how long the worker must wait for new messages:

framework:
    messenger:
        transports:
            async:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                options:
                    pgsql_get_notify: true
                    pgsql_get_notify_timeout: 500

Then you can use start the workers with something like: php bin/console messenger:consume --sleep=0

A demo app using this new feature is available in this repository: https://github.com/dunglas/demo-postgres-listen-notify

TODO:

Add tests

dunglas requested a review from sroze as a code owner

January 27, 2020 17:25

carsonbot added Status: Needs Review Messenger Feature labels

dunglas force-pushed the messenger-pg-listen-notify branch 3 times, most recently from 0171724 to 5363561 Compare

January 27, 2020 17:30

jderusse reviewed

View reviewed changes

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php Outdated Show resolved Hide resolved

Member Author

dunglas commented Jan 27, 2020 •

edited

Loading

An alternative implementation is to use a trigger instead of calling pg_notify directly from PHP. The benefit of using a trigger is that the system works even if another app or client insert a line in the Messenger's database. The drawback is that Doctrine DBAL doesn't "support" triggers, and we'll have to execute a custom query after the setup (not a big deal IMHO).

For the record, I first used LISTEN/NOTIFY as a driver for the HA version of Mercure.rocks. For Mercure, I use a trigger, and it works perfectly well.

Edit: implementation added in 5207f51

dunglas force-pushed the messenger-pg-listen-notify branch from 51fc6ee to 5207f51 Compare

January 27, 2020 19:39

bendavies reviewed

View reviewed changes

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php Outdated Show resolved Hide resolved

Ocramius suggested changes

View reviewed changes

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

@@ @@ -35,6 +35,9 @@ class Connection @@
                       'queue_name' => 'default',
                       'redeliver_timeout' => 3600,
                       'auto_setup' => true,
+                      'pgsql_get_notify' => false,

Contributor

Ocramius Jan 28, 2020

This should probably be its own transport, not mixing PostgreSQL-specifics with DBAL abstractions

Member Author

dunglas Jan 28, 2020

I had the same feeling in the beginning, but it will require to duplicate most of the code of this transport. So I'm not sure that it is worth it from a maintenance point of view.
Now that the implementation is less intrusive than it was, I'll check if the PostgreSQL related code can be moved to a decorator.

Contributor

Ocramius Jan 28, 2020

Yeah, but that's not a big deal: more problematic is having generic SQL and PGSQL mixed within the same service, activated conditionally.

If the "enable norify" logic deals to a completely dedicated adapter, fixing specific bugs and maintenance becomes much easier, and so will debugging too.

Member Author

dunglas Jan 28, 2020 •

edited

Loading

I isolated the PostgresSQL-specific code in a dedicated class, it's a bit cleaner, and while far from being perfect I think it will be good enough for now at least.

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php Outdated Show resolved Hide resolved

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

bendavies reviewed

View reviewed changes

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

@@ @@ -227,6 +270,28 @@ public function setup(): void @@
                           $this->driverConnection->getConfiguration()->setFilterSchemaAssetsExpression($assetFilter);
                       }
+                      if ($this->usePgsqlNotify) {
+                          $sql = sprintf(<<<'SQL'

Contributor

bendavies Jan 28, 2020

wrap in begin...commit?

Member Author

dunglas Jan 28, 2020

What would be the benefit?

Contributor

bendavies Jan 28, 2020

is exec transactional? you want both statements to fail or both to succeed.

Member Author

dunglas Jan 28, 2020

I'm not sure, pg_query is transnational, but I cannot find the information for exec.

dunglas force-pushed the messenger-pg-listen-notify branch 2 times, most recently from a131f83 to 2be2850 Compare

January 28, 2020 17:28

gquemener mentioned this pull request

Make use of Postgres pub/sub ? prooph/pdo-event-store#86

Open

chalasr approved these changes

View reviewed changes

Member

chalasr left a comment

with minor comment

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Connection.php Show resolved Hide resolved

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/PostgreSqlConnection.php Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Work labels

nicolas-grekas added this to the next milestone

dunglas force-pushed the messenger-pg-listen-notify branch from a32d12f to b976720 Compare

January 31, 2020 18:15

fabpot approved these changes

View reviewed changes

Member

fabpot left a comment

LGTM, minor comment.

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/PostgreSqlConnection.php

+               * file that was distributed with this source code.
+               */
+              declare(strict_types=1);

Member

fabpot Feb 3, 2020

Should be removed


          [Messenger] Add support for PostgreSQL LISTEN/NOTIFY

01f33c3

fabpot force-pushed the messenger-pg-listen-notify branch from b976720 to 01f33c3 Compare

February 4, 2020 10:43

Member

fabpot commented Feb 4, 2020

Thank you @dunglas.

fabpot mentioned this pull request

[Messenger] Add support for PostgreSQL LISTEN/NOTIFY symfony/symfony-docs#13039

Closed

fabpot closed this in

e3fa80a

fabpot merged commit 01f33c3 into symfony:master

dunglas deleted the messenger-pg-listen-notify branch

February 5, 2020 00:40

lyrixx mentioned this pull request

[Messenger] Made final class really final #35600

Merged

chalasr added a commit that referenced this pull request


          minor #35600 [Messenger] Made final class really final (lyrixx)

0b8adc9

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Messenger] Made final class really final

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

introduced in #35485 (so no BC break)

Commits
-------

cd8a386 [Messenger] Made final class really final

tacman mentioned this pull request

Support queued messages via Symfony Messenger? FriendsOfSymfony/FOSElasticaBundle#1586

Closed

nicolas-grekas modified the milestones: next, 5.1

fabpot mentioned this pull request

Release v5.1.0-BETA1 #36713

Merged

gubler mentioned this pull request

Error setting up Doctrine transport for Messenger with MS SQL Server #37532

Closed

t-richard mentioned this pull request

[Messenger] Add options for PostgreSQL LISTEN/NOTIFY support symfony/symfony-docs#15043

Merged

Nyholm mentioned this pull request

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

Merged

fabpot added a commit that referenced this pull request


          bug #40376 [Messenger] Don't lock tables or start transactions (Nyholm)

9978ba8

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](#40336 (comment)) 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?

Commits
-------

26061a1 Dont lock tables or start transactions

wouterj added a commit to symfony/symfony-docs that referenced this pull request


          feature #15043 [Messenger] Add options for PostgreSQL LISTEN/NOTIFY s…

d74f702

…upport (t-richard)

This PR was squashed before being merged into the 5.2 branch.

Discussion
----------

[Messenger] Add options for PostgreSQL LISTEN/NOTIFY support

<!--

If your pull request fixes a BUG, use the oldest maintained branch that contains
the bug (see https://symfony.com/releases for the list of maintained branches).

If your pull request documents a NEW FEATURE, use the same Symfony branch where
the feature was introduced (and `5.x` for features of unreleased versions).

-->
Documents symfony/symfony#35485

Closes #13039

Commits
-------

55edd48 [Messenger] Add options for PostgreSQL LISTEN/NOTIFY support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Ocramius Ocramius requested changes

jderusse jderusse left review comments

bendavies bendavies left review comments

fabpot fabpot approved these changes

chalasr chalasr approved these changes

sroze Awaiting requested review from sroze

Labels

Feature Messenger Status: Reviewed