Skip to content

[Feature Request] Allow to manage reset query for connection #780

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Closed
alexeynavarkin opened this issue Jul 8, 2021 · 12 comments · Fixed by #1191
Closed

[Feature Request] Allow to manage reset query for connection #780

alexeynavarkin opened this issue Jul 8, 2021 · 12 comments · Fixed by #1191

Comments

@alexeynavarkin
Copy link

alexeynavarkin commented Jul 8, 2021

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.

@elprans
Copy link
Member

elprans commented Jul 8, 2021

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

@alexeynavarkin
Copy link
Author

alexeynavarkin commented Jul 8, 2021

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.

@elprans
Copy link
Member

elprans commented Jul 8, 2021

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.

@mdespriee
Copy link

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 ?

@LMalikov
Copy link

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?

@alexeynavarkin
Copy link
Author

@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
Copy link
Author

alexeynavarkin commented Jan 16, 2023

@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.

@sandr8
Copy link

sandr8 commented Jan 18, 2023

The pgbouncer admin console complains with asyncpg clients:

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

is related, as it seems?

@alexeynavarkin
Copy link
Author

alexeynavarkin commented Jan 19, 2023

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

@sandr8
Copy link

sandr8 commented Jan 20, 2023

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.

@alexeynavarkin
Copy link
Author

@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.

@sandr8
Copy link

sandr8 commented Jan 21, 2023

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?

dsav added a commit to dsav/asyncpg that referenced this issue Apr 12, 2024
The default connection reset behaviour might be an overkill
for some cases e.g. when respective server capabilities are not used
or resources are cleaned up explicitly before returning the connection
to the pool.

This provides an opt-in way to override this default behaviour.

Also see MagicStack#780
elprans added a commit that referenced this issue Oct 18, 2024
A coroutine can be passed to the new `reset` argument of `create_pool`
to control what happens to the connection when it is returned back to
the pool by `release()`.  By default `Connection.reset()` is called.
Additionally, `Connection.get_reset_query` is renamed from
`Connection._get_reset_query` to enable an alternative way of
customizing the reset process via subclassing.

Closes: #780
Closes: #1146
elprans added a commit that referenced this issue Oct 18, 2024
A coroutine can be passed to the new `reset` argument of `create_pool`
to control what happens to the connection when it is returned back to
the pool by `release()`.  By default `Connection.reset()` is called.
Additionally, `Connection.get_reset_query` is renamed from
`Connection._get_reset_query` to enable an alternative way of
customizing the reset process via subclassing.

Closes: #780
Closes: #1146
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants