Enforce prepared statements cache limits at runtime, add memory limit - #1311
Enforce prepared statements cache limits at runtime, add memory limit#1311IgorOhrimenko wants to merge 7 commits into
Conversation
The cache had no working limit: prepared_statements_limit was applied to it only on RELOAD and from the admin console, so between those a workload preparing a stream of unique statements grows it without bound. A statement spike of a few million unique queries takes a pooler to gigabytes of RSS, and nothing reclaims that while traffic keeps flowing. Enforce the count limit continuously and add a companion prepared_statements_memory_limit (bytes, 0 = unlimited, also PGDOG_PREPARED_STATEMENTS_MEMORY_LIMIT and settable from the admin console), the same shape as the query cache limits. Only statements no client is holding are evicted, so the cache can still exceed its caps while everything in it is in use — it shrinks the moment statements are released. The byte total is maintained incrementally on insert/remove, so enforcement doesn't rescan the maps; the same number feeds the new prepared_statements_memory_limit gauge next to the existing prepared_statements_memory_used. One behavior change in the admin console: SET prepared_statements_limit TO 0 used to wipe the cache (close_unused treats 0 as "remove everything"); it now means "unlimited", matching the config semantics.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
On the Codecov report: the one uncovered line in the patch is the |
The unused set was a hash set, so eviction order depended on hasher state. A BTreeSet keyed by the statement counter makes it evict the oldest statement first, deterministically, and close_unused inherits the same order. SET prepared_statements_limit TO 0 used to clear the cache and now means unlimited, so log a warning pointing at RESET prepared_statements for operators relying on the old behavior.
- close_unused(0) no longer wipes the whole cache and resets the name counter: it now drops everything not in use, keeps statements clients hold, and never reuses global names. The old reset path could hand a server connection a reused __pgdog_N name pointing at a different query. - RESET prepared_statements passes 0 explicitly, so it clears the cache regardless of the configured limit. With the default (unlimited) limit it was a no-op. - GlobalCache::memory_usage() now returns the same number the memory limit is enforced against, so the prepared_statements_memory_used metric and the budget can't drift apart. - Statement::memory_usage() counts the rewritten Parse; rewrite() and insert_row_description() adjust the byte total and enforce. - remove() clears the unused entry for evict_on_close closes and asserts the two maps stay in sync; enforce() uses pop_first(). - Config doc note: a limit below the working set causes constant re-preparation.
|
A note on the
Same class of timing flake as #1303/#1304/#1305. Bumping |
run_maintenance() passed prepared_statements_limit straight into close_unused(), where 0 now means "drop everything unused" — the exact opposite of the "unlimited" this limit documents. Re-apply the configured limits through configure() instead: 0 flows through over_budget() as unlimited, the memory limit gets the same safety net, and the maintenance tick stays a no-op when runtime enforcement has already done the work. GlobalCache::reset() lost its last production caller when close_unused stopped wiping the cache; keep it for tests only, so the path that rolls the name counter back can't quietly return.
The admin parser accepts RESET PREPARED, not RESET prepared_statements: the warning was advising a command that answers with a syntax error. Verified against a live admin console.
The limit-0 warning quoted a command spelling the admin parser doesn't accept; nothing tied the two together. Put the spelling in one const — ResetPrepared::name(), the warning and the parser test all use it — so the advice can't drift from what actually parses. Also covers RESET PREPARED in the parser tests at all: RESET QUERY_CACHE had a test, this one didn't.
Problem
The global prepared statements cache is enforced by a 1 Hz maintenance task that calls
close_unused(prepared_statements_limit), but three things undermine it:prepared_statements_limitisi64::MAX, so out of the box every statement a client ever released is kept forever.Parse(plus a rewritten copy and aRowDescriptionwhen present).What this PR does not fix, explicitly: statements a client is still holding can never be evicted — dropping one would break the client using it. A workload that keeps millions of unique statements open (we watched ~6M held statements take a pooler to ~7 GiB RSS until the clients disconnected) is out of reach for any unused-only eviction, including this one and the existing maintenance task. Protecting against that needs a per-client cap or similar — happy to discuss as a follow-up; this PR bounds what released statements can accumulate, immediately rather than at the next tick.
Change
Same shape as the query cache limits in #1266:
prepared_statements_limitand the newprepared_statements_memory_limit(bytes,0= unlimited, envPGDOG_PREPARED_STATEMENTS_MEMORY_LIMIT, settable from the admin console) are enforced at the moment the cache grows or a statement is released, not just at the next maintenance tick.unusedis aBTreeSetnow) — and only ever touches statements nobody holds.GlobalCache::memory_usage()returns that same number — theprepared_statements_memory_usedmetric and the budget can't drift apart. Aprepared_statements_memory_limitgauge is exported next to it.Behavior changes
SET prepared_statements_limit TO 0used to wipe the cache; it now means "unlimited" (matching the config semantics) and logs a warning pointing atRESET PREPARED.RESET PREPAREDnow passes0explicitly and clears everything not in use regardless of the configured limit — with the default (unlimited) limit it used to be a no-op.close_unused(0)no longer resets the name counter: global__pgdog_Nnames are never reused, so a server connection holding an old name can't be handed a different query under it.Testing
Unit tests for enforcement (capacity and memory eviction, in-use statements never evicted,
0= unlimited, immediate enforcement on configure, deterministic oldest-first order, release via bothclose()anddecrement(), counter preserved acrossRESET PREPARED) and a byte-accounting invariant test that recounts the total from live entries after a mix of inserts, duplicate inserts,insert_anyway, lateRowDescription,rewrite()(including replacement), closes and evictions. Config env/default tests; JSON schema regenerated.Cost note
Eviction runs under the global cache write lock. The maintenance task now goes through
configure()once a second too, but when the cache is within its limits that's a couple of integer comparisons — there is nothing to evict, because insert/release enforcement already kept it in budget. The one expensive case is lowering a limit (adminSET,RELOAD) over a cache holding millions of released statements: they are all evicted in one pass and cache users wait for the duration. That's a one-off administrative action; chunked eviction can be added if it matters in practice.Docs PR to follow once the shape is agreed on.