Date: Wed, 03 Jun 2026 17:10:39 +0900 (JST)
Message-Id: <20260603.171039.1005148564287106445.horikyota.ntt@gmail.com>
To: michael@paquier.xyz
Cc: samimseih@gmail.com, lukas@fittl.com, pgsql-hackers@postgresql.org
Subject: Re: Improve pg_stat_statements scalability
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <ah54kGcap-VuWd7y@paquier.xyz>
References: 
 <CAA5RZ0uoxiQ2_=xHGRnyc4WdM9aR0fzdMhBubnw97po==--yGQ@mail.gmail.com>
	<20260601.135858.1116584574478485492.horikyota.ntt@gmail.com>
	<ah54kGcap-VuWd7y@paquier.xyz>
User-Agent: Mew version 6.8 on Emacs 29.4
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20260603.171039.1005148564287106445.horikyota.ntt%40gmail.com>
Precedence: bulk

Hello,

At Tue, 2 Jun 2026 15:30:40 +0900, Michael Paquier <michael@paquier.xyz> 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