public inbox for [email protected]  

help / color / mirror / Atom feed

From: Michael Paquier <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Subject: Re: Add callbacks for fixed-numbered stats flush in pgstats
Date: Wed, 4 Sep 2024 15:12:37 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>

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


Attachments:

  [application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
  download

view thread (7+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add callbacks for fixed-numbered stats flush in pgstats
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox