Date: Wed, 4 Sep 2024 05:28:56 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
Subject: Re: Add callbacks for fixed-numbered stats flush in pgstats
Message-ID: <ZtfwGONWW0MegydF@ip-10-97-1-34.eu-west-3.compute.internal>
References: <ZtaVO0N-aTwiAk3w@paquier.xyz>
 <20240904.140546.948122275033564187.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240904.140546.948122275033564187.horikyota.ntt@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZtfwGONWW0MegydF%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Wed, Sep 04, 2024 at 02:05:46PM +0900, Kyotaro Horiguchi wrote:
> At Tue, 3 Sep 2024 13:48:59 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> > Hi all,
> > 
> > The last TODO item I had in my bucket about the generalization of
> > pgstats is the option to a better control on the flush of the stats
> > depending on the kind for fixed-numbered stats.  Currently, this is
> > controlled by pgstat_report_stat(), that includes special handling for
> > WAL, IO and SLRU stats, with two generic concepts:
> > - Check if there are pending entries, allowing a fast-path exit.
> > - Do the actual flush, with a recheck on pending entries.
> > 
> > SLRU and IO control that with one variable each, and WAL uses a
> > routine for the same called pgstat_have_pending_wal().  Please find
> > attached a patch to generalize the concept, with two new callbacks
> > that can be used for fixed-numbered stats.  SLRU, IO and WAL are
> > switched to use these (the two pgstat_flush_* routines have been kept
> > on purpose).  This brings some clarity in the code, by making
> > have_iostats and have_slrustats static in their respective files.  The
> > two pgstat_flush_* wrappers do not need a boolean as return result.
> 
> The generalization sounds good to me, and hiding the private flags in
> private places also seems good.

+1 on the idea.

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

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com