Re: Add callbacks for fixed-numbered stats flush in pgstats

On Wed, Sep 04, 2024 at 05:28:56AM +0000, Bertrand Drouvot wrote:
> On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote:
>> The generalization sounds good to me, and hiding the private flags in
>> private places also seems good.
>
> +1 on the idea.

Thanks for the feedback.

>> Regarding pgstat_io_flush_cb, I think it no longer needs to check the
>> have_iostats variable, and that check should be moved to the new
>> pgstat_flush_io(). The same applies to pgstat_wal_flush_cb() (and
>> pgstat_flush_wal()). As for pgstat_slru_flush_cb, it simply doesn't
>> need the check anymore.
>
> Agree. Another option could be to add only one callback (the flush_fixed_cb one)
> and get rid of the have_fixed_pending_cb one. Then add an extra parameter to the
> flush_fixed_cb that would only check if there is pending stats (without
> flushing them). I think those 2 callbacks are highly related that's why I
> think we could "merge" them, thoughts?

I would still value the shortcut that we can use if there is no
activity to avoid the clock check with GetCurrentTimestamp(), though,
which is why I'd rather stick with two callbacks as that can lead to a
much cheaper path.

Anyway, I am not sure to follow your point about removing the boolean
checks in the flush callbacks. It's surely always a better to never
call LWLockAcquire() or LWLockConditionalAcquire() if we don't have
to, and we would go through the whole flush dance as long as we detect
some stats activity in at least one stats kind. I mean, that's cheap
enough to keep around.
--
Michael