Re: Remove wal_[sync|write][_time] from pg_stat_wal and track_wal_io_timing

Hi,

On Wed, Feb 19, 2025 at 01:32:33PM +0900, Michael Paquier wrote:
> These additions feel unbalanced with the existing contents, and
> overlap with the follow-up paragraph about the tuning that can be
> guessed from the contents of pg_stat_io because the new content refers
> twice to the section "WAL Configuration".  Perhaps this could be
> simpler, with one sentence in the tuning part?  I would propose
> something like:
> For the WAL object, "fsyncs" and "sync_time" track the sync activity
> of WAL files via issue_xlog_fsync().  "writes" and "write_time" track
> the write activity of WAL files via XLogWrite().  See <xref
> linkend="wal-configuration"/> for more information.
> 
> My point is that the "WAL configuration" section already provides all
> the details that were in pg_stat_wal removed in 0002 and added in
> 0001, leading to duplicated contents.

Makes sense, done that way in the attached.

> > - 0002: Remove wal_[sync|write][_time] from pg_stat_wal, PendingWalStats and 
> > track_wal_io_timing. That does not make sense to split this work in sub-patches
> > so that all of this in done in 0002.
> > - 0003: remove the pgstat_prepare_io_time() parameter. Now that track_wal_io_timing
> > is gone there is no need for pgstat_prepare_io_time() to get a parameter.
> 
> 0002 and 0003 can be grouped in the same commit, IMO.

Idea was to "ease" the review but I'm fine to merge them. Done that way in the
attached.

> -   When <xref linkend="guc-track-wal-io-timing"/> is enabled, the total
> +   When <xref linkend="guc-track-io-timing"/> is enabled, the total
>     amounts of time <function>XLogWrite</function> writes and
>     <function>issue_xlog_fsync</function> syncs WAL data to disk are counted as
> -   <literal>wal_write_time</literal> and <literal>wal_sync_time</literal> in
> -   <xref linkend="pg-stat-wal-view"/>, respectively.
> +   <literal>write_time</literal> and <literal>sync_time</literal> in
> +   <xref linkend="pg-stat-io-view"/> for the wal <literal>object</literal>, respectively.
> 
> Okay to update these are part of 0002 that removes the fields, but
> there is also an argument about updating that on its own because this
> is actually the case on HEAD?  (No need to post an updated patch for
> this remark.)

That's right but that would mean almost duplicating the pg_stat_wal related
stuff (because it can't be removed from the doc until the fields are gone). I 
think it's simpler to do it as proposed initially (the end result is the same).

Regards,

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