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

At Wed, 4 Sep 2024 15:12:37 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> 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.

Yeah, I was wrong in this point.

> 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.

I doubt that overprotection is always better, but in this case, it's
not overprotection at all. The flush callbacks are called
unconditionally once we decide to flush anything. Sorry for the noise.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center