On Wed, 11 Nov 2026 18:01:30 -0600, Sami Imseih <[email protected]> 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/
