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

Avoid unnecessary pg_listening_channels queries on connection release #648

Merged
merged 1 commit into from
Feb 10, 2021

Conversation

kitogo
Copy link
Contributor

@kitogo kitogo commented Nov 2, 2020

There's no need to unlisten on all channels if there are no active listeners on the connection. Querying pg_listening_channels could be quite time consuming even when no listeners are added. For example, 20% of all time spent on DB operations goes to pg_listening_channels on our project.

if (
caps.notifications and
caps.plpgsql and
len(self._listeners.keys()) != 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't account for any potential listeners created with LISTEN instead of with the connection API.

@mikosowski
Copy link

Hi! Any news on this PR? I have similar issue: we don't add any listeners, but pg_listening_channels is queried every time when connection is reseted. It's quite time consuming. Is there any solution for this?

@kitogo
Copy link
Contributor Author

kitogo commented Jan 26, 2021

Sorry, I kinda dropped the ball on this one.
Thanks a lot for the feedback @elprans , I didn't think about that use case. I can look into extending _ClientConfiguration to include a flag that can be used to disable listeners API on the connection. This of course would still allow creating listeners with LISTEN, but at least as a user you would explicitly disable subscribing/unsubscribing to Postgres notifications through asyncpg API and would have to deal with this manually if you decide to useLISTEN. Does this sound like a feasible solution?

@mikosowski
Copy link

For me this solution is completely OK ;) I'll be very happy if we push forward the work on this PR.

@elprans
Copy link
Member

elprans commented Jan 27, 2021

The majority of the overhead here comes from the use of the PL/pgSQL DO block, which is actually not necessary. IIRC, it was added to guard against some old/alternative Postgres implementation, but that case can be solved by tweaking capabilities instead. Dropping the conditional block around UNLISTEN removes over 50% of the acquire()/release() overhead:

diff --git a/asyncpg/connection.py b/asyncpg/connection.py
index e62928f..13b8283 100644
--- a/asyncpg/connection.py
+++ b/asyncpg/connection.py
@@ -1500,17 +1500,8 @@ class Connection(metaclass=ConnectionMeta):
             _reset_query.append('SELECT pg_advisory_unlock_all();')
         if caps.sql_close_all:
             _reset_query.append('CLOSE ALL;')
-        if caps.notifications and caps.plpgsql:
-            _reset_query.append('''
-                DO $$
-                BEGIN
-                    PERFORM * FROM pg_listening_channels() LIMIT 1;
-                    IF FOUND THEN
-                        UNLISTEN *;
-                    END IF;
-                END;
-                $$;
-            ''')
+        if caps.notifications:
+            _reset_query.append('UNLISTEN *;')
         if caps.sql_reset:
             _reset_query.append('RESET ALL;')

@kitogo
Copy link
Contributor Author

kitogo commented Feb 2, 2021

Great, thanks for clarification on this @elprans ! Seems like conditional block around UNLISTEN was added in 2b104e0 to fix connection reset when running against instances in hot standby mode, since PostgreSQL versions older than 9.4 didn't support UNLISTEN in hot standby. This shouldn't be a problem in all supported PostgreSQL versions though. I will update this PR to only include changes you've suggested in the comment.

UNLISTEN is now available in Hot Standby mode in all supported
PostgreSQL versions, therefore there's no reason anymore to wrap
it in DO block. This should significantly speed up connection reset.
elprans added a commit that referenced this pull request Feb 10, 2021
Currently, `UNLISTEN` in connection release is guarded by an
expensive PL/pgSQL block, because historically PostgreSQL
did not allow `UNLISTEN` on a hot standby node.  This has since
changed, and `UNLISTEN` is allowed in PostgreSQL 9.4.21+, 9.5.16+,
9.6.12+, 10.7+, 11.2+, and versions 12 and newer.  If this change
breaks your setup, upgrade your server.

The removal of the PL/pgSQL guard reduces the `acquire`/`release` overhead
by over 50%.

Upstream discussion: https://postgr.es/m/CADT4RqCf2gA_TJtPAjnGzkC3ZiexfBZiLmA-mV66e4UyuVv8bA@mail.gmail.com

Fixes: #648
@elprans elprans merged commit ff5da5f into MagicStack:master Feb 10, 2021
@elprans elprans mentioned this pull request Feb 10, 2021
elprans added a commit that referenced this pull request Feb 10, 2021
A new asyncpg release is here.

Notable additions include Python 3.9 support, support for recently added
PostgreSQL types like `jsonpath`, and last but not least, vastly
improved `executemany()` performance.  Importantly, `executemany()` is
also now _atomic_, which means that either all iterations succeed, or
none at all, whereas previously partial results would have remained in
place, unless `executemany()` was called in a transaction.

There is also the usual assortment of improvements and bugfixes, see the
details below.

This is the last release of asyncpg that supports Python 3.5, which has
reached EOL last September.

Improvements
------------

* Vastly speedup executemany by batching protocol messages (#295)
  (by @fantix in 690048d for #295)

* Allow using custom `Record` class
  (by @elprans in db4f1a6 for #577)

* Add Python 3.9 support (#610)
  (by @elprans in c05d726 for #610)

* Prefer SSL connections by default (#660)
  (by @elprans in 16183aa for #660)

* Add codecs for a bunch of new builtin types (#665)
  (by @elprans in b53f038 for #665)

* Expose Pool as `asyncpg.Pool` (#669)
  (by @rugleb in 0e0eb8d for #669)

* Avoid unnecessary overhead during connection reset (#648)
  (by @kitogo in ff5da5f for #648)

Fixes
-----

* Add a workaround for bpo-37658
  (by @elprans in 2bac166 for #21894)

* Fix wrong default transaction isolation level (#622)
  (by @fantix in 4a627d5 for #622)

* Fix `set_type_codec()` to accept standard SQL type names (#619)
  (by @elprans in 68b40cb for #619)

* Ignore custom data codec for internal introspection (#618)
  (by @fantix in e064f59 for #618)

* Fix null/NULL quoting in array text encoder (#627)
  (by @fantix in 92aa806 for #627)

* Fix link in connect docstring (#653)
  (by @samuelcolvin in 8b313bd for #653)

* Make asyncpg work with pyinstaller (#651)
  (by @Atem18 in 5ddabb1 for #651)

* Fix possible `AttributeError` exception in `ConnectionSettings` (#632)
  (by @petriborg in 0d23182 for #632)

* Prohibit custom codecs on domains
  (by @elprans in 50f964f for #457)

* Raise proper error on anonymous composite input (tuple arguments) (#664)
  (by @elprans in 7252dbe for #664)

* Fix incorrect application of custom codecs in some cases (#662)
  (by @elprans in 50f65fb for #662)
dmig pushed a commit to dmig/asyncpg that referenced this pull request Feb 22, 2021
UNLISTEN is now available in Hot Standby mode in all supported
PostgreSQL versions, therefore there's no reason anymore to wrap
it in DO block. This should significantly speed up connection reset.
dmig pushed a commit to dmig/asyncpg that referenced this pull request Feb 22, 2021
A new asyncpg release is here.

Notable additions include Python 3.9 support, support for recently added
PostgreSQL types like `jsonpath`, and last but not least, vastly
improved `executemany()` performance.  Importantly, `executemany()` is
also now _atomic_, which means that either all iterations succeed, or
none at all, whereas previously partial results would have remained in
place, unless `executemany()` was called in a transaction.

There is also the usual assortment of improvements and bugfixes, see the
details below.

This is the last release of asyncpg that supports Python 3.5, which has
reached EOL last September.

Improvements
------------

* Vastly speedup executemany by batching protocol messages (MagicStack#295)
  (by @fantix in 690048d for MagicStack#295)

* Allow using custom `Record` class
  (by @elprans in db4f1a6 for MagicStack#577)

* Add Python 3.9 support (MagicStack#610)
  (by @elprans in c05d726 for MagicStack#610)

* Prefer SSL connections by default (MagicStack#660)
  (by @elprans in 16183aa for MagicStack#660)

* Add codecs for a bunch of new builtin types (MagicStack#665)
  (by @elprans in b53f038 for MagicStack#665)

* Expose Pool as `asyncpg.Pool` (MagicStack#669)
  (by @rugleb in 0e0eb8d for MagicStack#669)

* Avoid unnecessary overhead during connection reset (MagicStack#648)
  (by @kitogo in ff5da5f for MagicStack#648)

Fixes
-----

* Add a workaround for bpo-37658
  (by @elprans in 2bac166 for #21894)

* Fix wrong default transaction isolation level (MagicStack#622)
  (by @fantix in 4a627d5 for MagicStack#622)

* Fix `set_type_codec()` to accept standard SQL type names (MagicStack#619)
  (by @elprans in 68b40cb for MagicStack#619)

* Ignore custom data codec for internal introspection (MagicStack#618)
  (by @fantix in e064f59 for MagicStack#618)

* Fix null/NULL quoting in array text encoder (MagicStack#627)
  (by @fantix in 92aa806 for MagicStack#627)

* Fix link in connect docstring (MagicStack#653)
  (by @samuelcolvin in 8b313bd for MagicStack#653)

* Make asyncpg work with pyinstaller (MagicStack#651)
  (by @Atem18 in 5ddabb1 for MagicStack#651)

* Fix possible `AttributeError` exception in `ConnectionSettings` (MagicStack#632)
  (by @petriborg in 0d23182 for MagicStack#632)

* Prohibit custom codecs on domains
  (by @elprans in 50f964f for MagicStack#457)

* Raise proper error on anonymous composite input (tuple arguments) (MagicStack#664)
  (by @elprans in 7252dbe for MagicStack#664)

* Fix incorrect application of custom codecs in some cases (MagicStack#662)
  (by @elprans in 50f65fb for MagicStack#662)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants