Re: Improve pg_stat_statements scalability

Поиск
Список
Период
Сортировка
Искать

Re: Improve pg_stat_statements scalability

От:
Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Дата:
Hello.

I only reviewed v3-0001 so far, and the comments below are limited to
that patch. Some of them are higher-level observations rather than
comments on individual code fragments.


+Datum
+pg_stat_report_anytime(PG_FUNCTION_ARGS)

The name pg_stat_report_anytime() feels a bit confusing to me.

While "report" makes sense from an implementation perspective because
the function ultimately reports local statistics to the shared
statistics subsystem, that meaning is not very obvious from the
perspective of a SQL-visible function.

Likewise, I'm not sure that "report_anytime" clearly conveys what the
function actually does. Although this is not intended for general
users, we already have pg_stat_force_next_flush(), and it seems
preferable to follow a similar naming convention.

With that in mind, perhaps something like
pg_stat_flush_anytime_stats() would be more descriptive.


+void
+pgstat_report_anytime_stat(void)

In the previous thread, I understood that there was discussion about
introducing a flush interval of around one second, and that the
direction later shifted toward reusing PGSTAT_MIN_INTERVAL.

While the introduction of a manually triggered API changes when
flushes are initiated, it does not seem to change the need for
limiting the flush frequency itself.

As far as I can see, the current implementation does not apply any
throttling to calls of this API. In theory, a large number of backends
could invoke it frequently and generate a high flush rate. For
example, if 1000 backends were to call it once per second, that would
result in 1000 shared-stats updates per second.

Whether such a usage pattern would occur in practice is a separate
question. However, given that existing pgstat code uses
PGSTAT_MIN_INTERVAL to limit flush frequency, it seems to me that
anytime stats should probably retain a similar restriction.


-		Assert(did_flush || nowait);
+		/*
+		 * When nowait is false we block for the lock, so the only reason a
+		 * flush_pending_cb can legitimately return false is that the entry
+		 * has active transaction state that must not be freed yet (e.g.
+		 * relation stats with trans != NULL).  That situation only arises
+		 * mid-transaction, hence the IsTransactionOrTransactionBlock() check.
+		 */
+		Assert(did_flush || nowait || IsTransactionOrTransactionBlock());

Given the current design, the assertion itself looks reasonable to me,
since we're now allowing flush_pending_cb() to return false during
mid-transaction flushes. However, I find the comment a bit difficult
to reconcile with the actual condition. The comment explains a
specific reason why flush_pending_cb() may legitimately return false,
while the assertion itself only distinguishes between transaction and
non-transaction contexts.

As I understand it, the intent here is to keep the existing
requirement outside a transaction while relaxing it for
mid-transaction flushes. If so, I wonder whether expressing the
assertion as

    Assert(IsTransactionOrTransactionBlock() ||
           (did_flush || nowait));

would make that intent a bit more obvious.

If that's indeed the intent, perhaps the comment could also be phrased
in terms of "the assertion is relaxed during a transaction" rather
than describing a specific reason why flush_pending_cb() may return
false.



 	 * Ignore entries that didn't accumulate any actual counts, such as
-	 * indexes that were opened by the planner but not used.
+	 * indexes that were opened by the planner but not used.  The entry cannot
+	 * be freed if there is active transaction state, since
+	 * AtEOXact_PgStat_Relations will still merge counters into it.
 	 */
 	if (pg_memory_is_all_zeros(&lstats->counts,
 							   sizeof(struct PgStat_TableCounts)))
-		return true;
+		return (lstats->trans == NULL);

I may be missing something, but I think there may be a more
fundamental issue behind this change.

Historically, flushing and freeing an entry were effectively the same
decision because flushing only happened after transaction end. With
anytime flushes, however, those become separate concerns. A callback
may be able to flush everything that is appropriate for the current
flush context, while the caller may still need to keep the entry
around for later transaction-end processing.

That makes it hard to reason about checks such as
pg_memory_is_all_zeros(&lstats->counts, ...). This check still appears
to tell whether the counters themselves are zero, but the proposed
change makes it look as if that is no longer enough to determine whether
the entry is "empty", because entry lifetime is also folded into the
callback result.

I wonder whether it would be cleaner for the caller to make the lifetime
decision, based on the kind of flush being performed, and for the
callback to be told that flush context explicitly. For example, the
callback could be passed whether this is an anytime flush or a
transaction-boundary flush, flush the counters that are appropriate for
that context, and then return whether any counters remain.

That way, the callback would not need to infer entry lifetime from
transaction state, and the return value could keep a simpler meaning.


Regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Improve pg_stat_statements scalability

От:
Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Дата:
Hello,

At Tue, 2 Jun 2026 15:30:40 +0900, Michael Paquier  wrote in 
> Hmm, OK.  One cost in this decision is that it could randomly make the
> tests randomly slower, which is one reason why this patch has been
> added to this thread.

That's a fair point.

If this interface is primarily intended for testing purposes, then I
don't think throttling is particularly important.

On the other hand, if it is expected to be used outside testing as
well, I think some form of throttling would still make sense. In that
case, I think pg_stat_force_next_flush() should probably apply to
anytime flushes as well.

> > I wonder whether it would be cleaner for the caller to make the lifetime
> > decision, based on the kind of flush being performed, and for the
> > callback to be told that flush context explicitly. For example, the
> > callback could be passed whether this is an anytime flush or a
> > transaction-boundary flush, flush the counters that are appropriate for
> > that context, and then return whether any counters remain.
> 
> So you would suggest to extend the flush callback(s) with a context
> value instead of having each flush callback do their own decisions
> depending on if we are in a transaction block or not, right?

Yes, that's roughly what I had in mind.

My concern was that the callback result used to answer a simple
question: whether anything remained after the flush.

The flush context is already known by the caller, so it seems cleaner
to keep that information separate. The callback can simply report
whether anything remains.

That keeps the meaning of the callback result straightforward. The
caller can make decisions based on the flush context, while the
callback only reports the state of the counters themselves.

One alternative would be to introduce a separate callback for anytime
flushes. However, since transaction-boundary flushes ultimately need
to flush everything anyway, it seems to me that passing a context flag
would be sufficient.

> Another question that I would have for you is: do you think that we
> should try to keep pgstat_report_stat() as the sole entry point for
> the flush of the stats?  Or should we try to keep the same approach as
> what this patch is doing with a new routine that loops through the
> flush callbacks?  FWIW, I am kind of finding the approach of having a
> single entry point rather than two as more appealing with the
> long-term picture in mind.  That's just a single take, where we could
> just pgstat_report_stat() an extra argument based on the context where
> it is called, lifting its current !IsTransactionOrTransactionBlock()
> requirement.
> 
> As you are the original author of this area of this code, I'd be
> really interested in your opinion here.

I tend to agree with the single-entry-point approach.

The basic operation is common; the anytime case differs only in the
context in which the flush is requested. The flush-timing logic in
pgstat_report_stat() may become slightly more complicated with anytime
flushes, but after that point I think the existing path should
continue to work with only minor adjustments, by carrying an explicit
context value down to the callbacks.

Regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Improve pg_stat_statements scalability

От:
Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Дата:
Hello.

Thank you for the new version.

 	 * Ignore entries that didn't accumulate any actual counts, such as
-	 * indexes that were opened by the planner but not used.
+	 * indexes that were opened by the planner but not used.  With
+	 * in-transaction flushing an entry may be flushed multiple times, so keep
+	 * it pending if it has active transaction state and commit will merge
+	 * counters into it.
 	 */
 	if (pg_memory_is_all_zeros(&lstats->counts,
 							   sizeof(struct PgStat_TableCounts)))
-		return true;
+		return (lstats->trans == NULL);

One of my previous comments was that this change makes the callback
return value carry two different meanings. Since whether lstats->trans
is NULL is purely part of the calling context and not derived from the
callback's own work, I still think it is cleaner to keep
pgstat_relation_flush_cb() unchanged and let the caller handle that
distinction instead.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Improve pg_stat_statements scalability

От:
Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Дата:
Hello,

At Thu, 2 Jul 2026 12:57:38 -0500, Sami Imseih  wrote in 
> The callback is ultimately responsible for knowing what to flush depending
> on the state ( in transaction --or-- end of transaction )

I agree with that part. The callback certainly needs to know what can
be flushed in the current context.

What I was trying to separate is the callback's flush logic from the
meaning of its return value.

Previously, the callback's return value indicated whether the requested
flush completed, for example, whether it failed to acquire a lock. With
this change, the same value would also indicate that some state was
intentionally excluded from an in-transaction flush. Although both
cases leave the entry pending for a later flush, they represent
different outcomes.

The callback still needs to determine what can be flushed and whether
anything remains. My point is only that intentionally retaining
transactional state due to the flush context should not be represented
as a failure of the callback's own work. I think it would be cleaner
for the caller to combine the callback's result with the flush context
when deciding whether to remove the entry from the pending list.

Similarly, the context flag is not intended to prevent every developer
mistake. Rather, it makes the changed callback contract explicit, so
extensions implementing custom stats kinds are forced to revisit their
callbacks instead of silently assuming the previous semantics.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Improve pg_stat_statements scalability

От:
Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Дата:
Hello,

I'm a bit concerned that this exposes some of the underlying dshash
implementation details to stats kinds. I'd prefer to keep those
details encapsulated if possible. That said, considering the
performance implications, I can understand the direction taken by this
series.

0003:
> +bool
> +pgstat_drop_current(PgStatShared_HashEntry *shent, dshash_seq_status *hstat)

However, pgstat_drop_current() seems too tightly coupled to the state
of the underlying dshash scan. shent must be the current entry of
exactly the scan represented by hstat, but that relationship is not
expressed by the API.

As far as I can see, the function only combines two operations:
releasing a local entry reference, if any, and calling
pgstat_drop_entry_internal() for the current scan entry. Would it be
better to keep these as separate operations, perhaps by adding a small
helper for releasing the local entry reference, rather than combining
them into pgstat_drop_current()?

> +bool
> +pgstat_has_pending(PgStatShared_HashEntry *shent)

I also wonder if this should be added at this point.

The name is rather vague, and the function is not used anywhere in
this series. Unless there is an immediate caller, I'd be inclined to
leave it out for now and add it later if a concrete need arises.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Improve pg_stat_statements scalability

От:
Michael Paquier <michael@paquier.xyz>
Дата:
On Mon, Jun 01, 2026 at 01:58:58PM +0900, Kyotaro Horiguchi wrote:
> As far as I can see, the current implementation does not apply any
> throttling to calls of this API. In theory, a large number of backends
> could invoke it frequently and generate a high flush rate. For
> example, if 1000 backends were to call it once per second, that would
> result in 1000 shared-stats updates per second.
> 
> Whether such a usage pattern would occur in practice is a separate
> question. However, given that existing pgstat code uses
> PGSTAT_MIN_INTERVAL to limit flush frequency, it seems to me that
> anytime stats should probably retain a similar restriction.

Hmm, OK.  One cost in this decision is that it could randomly make the
tests randomly slower, which is one reason why this patch has been
added to this thread.

> Historically, flushing and freeing an entry were effectively the same
> decision because flushing only happened after transaction end. With
> anytime flushes, however, those become separate concerns. A callback
> may be able to flush everything that is appropriate for the current
> flush context, while the caller may still need to keep the entry
> around for later transaction-end processing.

Noted.

> That makes it hard to reason about checks such as
> pg_memory_is_all_zeros(&lstats->counts, ...). This check still appears
> to tell whether the counters themselves are zero, but the proposed
> change makes it look as if that is no longer enough to determine whether
> the entry is "empty", because entry lifetime is also folded into the
> callback result.
> 
> I wonder whether it would be cleaner for the caller to make the lifetime
> decision, based on the kind of flush being performed, and for the
> callback to be told that flush context explicitly. For example, the
> callback could be passed whether this is an anytime flush or a
> transaction-boundary flush, flush the counters that are appropriate for
> that context, and then return whether any counters remain.

So you would suggest to extend the flush callback(s) with a context
value instead of having each flush callback do their own decisions
depending on if we are in a transaction block or not, right?

Another question that I would have for you is: do you think that we
should try to keep pgstat_report_stat() as the sole entry point for
the flush of the stats?  Or should we try to keep the same approach as
what this patch is doing with a new routine that loops through the
flush callbacks?  FWIW, I am kind of finding the approach of having a
single entry point rather than two as more appealing with the
long-term picture in mind.  That's just a single take, where we could
just pgstat_report_stat() an extra argument based on the context where
it is called, lifting its current !IsTransactionOrTransactionBlock()
requirement.

As you are the original author of this area of this code, I'd be
really interested in your opinion here.
--
Michael

Re: Improve pg_stat_statements scalability

От:
Julien Rouhaud <rjuju123@gmail.com>
Дата:
Hi,

On Fri, May 22, 2026 at 06:15:00PM -0700, Lukas Fittl wrote:
>
> For archive's sake and others reading along, we had a productive
> discussion about this today at PGConf.Dev.
>
> I've created a new wiki page combining the prior 2025 discussion, and
> notes from today:
>
> https://wiki.postgresql.org/wiki/Scalability_of_pg_stat_statements

Thanks a lot Lukas!

Just a small clarification, I wasn't asking for a way to remember the last time
a query was executed (although I think it's a very good thing to have), but
remembering the time each query text was saved.

In a reasonable system (that is a system where the number of entries doesn't
grow much more than pg_stat_statements.max, ie, you can actually use the
current version pg_stat_statements) you will likely get a lot of entries even
if you filter out the entries that didn't get executed in the last X minutes.
However, the vast majority of them should not be new queries.  So if you
maintain an external system that snapshot pg_stat_statements once in a while
you only care about the (possibly) missing query texts, which should be a very
small fraction of all the records you otherwise need.

Re: Improve pg_stat_statements scalability

От:
Michael Paquier <michael@paquier.xyz>
Дата:
On Wed, Jun 03, 2026 at 05:10:39PM +0900, Kyotaro Horiguchi wrote:
> One alternative would be to introduce a separate callback for anytime
> flushes. However, since transaction-boundary flushes ultimately need
> to flush everything anyway, it seems to me that passing a context flag
> would be sufficient.

Nah.  I am not really in favor of an extra callback.  I feel that this
could lead to more mistakes when implementing new stats kinds, so
doing your approach of using a value based on the context of the flush
works for me.

> I tend to agree with the single-entry-point approach.
> 
> The basic operation is common; the anytime case differs only in the
> context in which the flush is requested. The flush-timing logic in
> pgstat_report_stat() may become slightly more complicated with anytime
> flushes, but after that point I think the existing path should
> continue to work with only minor adjustments, by carrying an explicit
> context value down to the callbacks.

Yeah, same feeling here.  That would be the way I'd use to approach
this design, and to aim for one single entry point for all the stats
reports and the flushes, for all the contexts.
--
Michael

Re: Improve pg_stat_statements scalability

От:
Michael Paquier <michael@paquier.xyz>
Дата:
On Fri, Jun 12, 2026 at 06:21:42PM -0500, Sami Imseih wrote:
> I have implemented an approach, which I will share in the next
> revision, where every stats kind is written to its own dedicated
> dshash rather than into the common one. This is a new
> configuration in PgStat_KindInfo where an extension (or even a
> core stats kind) can request to be placed in a dedicated hash,
> else in the common hash.
> 
> I like this better, especially for kinds with unique access
> patterns like pg_stat_statements, where the view function and
> eviction need to iterate over all their own entries frequently.
> A dedicated hash eliminates the full-scan problem without a
> secondary coordination dshash.

I'm finding this alternative quite appealing, indeed.  For PGSS, I was
wondering about how it would be possible to avoid a secondary hash,
and I did not consider an approach where we could bypass the main one.
--
Michael

Re: Improve pg_stat_statements scalability

От:
Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Дата:
Hi,

On Fri, Jun 26, 2026 at 05:33:28PM -0500, Sami Imseih wrote:
> Hi,
> 
> Sorry for the long silence, but I have been spending time on
> this and would like to share my updates.
> 
> > > On Wed, Jun 03, 2026 at 05:10:39PM +0900, Kyotaro Horiguchi wrote:
> > > > One alternative would be to introduce a separate callback for anytime
> > > > flushes. However, since transaction-boundary flushes ultimately need
> > > > to flush everything anyway, it seems to me that passing a context flag
> > > > would be sufficient.
> > >
> > > Nah.  I am not really in favor of an extra callback.  I feel that this
> > > could lead to more mistakes when implementing new stats kinds, so
> > > doing your approach of using a value based on the context of the flush
> > > works for me.
> 
> > I will proceed with this approach.
> 
> After further thought, I don't think passing a context flag to the
> callback is necessary here.

I think this is needed so that custom stats extensions know that it exists (as
they would get a compilation errors and would need to make use of it).

Indeed, if we don't add a flag then it's easy for a custom stats kind to 
crash on assert-enabled build or produces weird behavior on non-assert enabled
build. All it has to do is, for example, call GetCurrentTransactionStopTimestamp()
at flush time, say:

static bool
my_extension_flush_pending_cb(PgStat_EntryRef *entry_ref, bool nowait)
{
	....
    PgStatShared_myextensiontEntry *shared;
	....
    shared->last_flush_time = GetCurrentTransactionStopTimestamp();
    ...
}

Then:

BEGIN;
do stuff that would produce the extension to flush stats;
SELECT pg_stat_force_next_flush();

Would:

TRAP: failed Assert("s->state == TRANS_DEFAULT || s->state == TRANS_COMMIT || s->state == TRANS_ABORT || s->state == TRANS_PREPARE"), File: "../src/backend/access/transam/xact.c", Line: 901, PID: 1111147
postgres: postgres postgres [local] SELECT(ExceptionalCondition+0xa4)[0x646bb1bbf2d4]
postgres: postgres postgres [local] SELECT(GetCurrentTransactionStopTimestamp+0x53)[0x646bb1546683]

By adding a flag, extensions owner would be aware that a change is needed.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Improve pg_stat_statements scalability

От:
Michael Paquier <michael@paquier.xyz>
Дата:
On Thu, Jul 16, 2026 at 01:22:40PM +0900, Kyotaro Horiguchi wrote:
> Similarly, the context flag is not intended to prevent every developer
> mistake. Rather, it makes the changed callback contract explicit, so
> extensions implementing custom stats kinds are forced to revisit their
> callbacks instead of silently assuming the previous semantics.

+1.
--
Michael

Re: Improve pg_stat_statements scalability

От:
Sami Imseih <samimseih@gmail.com>
Дата:
> > Now, as far as eviction goes. I have spent time with the help of
> > my benchmarking suite [1] looking into various strategies. My main
> > finding is that performing parallel evictions as discussed and
> > proposed in v3 will not work. In the high churn case, we will
> > not be able to put enough back pressure to keep the growth
> > of the dshash in check.
> >
> > So, the idea that I have now is what I am attaching in 0004.
> >
> > The important design points are:
> >
> > 1. We need to hard-cap growth of the dshash, so we cannot avoid
> > skipping new entries if we hit the cap. Right now the hard-cap
> > is pgss_max * 2.
> >
> > 2. We need to give new entries time to live before they become
> > eviction candidates. This helps in low-churn-but-some-churn cases.
> >
> > 3. Least-recently-used will be the eviction strategy, with a
> > threshold to keep new entries alive long enough to build up
> > their popularity before becoming candidates.

One subtle but important point is that eviction decisions are
driven by a new atomic counter that tracks live entries, which
is incremented when a new entry is created and decremented when
and an entry is dropped by eviction. This is a counter that
pg_stat_statements defines. The reason being is the core
pgstat_get_entry_count() is the number of entries in the dshash,
which also includes dropped-but-still-referenced by other backend
entries. So this core counter cannot be used to drive when an eviction
occurs, but it's used to set a hard cap on the growth of the dshash.

--
Sami Imseih
Amazon Web Services (AWS)

Re: Improve pg_stat_statements scalability

От:
Sami Imseih <samimseih@gmail.com>
Дата:
> > As far as I can see, the current implementation does not apply any
> > throttling to calls of this API. In theory, a large number of backends
> > could invoke it frequently and generate a high flush rate. For
> > example, if 1000 backends were to call it once per second, that would
> > result in 1000 shared-stats updates per second.
> >
> > Whether such a usage pattern would occur in practice is a separate
> > question. However, given that existing pgstat code uses
> > PGSTAT_MIN_INTERVAL to limit flush frequency, it seems to me that
> > anytime stats should probably retain a similar restriction.
>
> Hmm, OK.  One cost in this decision is that it could randomly make the
> tests randomly slower, which is one reason why this patch has been
> added to this thread.

The throttling does make the tests longer, but we can do similar to what
was done in f1e251be80a, and disable PGSTAT_MIN_INTERVAL if
injection points are enabled.

-       /* 50 tries with 100ms sleep between tries makes 5 sec total wait */
-       for (tries = 0; tries < 50; tries++)
+       /*
+        * Retry up to 50 times with 100ms between attempts (max 5s
total). Can be
+        * reduced to 3 attempts (max 0.3s total) to speed up tests.
+        */
+       int                     ntries = 50;
+
+#ifdef USE_INJECTION_POINTS
+       if (IS_INJECTION_POINT_ATTACHED("procarray-reduce-count"))
+               ntries = 3;
+#endif
+

--
Sami Imseih
Amazon Web Services (AWS)

Re: Improve pg_stat_statements scalability

От:
Sami Imseih <samimseih@gmail.com>
Дата:
Hi,

Sorry for the delay, and thank you for the comments!

> On Wed, Jun 03, 2026 at 05:10:39PM +0900, Kyotaro Horiguchi wrote:
> > One alternative would be to introduce a separate callback for anytime
> > flushes. However, since transaction-boundary flushes ultimately need
> > to flush everything anyway, it seems to me that passing a context flag
> > would be sufficient.
>
> Nah.  I am not really in favor of an extra callback.  I feel that this
> could lead to more mistakes when implementing new stats kinds, so
> doing your approach of using a value based on the context of the flush
> works for me.

I will proceed with this approach.

> > I tend to agree with the single-entry-point approach.
> >
> > The basic operation is common; the anytime case differs only in the
> > context in which the flush is requested. The flush-timing logic in
> > pgstat_report_stat() may become slightly more complicated with anytime
> > flushes, but after that point I think the existing path should
> > continue to work with only minor adjustments, by carrying an explicit
> > context value down to the callbacks.
>
> Yeah, same feeling here.  That would be the way I'd use to approach
> this design, and to aim for one single entry point for all the stats
> reports and the flushes, for all the contexts.

We can keep the scope of the work for now limited to the testing
infrastructure and allow pg_stat_force_next_flush() to call
pgstat_report_stat() while within a transaction. This will not require
throttling.

> 0004: pg_stat_statements: modernize entry storage with pgstat kind

> The main patch. Replaces ShmemInitHash with dshash via DSM registry,

v3-0004 introduces a secondary dshash meant to track the query
dsa_pointer, but its larger purpose is to avoid having
pg_stat_statements_internal or eviction do a full scan on the
core dshash to look up its own entries; which means filtering
out entries for other kinds.

After discussing this approach with several people at recent
conferences (Lukas Fittl and Robert Haas), I think we should do
something more robust in the core stats system instead.

I have implemented an approach, which I will share in the next
revision, where every stats kind is written to its own dedicated
dshash rather than into the common one. This is a new
configuration in PgStat_KindInfo where an extension (or even a
core stats kind) can request to be placed in a dedicated hash,
else in the common hash.

I like this better, especially for kinds with unique access
patterns like pg_stat_statements, where the view function and
eviction need to iterate over all their own entries frequently.
A dedicated hash eliminates the full-scan problem without a
secondary coordination dshash.

The implementation is straightforward and does not require
significant API changes.

--
Sami Imseih
Amazon Web Services (AWS)

Re: Improve pg_stat_statements scalability

От:
Sami Imseih <samimseih@gmail.com>
Дата:
> I agree with that part. The callback certainly needs to know what can
> be flushed in the current context.
>
> What I was trying to separate is the callback's flush logic from the
> meaning of its return value.
>
> Previously, the callback's return value indicated whether the requested
> flush completed, for example, whether it failed to acquire a lock. With
> this change, the same value would also indicate that some state was
> intentionally excluded from an in-transaction flush. Although both
> cases leave the entry pending for a later flush, they represent
> different outcomes.
>
> The callback still needs to determine what can be flushed and whether
> anything remains. My point is only that intentionally retaining
> transactional state due to the flush context should not be represented
> as a failure of the callback's own work. I think it would be cleaner
> for the caller to combine the callback's result with the flush context
> when deciding whether to remove the entry from the pending list.
>
> Similarly, the context flag is not intended to prevent every developer
> mistake. Rather, it makes the changed callback contract explicit, so
> extensions implementing custom stats kinds are forced to revisit their
> callbacks instead of silently assuming the previous semantics.

I think there are 2 different asks, so let me see if my understanding
is correct:

1/ Explicit callback contract:

Add a "bool xact_boundary" which tells the callback whether it is
safe to flush transactional counters.  When true (transaction end),
the callback flushes everything.  When false (mid-transaction), the
callback flushes only non-transactional counters unless the entry
has no active transactional state. So the new _cb will be:

```
bool            (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait,
bool xact_boundary);
```

Currently, there is only one caller now which is
pgstat_flush_pending_entries() and it will
call the _cb like this:

```
flush_result = kind_info->flush_pending_cb(entry_ref, nowait,
!IsTransactionOrTransactionBlock());
```

But in theory this _cb can be invoked by other callers.

Inside pgstat_relation_flush_cb(), the following will drive if we should flush
in-transaction safe counters or not:

```
flush_txn = (xact_boundary || lstats->trans == NULL);
```

2/ The return value of the callbacks is now an enum rather than a bool.

Previously it returned true (flush completed) or false (could not complete).
The issue raised with my earlier proposal is that "false" carried two different
meanings; lock contention or deferred transactional boundary stats.

The new enum makes the result explicit:

```
PGSTAT_FLUSH_DONE           - fully flushed, remove from pending list
PGSTAT_FLUSH_LOCK_CONFLICT  - could not acquire lock (nowait)
PGSTAT_FLUSH_PARTIAL        - partial flush, transaction boundary stats deferred
```

As it stands now, FLUSH_LOCK_CONFLICT and FLUSH_PARTIAL require the same
action (keep pending, retry later), but at least it is explicit why we
are retrying.  This also forces extension authors to choose the correct
return value rather than blindly returning true/false.

Does this match what you had in mind?

--
Sami Imseih
Amazon Web Services (AWS)

Re: Improve pg_stat_statements scalability

От:
Zsolt Parragi <zsolt.parragi@percona.com>
Дата:
Hello!

I noticed that the patch has some issues with minmax_only, and these
differences seem to be unintended to me:

1. minmax only resets updates mean/sum_var_time (but then when it
calculates them again, it also uses the data before the minmax reset)

+		/*
+		 * Reset only min/max timing values and update minmax_stats_since.
+		 * Iterate matching entries in the dshash, reset the corresponding
+		 * pgstat counters' min/max fields.
+		 */
...
+						shared->counters.mean_time[kind] = 0;
+						shared->counters.sum_var_time[kind] = 0;

2. min_time doesn't recover after a minmax reset:

+		if (pending->min_time[kind] < shared->min_time[kind])
+			shared->min_time[kind] = pending->min_time[kind];

As it is only updated if it's smaller, but it is 0 after it, and
because the call count remains > 0 it doesn't get initialized with the
current value.



+		if ((int) pg_atomic_read_u64(&pgss_shared->nentries) <= pgss_max *
(100 - USAGE_DEALLOC_PERCENT) / 100)
+			break;

isn't there a possible overflow here? The similar deleted check had
explicit uint64 casts for pgss_max multiplication.

-	/*
-	 * Don't proceed if file does not exceed 512 bytes per possible entry.
-	 */
-	if ((uint64) extent < (uint64) 512 * pgss_max)
-		return false;

Re: Improve pg_stat_statements scalability

От:
Lukas Fittl <lukas@fittl.com>
Дата:
On Mon, May 11, 2026 at 4:54 PM Sami Imseih  wrote:
> pg_stat_statements has well-known scaling problems under
> high concurrency. This patch series is an initial proposal
> for how to $SUBJECT.

Agreed. Let's fix this for Postgres 20.

Thank you for putting together initial patches!

>
> The three scaling problems:
>
> 1. Per-entry SpinLock contention. Every counter update acquires
> a SpinLock on the entry. Hot queries executed across many
> backends contend on the same lock, and the hold time grows as
> we add more counters to the struct. Higher core counts mean
> more CPUs contending for the same spinlock, making the problem
> worse on modern hardware. See this complaint here [3].
>
> 2. Deallocation under exclusive LWLock. The hash table is
> fixed-size, bounded by pg_stat_statements.max. When full, a
> single backend performs least-frequently-used eviction of
> the bottom 5% while holding an exclusive LWLock, blocking all
> other backends from reading or writing pg_stat_statements.
> This happens inline during query execution, and workloads with
> many unique queries trigger it frequently. Making matters
> worse, pg_stat_statements.max is a static GUC, requiring a
> full server restart to change, and that is disruptive in
> production systems.
>
> 3. Query text file bloat and GC. Deallocated entries leave dead
> text in the external file. When the file grows to 2x the size
> of live text, GC rewrites the entire file under exclusive lock.
> Disk I/O under exclusive lock is the worst combination, and
> frequent deallocations (problem #2) trigger more GC.

Yep, agreed on these problems.

>
> Using the pgstat, "statistics collector", system to improve
> pg_stat_statements scalability has been discussed previously
> [4]. It writes stats locally first and flushes periodically
> to shared memory, avoiding spinlock contention on the write
> path. Also, the storage is a dshash (partitioned hash table),
> which reduces contention on lookups and allows dynamic resizing
> without restart. The prerequisite work to make this usable by
> extensions has been building over two release cycles:
>
> - PG18: Pluggable cumulative statistics API
> (pgstat_register_kind) [7949d959]
>
> - PG19: Serialization callbacks (to_serialized_data,
> from_serialized_data, finish) for custom stats kinds
> [4ba012a8ed9]
>
> This patch builds on the prerequisite work to address the
> scalability issues mentioned above.
>
> The series consists of two patches:
>
> ---
>
> [0001] pgstat: add pgstat_flush_pending() and pg_stat_flush_pending(pid)
>
> Adds infrastructure for flushing pending statistics to shared
> memory on demand. pgstat_flush_pending() flushes all pending
> entries in the calling backend immediately. Unlike
> pgstat_report_stat(), it can be called mid-transaction, making
> it suitable for view functions that need fresh shared stats
> before the transaction ends.
>
> pg_stat_flush_pending(pid) is the SQL-callable interface to
> the same function.
>
> This patch is related to the discussion in [1] about flushing
> stats within running transactions. The pg_stat_statements
> modernization provides a good example where this is useful.
> At least for the pg_stat_statements tests, we need a way to
> flush statistics within a transaction. I plan to spin this off
> to a dedicated thread, but include it here for now so this
> patchset can be tested.

I noticed the following in the 0001 patch (to be clear, looking at the
v2 version you posted later), which I wasn't sure about:

> diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
> index b2ca28f83ba..848687a9f7e 100644
> --- a/src/backend/utils/activity/pgstat_relation.c
> +++ b/src/backend/utils/activity/pgstat_relation.c
> @@ -828,64 +828,76 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
>
> ...
>         if (t > tabentry->lastscan)
>             tabentry->lastscan = t;
>     }
>     tabentry->tuples_returned += lstats->counts.tuples_returned;
>     tabentry->tuples_fetched += lstats->counts.tuples_fetched;
> -    tabentry->tuples_inserted += lstats->counts.tuples_inserted;
> -    tabentry->tuples_updated += lstats->counts.tuples_updated;
> -    tabentry->tuples_deleted += lstats->counts.tuples_deleted;
>     tabentry->tuples_hot_updated += lstats->counts.tuples_hot_updated;
>     tabentry->tuples_newpage_updated += lstats->counts.tuples_newpage_updated;
> +    tabentry->blocks_fetched += lstats->counts.blocks_fetched;
> +    tabentry->blocks_hit += lstats->counts.blocks_hit;

If I follow correctly, you're moving tuples_(inserted|updated|deleted)
here to special case them so they respect transaction rollbacks, but
you don't do that for tuples_hot_updated and tuples_newpage_updated.
Shouldn't those also be ignored if a transaction rolls back?

>
> [0002] pg_stat_statements: modernize entry storage with
> pgstat kind
>
> This is the main patch. It replaces the private shared-memory
> hash table with the pgstat subsystem's dshash (registered as a
> custom stats kind).

I've taken a first pass at this, and noticed an issue (again, in the
v2 patch for clarity):

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c
b/contrib/pg_stat_statements/pg_stat_statements.c
> index 92315627916..0e6e65e3e51 100644
> --- a/contrib/pg_stat_statements/pg_stat_statements.c
> +++ b/contrib/pg_stat_statements/pg_stat_statements.c
> ...
> @@ -1254,9 +1066,62 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
>      }
>  }
>
> ...
> +/*
> + * Store query text into a shared entry via the external text file.
> + *
> + * Caller must hold the entry lock.  Does nothing if text is already present.
> + */
> +static void
> +pgss_store_query_text(PgStatShared_Pgss *shared_entry,
> +                      const char *query, int query_len, int encoding)
> +{
> +    Size        query_offset;
> +    int            gc_count;
> +
> +    if (shared_entry->query_len >= 0)
> +        return;
> +
> +    LWLockAcquire(&pgss->lock.lock, LW_SHARED);
> +    if (qtext_store(query, query_len, &query_offset, &gc_count))
> +    {
> +        shared_entry->query_offset = query_offset;
> +        shared_entry->query_len = query_len;
> +        shared_entry->encoding = encoding;
> +    }
> +    LWLockRelease(&pgss->lock.lock);
> +}
> +
>
> ...
>      /*
> @@ -1313,192 +1180,171 @@ pgss_store(const char *query, int64 queryId,
>      key.queryid = queryId;
>      key.toplevel = (nesting_level == 0);
>
> -    /* Lookup the hash table entry with shared lock. */
> -    LWLockAcquire(&pgss->lock.lock, LW_SHARED);
> -
> -    entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_FIND, NULL);
> -
> -    /* Create new entry, if not present */
> -    if (!entry)
> +    /*
> +     * If jstate is set, create the shared entry and store normalized query
> +     * text.  Don't increment counters; entries with zero calls are not
> +     * returned by pg_stat_statements_internal().
> +     */
> +    if (jstate)
>      {
> -        Size        query_offset;
> -        int            gc_count;
> -        bool        stored;
> -        bool        do_gc;
> +        const char *store_query;
>
> -        /*
> -         * Create a new, normalized query string if caller asked.  We don't
> -         * need to hold the lock while doing this work.  (Note: in any case,
> -         * it's possible that someone else creates a duplicate hashtable entry
> -         * in the interval where we don't hold the lock below.  That case is
> -         * handled by entry_alloc.)
> -         */
> -        if (jstate)
> ...
>
> -        /* Append new query text to file with only shared lock held */
> -        stored = qtext_store(norm_query ? norm_query : query, query_len,
> -                             &query_offset, &gc_count);
> -
> -        /*
> -         * Determine whether we need to garbage collect external query texts
> -         * while the shared lock is still held.  This micro-optimization
> -         * avoids taking the time to decide this while holding exclusive lock.
> -         */
> -        do_gc = need_gc_qtexts();
> -
> -        /* Need exclusive lock to make a new hashtable entry - promote */
> -        LWLockRelease(&pgss->lock.lock);
> -        LWLockAcquire(&pgss->lock.lock, LW_EXCLUSIVE);
> +        norm_query = generate_normalized_query(jstate, query,
> +                                               query_location,
> +                                               &query_len);
> +        store_query = norm_query ? norm_query : query;
>
> -        /*
> -         * A garbage collection may have occurred while we weren't holding the
> -         * lock.  In the unlikely event that this happens, the query text we
> -         * stored above will have been garbage collected, so write it again.
> -         * This should be infrequent enough that doing it while holding
> -         * exclusive lock isn't a performance problem.
> -         */
> -        if (!stored || pgss->gc_count != gc_count)
> -            stored = qtext_store(norm_query ? norm_query : query, query_len,
> -                                 &query_offset, NULL);
> +        entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_PGSS,
> +                                                key.dbid,
> +                                                pgss_objid(&key),
> +                                                true);
> +        if (!entry_ref)
> +        {
> +            if (norm_query)
> +                pfree(norm_query);
> +            return;
> +        }
>
> -        /* If we failed to write to the text file, give up */
> -        if (!stored)
> -            goto done;
> +        shared_entry = (PgStatShared_Pgss *) entry_ref->shared_stats;
> +        pgss_entry_init(shared_entry, &key, encoding);
> +        pgss_store_query_text(shared_entry, store_query, query_len, encoding);

It appears you've moved the equivalent of the "if (!entry)" check into
the pgss_store_query_text function, and we now unconditionally call
generate_normalized_query.

Is there a reason we couldn't just defer that work until we've
confirmed we actually need to save the query text?

>
> The pgstats hash key has an objid:
>
> typedef struct PgStat_HashKey
> {
>     PgStat_Kind kind;           /* statistics entry kind */
>     Oid         dboid;          /* database ID. InvalidOid for shared
> objects. */
>     uint64      objid;          /* object ID (table, function, etc.), or
>                                  * identifier. */
> } PgStat_HashKey;
>
> So the entry objid is computed by combining hashes from userid, queryid and
> toplevel.

Makes sense.

>
> Note that because of 0001 there are no changes required to the
> existing regression tests. We may need to add some new tests,
> but I have not thought too much about that yet.
>
> Key design changes:
>
> - No SpinLock on the execution path. Counters accumulate
> per-backend and merge into shared memory on flush. Stddev
> and related fields use Welford's algorithm as before, but since the stats
> are first updated locally, we need
> a way to merge the stats on flush, so we use Chan's parallel
> algorithm for this purpose [5].
>
> - No fixed shared memory allocation. Entries live in the pgstat
> dshash, which grows dynamically. pg_stat_statements.max
> becomes PGC_SIGHUP, and therefore can be changed dynamically.

Big +1 on these first two design changes - its clear that we get a
benefit here, and its nice to simplify/streamline the code as well.

> - Throttled inline eviction. When entry count reaches
> pgss_max, a backend attempts eviction using a conditional
> lock and a shared timestamp that ensures at most one eviction
> cycle per 10 seconds. Other backends simply skip entry
> creation without blocking. This is acceptable because the
> current upstream behavior already suffers from the same data
> loss under heavy churn. Newly created entries are immediately
> candidates for eviction and are frequently removed in the
> next deallocation cycle.

I feel this is problematic as presented (looking at pgss_maybe_evict
in v2), because even on a somewhat steady workload you have a high
risk of losing the 5001st entry (if max is 5000).

What if we instead trigger deallocation early, e.g. at 90% of entries
being full? I think this could also go well with a background worker
doing the work.

> The throttled approach makes this
> trade-off explicit and avoids the cost of blocking all backends
> behind an exclusive lock to create space for entries that may
> just be removed shortly. See benchmark results below for
> measuring the retention of "hot" entries.
>
> The aging decay mechanism remains in place, but the sticky
> entries are no longer needed for this patch, and we simply
> don't evict calls == 0.
>
> A background worker for eviction was also considered. The
> current inline approach was chosen because it avoids the
> added complexity while still preventing other backends from
> blocking. If a background worker has more merit, I am open
> to that discussion.

I think a background worker might simplify things around deallocation
and query text GC handling - we have precedent that contrib extensions
utilize background workers, e.g. pg_prewarm, pg_stash_advice, etc.

> - Query texts in DSA memory with disk fallback. A new GUC
> pg_stat_statements.query_text_memory (default 64 MB) controls
> DSA shared memory for query text storage. When enabled and
> not exhausted, new texts are stored in DSA instead of the
> external file, eliminating file I/O on the read path. If DSA
> is disabled (set to 0) or full, texts fall back to the
> existing file-based storage. Entry eviction and reset properly
> free DSA-allocated texts, and GC of the text file skips
> DSA-backed entries.

I feel like this design introduces a performance cliff at the
in-memory => disk boundary that will be surprising and hard to
understand. We had previously discussed doing an in-memory only
design, but I could see that being harder to get agreement on in
practice with the large query text files many of us have seen on
customer systems.

What if we instead designed this as an in-memory buffering mechanism?

i.e. new query texts are first written to the in-memory buffer, and
then flushed to disk. The in-memory buffer is kept as DSA like you
have it, so many concurrent sessions suddenly running a new query only
have one of them actually saving it to the buffer. We could then have
a background writer write that out to disk in batches, to make room
for new entries in the in-memory buffer.

If we wanted to get fancy we could even introduce compression when we
write it out to disk, since that work would no longer have to be
happening inline with the query execution - it could instead be
happening in the background worker.

>
> Open questions:
>
> 1. The attached patches use PGSTAT_KIND_EXPERIMENTAL for the
> custom kind ID. For commit, we'd want a proper kind number.

Makes sense to me - I assume the question is when we assign the real ID?

I think we could separately consider whether pg_stat_statements should
live in core (maybe?), but that seems like a different patch set to
me.

>
> 2. The 10-second eviction throttle is a compile-time constant
> (EVICTION_INTERVAL_MS). Should this be a GUC, or is a fixed
> interval sufficient?

I suspect its probably slightly too frequent, but its a bit hard to
say for sure. Personally, I don't think we need a GUC for it
necessarily, at least not from how I currently look at it.

> 3. The current design skips entry creation when at capacity and
> eviction is throttled. An alternative would be to allow
> temporary overshoot (soft limit) and never reject entries.
> Thoughts on the trade-off? However, for heavy churn and
> many unique entries we can easily overshoot the max by
> magnitudes higher which I don't think is a good idea.

Per earlier note, I think we should change this so that we trigger
eviction early before actually reaching the max.

It seems reasonable to still drop entries when we actually exceed the
max, instead of blocking on the deallocation finishing (as we do
today).

>
> 4. The default size of query text memory. The patch has it at 64MB.

I think we can work on other parts of the design first, but I wonder
if we should auto-scale that based on shared_buffers (and allow
overriding) - 64MB default seems high for very small instances.

Thanks,
Lukas

--
Lukas Fittl

Re: Improve pg_stat_statements scalability

От:
Lukas Fittl <lukas@fittl.com>
Дата:
On Thu, May 21, 2026 at 9:38 PM Lukas Fittl  wrote:
>
> On Mon, May 11, 2026 at 4:54 PM Sami Imseih  wrote:
> > pg_stat_statements has well-known scaling problems under
> > high concurrency. This patch series is an initial proposal
> > for how to $SUBJECT.
>
> Agreed. Let's fix this for Postgres 20.
>
> Thank you for putting together initial patches!

For archive's sake and others reading along, we had a productive
discussion about this today at PGConf.Dev.

I've created a new wiki page combining the prior 2025 discussion, and
notes from today:

https://wiki.postgresql.org/wiki/Scalability_of_pg_stat_statements

Thanks,
Lukas

-- 
Lukas Fittl
FAQ