Skip to content

Ability to support pgbouncer with transaction pool_mode #339

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

Closed
aleksey-mashanov opened this issue Aug 13, 2018 · 15 comments
Closed

Ability to support pgbouncer with transaction pool_mode #339

aleksey-mashanov opened this issue Aug 13, 2018 · 15 comments

Comments

@aleksey-mashanov
Copy link
Contributor

I have seen previous discussions on this topic (#121 (comment)), but still have use case where using pool_mode=session is not an option. We used to operate clusters of applications distributed over tenths to hundreds servers with dozens processes running asynchronous event loop on each. Every process has connection pool to allow several concurrent requests to the database to be processed at the same time. All of this gives us over thousand open connections to the database (most of them are idle because we query PostgreSQL not on every incoming request). The only solution to handle this without impact on PostgreSQL performance is to use pgbouncer in pool_mode=transaction.
We have plans to provide a PR allowing to disable prepared statements. Are you still solid in your opinion or we can agree to merge this feature to upstream?

@elprans
Copy link
Member

elprans commented Aug 13, 2018

You can already prevent the use of named prepared statements with statement_cache_size=0. Avoiding the use of Prepare is impossible, as it's the only way to pass query arguments without interpolating them into your query string. What is your plan here?

@elprans
Copy link
Member

elprans commented Aug 13, 2018

I still think that fixing pgbouncer to handle the connection affinity at least in the anonymous Prepare/Bind/Execute cycle is the only sane way to go.

@aleksey-mashanov
Copy link
Contributor Author

I still think that fixing pgbouncer to handle the connection affinity at least in the anonymous Prepare/Bind/Execute cycle is the only sane way to go.

It is still two roundtrips from client to server not one and connection will be pinned to client between them. More then that the latency will be higher. Network costs are the most notable for simple queries.

You can already prevent the use of named prepared statements with statement_cache_size=0. Avoiding the use of Prepare is impossible, as it's the only way to pass query arguments without interpolating them into your query string. What is your plan here?

I don't see how statement_cache_size=0 can help to deal with pool_mode=transaction, may be I missed something. The plan is to make it work, if to interpolate is the only solution then so be it.

@elprans
Copy link
Member

elprans commented Aug 13, 2018

You still need Prepare to get the output description. Simple query with interpolated arguments is only good for connection.execute() where you don't need the query output, since the exchange format for the simple-query flow is always text, and asyncpg does not really do the nice Python object conversion from text output.

It is still two roundtrips from client to server not one and connection will be pinned to client between them. More then that the latency will be higher.

This is one of the reasons why the cache is there in the first place. Two roundtrips is the minimum for the extended-query protocol.

@aleksey-mashanov
Copy link
Contributor Author

aleksey-mashanov commented Aug 14, 2018

From the source codes of pgbouncer it looks like sending Parse/Bind/Describe/Execute/Sync should work (with exactly one terminating Sync) and this can be done in a one packet with one roundtrip. Still need some tests though.

@aleksey-mashanov
Copy link
Contributor Author

Yep, simple tests show what P/D/B/E/S works fine with pgbouncer. The hard thing will be to nicely integrate it into asyncpg and not break introspection middle-query. I will try to.

@elprans
Copy link
Member

elprans commented Aug 15, 2018

The introspection query should not be a problem, it is treated as a separate query, after which the original query flow is restarted (if anonymous statements are used), or continued (if a named prepared statement was used in Parse/Describe).

@aleksey-mashanov
Copy link
Contributor Author

Implemented in the fork: https://github.com/my-mail-ru/asyncpg

Feel free to close this issue.

@elprans elprans changed the title Ability to disable Prepared Statements Ability to support pgbouncer with transaction pool_mode Aug 28, 2018
@elprans
Copy link
Member

elprans commented Aug 28, 2018

This is a valid issue, and advertising a fork with a rejected patch is not a resolution, so this should remain open until an acceptable fix is implemented.

@aleksey-mashanov
Copy link
Contributor Author

OK. Hope someday we can switch to use upstream version.

@gmr
Copy link

gmr commented May 25, 2019

I have a use case (many distributed processes) across many machines that use pgbouncer in txn mode to drop connection count. All processes call a single stored-proc for writing to the database.

Avoiding the use of Prepare is impossible, as it's the only way to pass query arguments without interpolating them into your query string.

Is there an option here for me to interpolate the args and provide a single query with no args, explicitly tell asyncpg to not prepare, and meet your requirements? I'm not dealing with user input, don't need to be concerned about input validation, etc.

@fvannee
Copy link
Contributor

fvannee commented Sep 23, 2019

Any news on this? I basically have the same use-case: many different distributed processes that I want to multiplex on a few pgbouncer connections in transaction mode.

@elprans
Copy link
Member

elprans commented Sep 23, 2019

I haven't had time to work on this properly yet. I described a way to fix this in #348 (comment), so if anyone wants to take a stab at it, I'll be happy to review.

@fvannee
Copy link
Contributor

fvannee commented Nov 4, 2019

@elprans I implemented an intermediate solution that works based on your feedback in #348, by always sending just one SYNC in the parse/describe/bind/execute sequence. Could you let me know if that's an acceptable solution for you?
#493

@elprans
Copy link
Member

elprans commented Nov 27, 2020

#493 was merged a while ago, so I guess we can close this one.

@elprans elprans closed this as completed Nov 27, 2020
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

No branches or pull requests

4 participants