Re: Improve pg_stat_statements scalability

On Wed, 11 Nov 2026 18:01:30 -0600, Sami Imseih <samimseih(dot)pg(at)gmail(dot)com> wrote:
> One of the new tests was missing COLLATE "C", so it
> failed on Mac. Attached v6 fixes that.
>
> --
> Sami

Hello Sami.

Thanks for working on this, there was a lot of room for improvement in
the existing code.

I applied v6 on top of ef1b740441c, each patch builds cleanly on its
own, and the pg_stat_statements, test_custom_stats, regress, isolation,
and recovery suites pass at the tip. A few problems turned up while
trying it out.

Lowering pg_stat_statements.max with a reload can stop all new tracking.
With max = 1000 and about 1000 entries, I reloaded with max = 400. None
of the next 100 new statements were tracked, and dealloc stayed at 0.
IIUC, the check in pgss_store() against twice the maximum returns before
entry_dealloc() is reached, and nothing else evicts, so it stays that
way until a reset or a restart. Master can't get into this state, since
changing max there needs a restart. t/011_eviction.pl lowers max from
200 to 100, but it passes only because eviction has already left the
entry count just under 200.

When query_text_memory fills up in 0004, statements whose texts don't
fit are dropped without any sign of it. With query_text_memory = 1MB
and 400 distinct statements of about 4kB each, 166 were tracked, 234
were dropped, and dealloc stayed at 0. Short texts still fit in the
space left, so it's the long ones that go missing. Nothing evicts
low-usage entries to make room, and while the docs say dealloc counts
these drops, I don't see code that does it (t/013_qtext_memory.pl checks
that dealloc is 0). Would evicting when the allocation fails work here?

> 4/ If pg_stat_statements is removed from shared_preload_libraries,
> pgstat_read_statsfile() finds entries of a kind that is not loaded and
> resets the stats of every kind. That goes for any custom kind, so it
> may be worth skipping the entries of a kind that is no longer loaded.

+1, and I think core needs that before this lands. In my test,
n_dead_tup went from 500 to 0 once the module was removed, which leaves
autovacuum with nothing to go on, and the log called the stats file
corrupted. This is existing behavior for any custom kind, but
pg_stat_statements is the extension people most often add and remove.
IIUC, skipping a kind needs the file to record the size of each entry,
as the serialized data of a kind has no framing of its own.

> 5/ pg_stat_statements uses kind IDs 31 and 32, which I have not
> reserved at [5] yet.

They are the last two custom IDs (PGSTAT_KIND_MAX is 32), so a contrib
module would leave third-party extensions 24 through 30. Have you
considered raising PGSTAT_KIND_MAX, or another way for a module shipped
with core to get its IDs?

Entries are found by a 64-bit hash of (userid, dbid, queryid, toplevel),
and the full key is never compared after the lookup. On master, the
whole key is compared, so two users or two databases can never share an
entry. A collision is very unlikely, but comparing entry->key after the
lookup seems cheap. What do you think?

A long transaction anywhere in the cluster, like pg_restore
--single-transaction, keeps evicted entries allocated until it ends.
Could that push the cluster into the state above where new statements
aren't tracked, with no counter showing it?

Counts reach the shared entry only when a session flushes outside a
transaction, so the work of a long CALL or an idle-in-transaction
session doesn't show up until it ends. On master it shows at the end of
each execution. That's a visible change for anyone watching batch jobs,
and I think the docs should at least say so.

Reading the view flushes the pending stats of every kind, which is why
pg_stat_statements() is now PARALLEL RESTRICTED. Would flushing only
this kind's pending entries let it stay PARALLEL SAFE?

Every execution still takes the shared entry's lock, conditionally, to
add to usage. For a hot statement run by many backends at once, that's
a write to the same cache line on each execution. I haven't measured it,
but a flag that eviction clears and executions set only when it is clear
might keep the hot path out of shared memory. Does that show up in your
benchmarks?

I see you'd like 0003 reviewed on its own while keeping the texts in
memory gets discussed. If 0004 does turn out to be the goal, though,
squashing them would save reviewing the 730 or so lines of 0003 that
0004 removes. I'm betting that's your intention.

0001 adds a CHECK_FOR_INTERRUPTS() to the flush loop, which also runs
from the idle-time flush in PostgresMain, while DoingCommandRead is
still false. Am I right that a cancel arriving just after commit could
then report an ERROR for a statement that already committed? I haven't
reproduced that, and it may belong on the 0001 thread [0].

nitpick: 0001 doesn't add PgStat_StatCustomVarCountsNonTxn and
PgStat_StatCustomVarCountsTxn to typedefs.list, so pgindent misformats
their declarations.

Nice work, I look forward to v7.

-greg

[0] https://commitfest.postgresql.org/patch/6781/