Skip to content

Deallocate client prepared statements at checkin - #1302

Open
IgorOhrimenko wants to merge 3 commits into
pgdogdev:mainfrom
IgorOhrimenko:fix-pg-dump-prepared
Open

Deallocate client prepared statements at checkin#1302
IgorOhrimenko wants to merge 3 commits into
pgdogdev:mainfrom
IgorOhrimenko:fix-pg-dump-prepared

Conversation

@IgorOhrimenko

Copy link
Copy Markdown
Contributor

Problem

pg_dump prepares 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:

pg_dump: error: query failed: ERROR:  prepared statement "dumpfunc" already exists

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:

pg_dump -h <pgdog> -p 6432 -U pgdog -d pgdog > /dev/null   # ok
pg_dump -h <pgdog> -p 6432 -U pgdog -d pgdog > /dev/null   # ERROR: prepared statement "dumpfunc" already exists

Fix

Run DEALLOCATE ALL at checkin when the client prepared statements with SQL — the connection already tracks that in sync_prepared. Session-mode clients are unaffected: they keep the connection and already get DISCARD ALL when 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 old else if chain silently dropped the second one.

Two follow-ons from that:

  • With the statements dropped, re-reading pg_prepared_statements at 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.
  • Clearing the cache now also zeroes the prepared-statement stat, which previously only happened on that removed sync path.

Not touched here: prepared_sync in pgdog-stats is 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 on main (8 runs out of 8) and pass with this branch.
  • Unit test updated to the new contract: a client's SQL-prepared statement is deallocated at checkin, and the next client can reuse the same name — which is exactly what pg_dump does.
  • Verified on a live cluster: repeated full dumps and pg_dump -t <table> both clean, no prepared statement already exists in application logs afterwards.

Related: #1298 and #1299 fix the other two ways session state survived checkin (an untracked set_config, and a RESET revived by rollback). Same class of bug, independent code paths.

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

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@IgorOhrimenko
IgorOhrimenko force-pushed the fix-pg-dump-prepared branch 3 times, most recently from 7e44d44 to 2c0b9ff Compare August 3, 2026 07:26
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.
}

#[tokio::test]
async fn test_reset_schema_changed_clears_cache() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@levkk

levkk commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

Good catch — the PREPARE in that test was doing nothing.

reset_schema_changed() only touches what PgDog holds in memory: the flag, the
prepared statements cache and the stat counting them. Nothing in the assertion
involved the server, so the round trip was decoration. The manual
prepared_statements.prepared("schema_stmt") right after it was there only
because a SQL-level PREPARE doesn't populate that cache — which is what this
PR is about, and a confusing thing to lean on in a test that isn't.

Replaced both with a protocol-level Parse, which fills the cache the way it
gets filled outside tests and matches test_discard_all_clears_cache next to it:

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 reset_schema_changed() stops
clearing the cache — I checked by commenting out the clear_prepared_statements()
call and watching it go red.

Pushed as 3dcd0aa.

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

Successfully merging this pull request may close these issues.

2 participants