support unnamed statements

[devsnek]

Refs: https://github.com/sfackler/rust-postgres/issues/1017 Refs: https://github.com/sfackler/rust-postgres/issues/1044

This PR implements unnamed statements when querying the database. This is not a performance optimization. The library still uses a "prepared" statement in order to parse the columns and types of queries, so round trips are not reduced (although we don't need to send an additional query to clean up the named prepared statement, which is nice). The reason for this change is to enable queries against pgbouncer instances which are running in transaction or statement pooling modes. A named prepared statement cannot be used in these modes, because the connection may be swapped from under you between preparing the statement and using it.

I found this helpful in understanding what was going on while I was debugging this problem: https://jpcamara.com/2023/04/12/pgbouncer-is-useful.html#prepared-statements

While writing this I also changed the "unexpected message" errors to help me debug, but I can remove that from this PR if you feel it is out of scope.

[sfackler]

This will break pipelined queries.

[devsnek]

@sfackler Can you elaborate on this? My understanding is the parse/bind/execute is sent as a single unit, and would therefore be unaffected by pipelining?

[sfackler]

Oh wait, the same query is prepared both at preparation time and at each use? I guess that works but it feels like an pretty janky setup, and defeats the purpose of statement preparation.

IMO, if you're running pgbouncer in transaction mode you should just make sure you scope your statements within the transaction that uses them. If you're using it in statement mode, either don't, or deal with simple_query.

[devsnek]

Yeah the prepare flow is only for this library to get a handle on those types that it expects. It is definitely not ideal BUT it is not worse than the current implementation of doing a named prepare for every query. I don't think I agree that simple_query is an acceptable solution as you have to go back to worrying about sql injection. It also doesn't return something that works with the To/FromSql traits, which is a huge usability problem :grimacing:

[sfackler]

It is definitely not ideal BUT it is not worse than the current implementation of doing a named prepare for every query.

It is worse - it means you cannot avoid preparing frequently-executed queries on every execution.

[devsnek]

it means you cannot avoid preparing frequently-executed queries on every execution.

To be clear:

let statement = client.prepare("...").await?;
client.query(statement, &[]).await?;

client.query("...", &[]).await?;

The first example is not changed at all by this PR. The second example behaves almost the identically, except that it uses an unnamed statement instead of a named statement. No additional network calls or round trips are introduced in either case by this PR. This matches how other postgres libraries (including libpq) generally behave.

[devsnek]

@sfackler any chance of this getting merged? I really don't want to have to maintain a fork of this library... If you're still against this for some reason, maybe it could be behind a cargo feature?

[fbjork]

@sfackler We would love to see this one merged too as we're about to launch the Postgres connector at Grafbase.

[janpio]

We at @prisma would also be very interested in this.

Right now, we do a weird dance of wrapping all queries in transactions, and each time deallocating the named prepared statements when talking to a PgBouncer instance in transaction mode - which adds up to 3 additional roundtrips for 1 query and has an obvious very bad effect on our performance.

With unnamed (but still prepared) statements we could probably avoid this.

[pimeys]

It would be extra cool to get this merged... Now, again, needing to use a fork of this crate for a production system.

[devsnek]

@sfackler any word here?

[devsnek]

@sfackler anything? should i just close this? i really don't want to fork...