asyncpg icon indicating copy to clipboard operation
asyncpg copied to clipboard

[Feature Request] Allow to manage reset query for connection

Open alexeynavarkin opened this issue 4 years ago • 12 comments

Every connection on release calls reset, which results in evaluation of this statements (for postgres caps):

SELECT pg_advisory_unlock_all();
CLOSE ALL;
UNLISTEN *;
RESET ALL;

So if you use postgres your caps predefined. But not every project uses this capabilities, so this calls looks like overhead.

Maybe add interface to specify server caps explicitly, this will reduce reset statements to only useful ones or even noop at all.

Something like pass server caps in init of connection or/and connection pool.

alexeynavarkin avatar Jul 08 '21 12:07 alexeynavarkin

There's already a way to override connection behavior: make a subclass and pass it in as connection_class argument to connect().

elprans avatar Jul 08 '21 16:07 elprans

Thanks for reply!

But is there something, that stopping implementation of this feature in default one?

As I see, almost in every project you would need to implement custom, almost one line class, if you want to optimize performance. In case of many short queries asyncpg will almost double qps to postgresql. I think many connection poolers has interface for tuning connection reset algorithm, in example pg_bouncer server_reset_query param.

I suppose, that most projects do not use all the caps.

We can keep default behavior, but add caps arg to init, that will override autodetection, or allow directly set reset query.

alexeynavarkin avatar Jul 08 '21 17:07 alexeynavarkin

Server capabilities aren't meant to be an API and overriding the detection is a bad idea, because new releases might add more caps for newer Postgres versions or alternative implementations and you overload will become outdated, which might lead to unexpected behavior. Basically, this is a dangerous way to achieve better performance.

allow directly set reset query

Setting the reset query directly is a more reasonable approach. Feel free to send a PR.

elprans avatar Jul 08 '21 17:07 elprans

I stumbled upon this thread, because we noticed a lot of pg_advisory_unlock_all, sometimes taking a significant amount of time to complete. But I don't see any use of advisory_locks in asyncpg codebase. But I may be mistaken.

Is there any doc for these server capabilities ? And why there are these unused ones ?

mdespriee avatar May 25 '22 16:05 mdespriee

Just wondering if it's possible to add this reset query to the last executed query before connection is released instead of making a separate request that ends up in extra round trip to DB?

LMalikov avatar Jan 12 '23 19:01 LMalikov

@LMalikov i think - it can not be done in that way.

At least when asyncpg executing query it can't understand that it is the last one query in a connection context.

alexeynavarkin avatar Jan 16 '23 16:01 alexeynavarkin

@mdespriee i can't see any documentation about this.

But you can understand how that works from here:

https://github.com/MagicStack/asyncpg/blob/v0.27.0/asyncpg/connection.py#L93 - how caps determined.

https://github.com/MagicStack/asyncpg/blob/v0.27.0/asyncpg/connection.py#L1525 - how thay used to generate reset query that executed before connection release.

alexeynavarkin avatar Jan 16 '23 16:01 alexeynavarkin

The pgbouncer admin console complains with asyncpg clients:

CLOSE ALL;
UNLISTEN *;
RESET ALL;', use SHOW HELP;

is related, as it seems?

sandr8 avatar Jan 18 '23 13:01 sandr8

@sandr8 Looks like so, except the last one statement use show help - that part not related to asyncpg.

alexeynavarkin avatar Jan 19 '23 05:01 alexeynavarkin

That's part of the notice message from pgbouncer of course... which is quoting the command sent by pgbouncer. What I would like to understand, when I find the time, is what the deal is with the «capabilities» cited above and if, as it sounds, the server is supposed somehow to let the client know what the client can do, and the client should act accordingly.

sandr8 avatar Jan 20 '23 14:01 sandr8

@sandr8 i don't think you can do anything with it from pgbouncer/postgresql side.

This can be configured in asyncpg as @elprans suggested earlier or i need some time to create PR to make it more easy to config.

alexeynavarkin avatar Jan 20 '23 17:01 alexeynavarkin

I took a look at the code. I think the current approach is destined to become more complicated. For example, CockroachDB is soon going to support PL/pgSQL. At that point the code will have to branch on the CockroachDB version. Why not just dynamically try...except each little bit of the reset query at the beginning of the (first) connection (within a pool) and see and remember which parts are accepted?

sandr8 avatar Jan 21 '23 13:01 sandr8