Compatibility with pgbouncer with pool_mode != session

[aleksey-mashanov]

Add session parameter to connection() with default value True.
When set to False the client will limit usage of prepared statements
to unnamed in a transaction only.

As a side effect handling of unnamed prepared statements improved
irrespective of session value - they are automatically reprepared
if something invalidating them occurs.

[aleksey-mashanov]

Implements #339

[elprans]

Hi Aleksey. Thanks for working on this. The PR looks interesting, however I have concerns with the introduced redundancy. With this change, uncached anonymous statements would incur the penalty of preparing the query twice instead of once. This might be fine for simple queries, but if a query is sufficiently large and complicated, the Parse step easily dominates everything else.

I feel like the best solution would be to make some changes on the pgbouncer side as well. IIRC, the crux of the issue is that pgbouncer is hardcoded to only support the libpq behaviour by pinning the Parse/Bind/Execute/Sync sequence to a connection. I think teaching it to support Parse/Describe/Sync/Bind/Execute/Sync the same way should remove the need to send Parse twice. It would also be awesome to add a way to deterministically detect pgbouncer (e.g. by having it append a ParameterStatus message on connection), to remove the need for the session argument.

[aleksey-mashanov]

With this change, uncached anonymous statements would incur the penalty of preparing the query twice instead of once.

Uncached statements prepare twice iff session == False. For session == True amount of commands sent to a server is the same compared to the previous version. If session == False and prepared statements cache enabled then unnamed prepared statements cached (because of Describe) and only reParsed on each Bind/Execute.

Session == True: 
    prepare(): Parse/Describe/Sync (once, cached)
    execute(): Bind/Execute/Sync

Session == False && !is_in_transaction():
    prepare(): Parse/Describe/Sync (once, cached)
    execute(): Parse/Bind/Execute/Sync

Session == False && is_in_transaction():
    prepare(): Parse/Describe/Sync (once, cached)
    execute(): Bind/Execute/Sync

I think teaching it to support Parse/Describe/Sync/Bind/Execute/Sync the same way should remove the need to send Parse twice.

This is against the nature of pgbouncer and against the reason "why we use pgbouncer with exactly this settings". We don't want connection to postgresql to be held by someone outside of a transaction. Holding connection between Prepare and Execute leads to unpredictable behaviour under high load.

It would also be awesome to add a way to deterministically detect pgbouncer (e.g. by having it append a ParameterStatus message on connection), to remove the need for the session argument.

The choice to use pgbouncer or not should be done explicitly by a human. It has impact on performance characteristics, architecture and code of a service.

[aleksey-mashanov]

the crux of the issue is that pgbouncer is hardcoded to only support the libpq behaviour by pinning the Parse/Bind/Execute/Sync sequence to a connection

This is not true. pgbouncer has no hardcoded sequences. Sync is the unit of work delimiter and this declared by PostgreSQL, not pgbouncer:

"At completion of each series of extended-query messages, the frontend should issue a Sync message. This parameterless message causes the backend to close the current transaction if it's not inside a BEGIN/COMMIT transaction block (“close” meaning to commit if no error, or roll back if error). Then a ReadyForQuery response is issued. The purpose of Sync is to provide a resynchronization point for error recovery. When an error is detected while processing any extended-query message, the backend issues ErrorResponse, then reads and discards messages until a Sync is reached, then issues ReadyForQuery and returns to normal message processing."

[elprans]

Uncached statements prepare twice iff session == False

Yes, and this is a problem. There may be multiple reasons why the cache may be disabled, too small, get cleared often, etc. Also, I don't think adding a new connection parameter is the right way to go at all. See below.

This is against the nature of pgbouncer and against the reason "why we use pgbouncer with exactly this settings". We don't want connection to postgresql to be held by someone outside of a transaction. Holding connection between Prepare and Execute leads to unpredictable behaviour under high load.

I was referring strictly to the handling of unnamed statements, where the time between Parse and Execute is predictably small. I agree that named prepared statements, portals and other session-level features are "against the nature of pgbouncer" in transaction mode. I think, however, that a more robust bouncer implementation would reject these requests outright as opposed to failing in an unpredictable fashion.

It would also be awesome to add a way to deterministically detect pgbouncer (e.g. by having it append a ParameterStatus message on connection), to remove the need for the session argument.

The choice to use pgbouncer or not should be done explicitly by a human. It has impact on performance characteristics, architecture and code of a service.

This is not about the choice to use pgbouncer. This is about detecting the capabilities of the server and adjusting accordingly. We do this already to support various PostgreSQL variants and servers speaking its protocol.

pgbouncer is not a transparent proxy by any means, given how it breaks the session semantics, so it should be treated as a PostgreSQL variant that doesn't support prepared statements properly, in which case the statement_cache_size should default to 0. If we are unable to detect pgbouncer, manually setting statement_cache_size to 0 will be required.

pgbouncer has no hardcoded sequences.

If that's, in fact, true, and pgbouncer always waits for Sync as a transaction boundary, then the solution is not to emit Sync after the initial Parse or Parse/Describe.

Sync is the unit of work delimiter and this declared by PostgreSQL, not pgbouncer:

I am aware of that, thanks.

Bottom line, here's what I think should be done to add support for pgbouncer in an acceptable fashion:

1. Decouple the codec configuration cache from the server-side prepared statement cache. statement_cache_size explicitly becomes about the use of server-side prepared statements only.
2. Make it so that in the unnamed statement case the Parse/Describe/Bind/Execute sequence always has exactly one Sync (Parse/Bind/Execute/Sync for the cached codec case, Parse/Describe/Flush/Bind/Execute/Sync for the uncached case).
3. Add detection of pgbouncer (possibly relying on remote help). Set statement_cache_size to 0, when detected (and raise an exception if it is explicitly set to a non-zero value). Lacking detection, and when a prepared statement exception occurs, advise the user to set statement_cache_size to 0 explicitly.

[elprans]

Per discussion above.

[aleksey-mashanov]

Decouple the codec configuration cache from the server-side prepared statement cache. statement_cache_size explicitly becomes about the use of server-side prepared statements only.

Can you explain this idea in more details?
Looks very complicated for me, maybe I missed something.

Parse/Describe/Flush/Bind/Execute/Sync for the uncached case

Flush will not help. This will be one connection to PostgreSQL hold by one client. Application may be non-perfect and can not return control to coroutine connected to database for tenths and even hundreds of milliseconds resulting in pgsql connection hold by pgbouncer for the same time. It is inappropriate for distributed systems.

[elprans]

Can you explain this idea in more details? Looks very complicated for me, maybe I missed something.

It's not complicated. The only change is making statement_cache_size track the number of named statements in the PreparedStatementState cache. That way setting statement_cache_size to 0 will prevent the use of named statements, but will retain the benefit of cached Describe results.

Flush will not help. This will be one connection to PostgreSQL hold by one client. Application may be non-perfect and can not return control to coroutine connected to database for tenths and even hundreds of milliseconds resulting in pgsql connection hold by pgbouncer for the same time. It is inappropriate for distributed systems.

Then you need to fix your application! There is no guarantee that your await query will always run until completion and will not get preempted by another coroutine at an arbitrary point in the protocol flow, regardless of the number of hacks you make. It all depends on what and how much was select()-ed from the sockets on a particular loop iteration, and you can't reliably control that. This is how asyncio works and you have to deal with it. So, please stop making general claims that asyncpg is "inappropriate for distributed systems" when all I'm trying to do is help you improve the status-quo in the first place.

[aleksey-mashanov]

The only change is making statement_cache_size track the number of named statements in the PreparedStatementState cache.

I thought the primary reason of this parameter is to limit memory used by the client. If memory is not an issue then OK.

Then you need to fix your application!

Different systems have different requirements. Small error in one of application request handlers shouldn't crash all the system (by utilizing all pgsql connections), one of worker can get stuck or die, but not the system at all. Web applications are live, they modified couple times a weak and even a day, they should be tolerant to small errors.

There is no guarantee that your await query will always run until completion and will not get preempted by another coroutine at an arbitrary point in the protocol flow, regardless of the number of hacks you make.

I don't really care about python workers, there are many of them, they easily scale. I care about database itself, master is the only and not scale.

please stop making general claims that asyncpg is "inappropriate for distributed systems"

asyncpg has nothing about "distributed systems", and any database client too. It is an architecture of application about it. The only thing a database client should do - not to interfere. Right now asyncpg prevents usage of pgbouncer. So, scale is not possible with current asyncpg version.

I don't really mean that asyncpg is "inappropriate", this word was about Parse/Describe/Flush/Bind/Execute/Sync sequence.

[aleksey-mashanov]

        if (use_cache or named) and self._protocol.session:
            stmt_name = self._get_unique_id('stmt')
        else:
            stmt_name = ''

Looks like statement_cache_size=0 is insufficient. I can change this line to if use_cache and named but this change current behaviour.

[elprans]

I thought the primary reason of this parameter is to limit memory used by the client. If memory is not an issue then OK.

It is about the memory usage, but mostly server-side, as prepared statements can consume a non-trivial amount of memory. The overall size of the client-side cache should obviously remain capped. I don't think we should expose that as a connection argument just yet, let's default to some larger value than the current statement_cache_size default.

Different systems have different requirements.

Yes, and if we were to satisfy all the different requirements of different systems in asyncpg, we would have a mess that wouldn't work for anybody. The goal of asyncpg is to work well with vanilla PostgreSQL, as that is a known common baseline. We are willing to support other PostgreSQL-like systems, but only if that support does not compromise the integrity and maintainability of asyncpg.

Small error in one of application request handlers shouldn't crash all the system (by utilizing all pgsql connections).

Again, what does that have to do with asyncpg? Your misbehaving application would exhaust the pgbouncer pool, or any other limited resource pool equally well. Fix it! Arguably, a system that can go down due to a small error in a single component is badly designed. If you are so worried about connection stalls, then refactor the database access into a standalone, well reviewed and tested service, separate from the application logic.

There is no guarantee that your await query will always run until completion

I don't really care about python workers, there are many of them, they easily scale. I care about database itself, master is the only and not scale.

I think you misunderstood, or deliberately ignored what I wrote. Whatever you do, you cannot guarantee with absolute certainty that the protocol instance will always receive the entire database response in a single loop iteration, and your await query might get preempted while holding the connection. If your system cannot tolerate that, then asyncio is not for you.

Right now asyncpg prevents usage of pgbouncer...

... in the transaction mode. And the approach I suggested would fix that without introducing much complexity and side effects for existing workflows.

So, scale is not possible with current asyncpg version.

You just made that claim again.

I don't really mean that asyncpg is "inappropriate", this word was about Parse/Describe/Flush/Bind/Execute/Sync sequence.

Well, I think that Parse/Describe/Sync + Parse/Bind/Execute/Sync is inappropriate since we would be forcing double Parse on all uncached queries, and there is a quantifiable cost to that, unlike your abstract claims about "distributed systems".

As it stands, I'm against merging your version.

[aleksey-mashanov]

I don't think you understood, or deliberately ignored what I wrote. Whatever you do, you cannot guarantee with absolute certainty that the protocol instance will always receive the entire database response in a single loop iteration, and your await query might get preempted while holding the connection. If your system cannot tolerate that, then asyncio is not for you.

It's OK if asyncpg is talking with pgbouncer. pgbouncer will cache the response and postgresql connection will be released. I don't care how many loop iteration required in python, I only want postgresql connection to be released as soon as possible. Think of pgbouncer in front of postgresql the same way as you think about nginx in front of python backend - it is used to handle slow clients.

If you are so worried about connection stalls, then refactor the database access into a standalone, well reviewed and tested service, separate from the application logic.

I love to keep things simple. A lot of small services which are modified often are harder to maintain.
Anyway a database client should not impact on architecture.

And the approach I suggested would fix that without introducing much complexity and side effects for existing workflows.

This PR has no side effects for existing workflow.

Well, I think that Parse/Describe/Sync + Parse/Bind/Execute/Sync is inappropriate since we would be forcing double Parse on all uncached queries

When connected to pgbouncer in pool_mode=transaction only. Right now even this is impossible.
This PR has no impact on existing workflow. It only adds additional one.

[elprans]

It's OK if asyncpg is talking with pgbouncer. pgbouncer will cache the response and postgresql connection will be released.

Not true in every case. pgbouncer is a buffered proxy, and if the response is larger than its buffer (which is fairly small by default) then it will take multiple network operations to send the response.

This PR has no impact on existing workflow. It only adds additional one...

Even so, it's a flawed implementation, and I'm against adding a new connection parameter for every "workflow" for reasons I outlined in my previous response. Please stop arguing your case by repeating the old arguments. I've heard them and articulated my opinion at length. Adding the necessary pgbouncer support is possible without these complications.

---

Looks like statement_cache_size=0 is insufficient. I can change this line to if use_cache and named but this change current behaviour.

named=True basically means "I want a named prepared statement regardless of the caching", and is used by cursors (which don't work with pgbouncer). I'd rename the argument to force_named and then the check becomes if named_cache or force_named, where named_cache is determined by statement_cache_size, and use_cache is a separate concern determined by local_statement_cache_size > 0.

For simplicity, I would define local_statement_cache_size as:

local_statement_cache_size = statement_cache_size if named_supported else 1000

so that the current behaviour remains the same. We can work on improving the cache logic as a followup.

[aleksey-mashanov]

Not true in every case. pgbouncer is a buffered proxy, and if the response is larger than its buffer (which is fairly small by default) then it will take multiple network operations to send the response.

Yes, you are right. I mean "buffer" not "cache". Buffer size is configurable.

used by cursors (which don't work with pgbouncer)

They work. In a transaction. Exactly the same way as with vanilla PostgreSQL. I only limit them to be unnamed if connected to pgbouncer to prevent cleanup mess.

named=True is used by Connecton::prepare() too. I'd prefer stmt = conn.prepare(...) and then stmt.execute() to work even with pgbouncer inside a transaction and outside.

[elprans]

They work. In a transaction. Exactly the same way as with vanilla PostgreSQL. I only limit them to be unnamed if connected to pgbouncer to prevent cleanup mess.
named=True is used by Connecton::prepare() too. I'd prefer stmt = conn.prepare(...) and then stmt.execute() to work even with pgbouncer inside a transaction and outside.

You cannot put named prepared statements into cache even inside a transaction, unless you clean them up from the cache once the transaction is closed, and this is way too complicated.

[aleksey-mashanov]

You cannot put named prepared statements into cache even inside a transaction, unless you clean them up from the cache once the transaction is closed, and this is way too complicated.

Right. That's why in case of pgbouncer I put unnamed prepare statements into cache and use following check:

            if state.name == '' and (state != self.last_unnamed
                                     or state.need_reprepare):
                query = state.query
                state.need_reprepare = False
                self.last_unnamed = state
            else:
                query = None

[elprans]

Well, that's no longer a prepared statement then, is it? If you need that, you can always subclass the PreparedStatement class and do a Parse in the subclass.

[aleksey-mashanov]

Well, that's no longer a prepared statement then, is it? If you need that, you can always subclass the PreparedStatement class and do a Parse in the subclass.

That's a prepared statement still. But unnamed and with lifetime until next unnamed statement or end of a transaction.

Subclassing is overcomplicated and anyway this feature requires modifications in coreproto.pyx, protocol.pyx and connection.py.

If you don't like session parameter (neither I), then we can add to statement_cache_size special value 'pgbouncer' or something like this to distinguish it from 0.

[elprans]

That's a prepared statement still. But unnamed and with lifetime until next unnamed statement or end of a transaction.

Which makes it essentially useless in a prepared statement sense. The purpose of an explicit prepared statement is to control precisely when it gets prepared and deallocated. Barring these guarantees it can no longer pretend to be a true prepared statement.

Subclassing is overcomplicated and anyway this feature requires modifications in coreproto.pyx, protocol.pyx and connection.py.

In this case subclassing is the only way to go. Changing things around to make it easier is fine, as long as those changes are generic and have no negative impact.

then we can add to statement_cache_size special value 'pgbouncer' or something like this to distinguish it from 0.

Calling it differently does not remove my objection. The magical re-Parse approach is a non-starter.

[aleksey-mashanov]

Barring these guarantees it can no longer pretend to be a true prepared statement.

True or untrue - it is subjective. From the PostgreSQL docs all of them are true.

The magical re-Parse approach is a non-starter.

It is not magical. It is logical and predictible. More then that it already existed in the code, just was moved to a more appropriate place.

[elprans]

True or untrue - it is subjective. From the PostgreSQL docs all of them are true.

Sorry, I don't understand your statement. If I do PREPARE foo AS query, I am guaranteed to execute the same query plan (if a generic plan was chosen) on all subsequent EXECUTE foo calls, and I'm also guaranteed that the server would not waste time parsing and planning the query. This is exactly the promises of conn.prepare(). Your approach would silently break these promises.

Example:

ps = await conn.prepare('complicated query...')
for i in range(100):
    param = await conn.fetchval('simple query', i)
    result = await ps.fetch(param)

I expect the above to either work as expected, or fail loudly. What I don't expect is the above doing silent Parse calls on every iteration wrecking my performance. This is what I was referring to as "magical". pgbouncer in the transaction mode does not support prepared statements properly, so an asyncpg client connecting to it shouldn't either.

[aleksey-mashanov]

"In the extended protocol, the frontend first sends a Parse message, which contains a textual query string, optionally some information about data types of parameter placeholders, and the name of a destination prepared-statement object (an empty string selects the unnamed prepared statement)."
There is nothing about true or untrue. Only named and unnamed.

"Named prepared statements can also be created and accessed at the SQL command level, using PREPARE and EXECUTE." PREPARE SQL command used in PostgreSQL is an additional feature, not compatible with SQL standard. It doesn't support unnamed statements because it is impossible on a high level of the SQL language, but this not imply what unnamed prepared statements - untrue.

I expect the above to either work as expected, or fail loudly. What I don't expect is the above doing silent Parse calls on every iteration wrecking my performance.

This is how database clients in different languages work. If a server side prepared statements are disabled then a client falls back to a client side. This is common, widely used and familiar behaviour. Users experienced with other database clients are ready to this. This behaviour allows to write code able to work optimally with and without pgbouncer.
Anyway someone who uses pgbouncer in pool_mode=transaction (not default) exactly knows what they is doing and why.

[elprans]

This is how database clients in different languages work. If a server side prepared statements are disabled then a client falls back to a client side. This is common, widely used and familiar behaviour.

Where did you get that from?

Are you able to show at least one example of a database driver that silently emulates an explicit call to make a server-side prepared statement? Does libpq PQprepare do that? Does JDBC Connection.prepareStatement? Most drivers don't even expose PREPARE as a method (mostly because they follow some DB API standard, and PREPARE is non-standard).

[aleksey-mashanov]

Does libpq PQprepare do that?

libpq is a low level library. PQprepare is highly tied with PostgreSQL client/server protocol.

Does JDBC Connection.prepareStatement do that?

Yes, 5 times for each query by default: https://jdbc.postgresql.org/documentation/head/server-prepare.html

Are you able to show at least one example of a database driver that silently emulates an explicit call to make a server-side prepared statement?

Wikipedia can (https://en.wikipedia.org/wiki/Prepared_statement):
"A number of programming languages support prepared statements in their standard libraries and will emulate them on the client side even if the underlying DBMS does not support them, including Java's JDBC,[11] Perl's DBI,[12] PHP's PDO [1] and Python's DB-API.[13] Client-side emulation can be faster for queries which are executed only once, by reducing the number of round trips to the server, but is usually slower for queries executed many times. It resists SQL injection attacks equally effectively."

[elprans]

"explicit call to make a server-side prepared statement"

libpq is a low level library. PQprepare is highly tied with PostgreSQL client/server protocol.

This is exactly the point. asyncpg is like libpq. Please read the README.

Yes, 5 times for each query by default: https://jdbc.postgresql.org/documentation/head/server-prepare.html

After which it would attempt to create a server-side prepared statement and fail unpredictably.

Everything else you listed are interfaces implementing generic DB APIs, which is explicitly not the goal of asyncpg.

[aleksey-mashanov]

asyncpg is like libpq.

It is not like libpq. Hidden introspection queries and automagical hidden prepared statements are the most notable differences.

OK, your code your rules. I can live without stmt = conn.prepare(), stmt.execute() and use conn.execute() only. But not without Parse/Describe/Sync + Parse/Bind/Execute/Sync. If you still against it (are you?) then this project can not solve my problem.

[elprans]

I'm not against supporting pgbouncer cleanly, which can be done. I'm against your specific approach. If you are unwilling to compromise, so be it.

[aleksey-mashanov]

I'm not against supporting pgbouncer cleanly, which can be done. I'm against your specific approach. If you are unwilling to compromise, so be it.

You want to support pgbouncer in a fashion what will scale badly with possibility of connections get stuck. Cleanly or not is a second question. At first it just should work.

For anyone who aware of horizontal scaling and who exactly knows why they want to use pgbouncer with pool_mode=transaction fork will live here: https://github.com/my-mail-ru/asyncpg

[elprans]

You want to support pgbouncer in a fashion what will scale badly with possibility of connections get stuck.

You provided zero data to back up this claim and ignored my counter-arguments, so this is just FUD.