On Wed, Feb 18, 2026 at 6:41 AM Bertrand Drouvot
<[email protected]> 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.


Reply via email to