Deallocate client prepared statements at checkin - #1302
Conversation
pg_dump creates a SQL-level prepared statement, which currently survives checkin, so the next dump landing on the same server connection fails with "prepared statement already exists". Runs against a database with a single server connection, so the reuse is deterministic instead of depending on which connection the pool hands out.
b4b0243 to
060c331
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
7e44d44 to
2c0b9ff
Compare
A client can create prepared statements with SQL (PREPARE ... AS ...). Those belong to its session, but in transaction pooling the server connection goes back into the pool at the end of the transaction, taking them along. The next client that gets it collides on the name. pg_dump hits this every time: it prepares "dumpFunc", so the second dump through the pooler fails with 'prepared statement already exists' — the first one works only because it gets a connection nobody dumped on yet. Treat them like the other session state we already clean up and run DEALLOCATE ALL at checkin. A connection can need this alongside a parameter reset, so cleanup queries are now composed instead of picked from mutually exclusive branches. With the statements dropped, re-reading them from pg_prepared_statements at checkin only ever returned an empty set, so that round trip is gone and the flag it cleared is cleared by the cleanup itself.
2c0b9ff to
ed1c2d2
Compare
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_reset_schema_changed_clears_cache() { |
There was a problem hiding this comment.
I'm confused by this test. You're manually adding schema_stmt to the cache and then clearing it. Why execute a PREPARE statement against the server as well?
|
This looks good to me. Just one question to address around the test and we should be good to merge. |
The test prepared a statement on the server and then put the name into the cache by hand, so the round trip proved nothing: reset_schema_changed() only touches what PgDog holds in memory, and the assertion would have passed without the server ever seeing a statement. A protocol-level Parse populates the cache on its own, which is how the cache is filled outside tests, and matches the DISCARD ALL test next to it.
|
Good catch — the
Replaced both with a protocol-level server
.send(&vec![Parse::named("__pgdog_1", "SELECT 1").into(), Flush.into()].into())
.await
.unwrap();
let msg = server.read().await.unwrap();
assert_eq!(msg.code(), '1');
assert!(!server.prepared_statements.is_empty());
// A schema change invalidates everything we cached for this connection.
server.reset_schema_changed();
assert!(server.prepared_statements.is_empty());
assert_eq!(server.stats().total().prepared_statements, 0);Twelve lines shorter, and it still fails if Pushed as 3dcd0aa. |
Problem
pg_dumpprepares a statement with SQL (PREPARE dumpFunc(pg_catalog.oid) AS ...). In transaction pooling the server connection goes back into the pool at the end of the transaction and takes that statement with it, so the next dump that lands on the same connection fails:The first dump usually works — it gets a connection nobody has dumped on yet — which is what makes this look intermittent. With a single-connection pool it fails on the second dump, every time.
Reproduction:
Fix
Run
DEALLOCATE ALLat checkin when the client prepared statements with SQL — the connection already tracks that insync_prepared. Session-mode clients are unaffected: they keep the connection and already getDISCARD ALLwhen they leave.Cleanup queries are now composed rather than picked from mutually exclusive branches: a connection can need both a parameter reset and a deallocate (
RESET x; PREPARE y ...in one checkout), and the oldelse ifchain silently dropped the second one.Two follow-ons from that:
pg_prepared_statementsat checkin only ever returned an empty set, so that round trip and the method behind it are gone; the flag it cleared is cleared by the cleanup itself.Not touched here:
prepared_syncinpgdog-statsis no longer incremented by anything. Removing it reaches into public structs of that crate, so it seemed better left to a separate change.Testing
integration/python/test_pg_dump.py: three dumps in a row must all succeed, and a statement prepared on a connection that also needs a parameter reset must not outlive its client's checkin. Runs against a database with a single server connection, so the reuse is deterministic. Verified to fail onmain(8 runs out of 8) and pass with this branch.pg_dumpdoes.pg_dump -t <table>both clean, noprepared statement already existsin application logs afterwards.Related: #1298 and #1299 fix the other two ways session state survived checkin (an untracked
set_config, and aRESETrevived by rollback). Same class of bug, independent code paths.