Re: Flush some statistics within running transactions

On Wed, Feb 18, 2026 at 6:41 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Tue, Feb 17, 2026 at 01:18:35PM -0600, Sami Imseih wrote:
> > >
> > > > I do not have any further comments on this patchset.
> > >
> > > Thanks for the review!
> >
> > I flipped this CF entry to Ready-for-committer
>
> Thanks!
>
> PFA a mandatory rebase (nothing that needs review) due to a92b809f9da1.

Hi Bertrand!

Thanks for working on this. I've took a quick look on this patchset:

v8-0005: you start using pgstat_schedule_anytime_update() from really
hot macros like pgstat_count_buffer_hit() / pgstat_count_buffer_read()
or pgstat_count_heap_getnext(), e.g.:

#define pgstat_count_buffer_hit(rel)
do {
if (pgstat_should_count_relation(rel))
+ {
(rel)->pgstat_info->counts.blocks_hit++;
+ pgstat_schedule_anytime_update();
+ }
} while (0)

where #define pgstat_schedule_anytime_update() is
do {
if (IsUnderPostmaster &&
!get_timeout_active(ANYTIME_STATS_UPDATE_TIMEOUT))
enable_timeout_after(ANYTIME_STATS_UPDATE_TIMEOUT,
pgstat_flush_interval);
} while (0)

however that function (get_timeout_active()) is not static inlined so I'm
wondering wouldn't there some major performance impact? Those buffer
macros seem to be pretty heavy hitters, e.g. quite often even per single
buffer in PinBufferForBlock():
pgstat_count_buffer_read(rel);
if (*foundPtr)
pgstat_count_buffer_hit(rel);

so it seems to be:
- often unnecessary double work (and probably as this is a function call to
get_timeout_active it won't be optimized by compiler?)
- but the main question is: why do we need that often to recheck and re-enable
timers so often from such hot places?

v8-0001: this patch modifies ProcessInterrupts() which checks for
AnytimeStatsUpdateTimeoutPending and it may happen that it takes LWLocks
(pgstat_report_anytime_stat()->pgstat_flush_pending_entries()->e.g.
pgstat_*_flush_cb()-> pgstat_lock_entry() -> LWLock)

(It's more a question than finding): isn't it too risky to take that
LWLock from potentially random spots as the CHECK_FOR_INTERRUPTS() is
literally everywhere (~318 places). Wouldn't it be safer to flush
from a couple of desired places?

-J.