Thread: 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

On Wed, Feb 19, 2025 at 09:24:41AM +0000, Bertrand Drouvot wrote:
> 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).

After sleeping on it, I still saw no reason to not apply the changes
from 0002 in wal.sgml to describe that the stats for the writes/fsyncs
are in pg_stat_io rather than pg_stat_wal for the "WAL configuration"
section, so done that.  The tags were a bit inconsistent, and I've
managed to miss the.. cough.. s/sync_time/fsync_time/.

0001 was mixing a bit the tags <varname>, <literal> and <function>,
and I've moved that as a paragraph not under the tuning part, to make
it a more general statement with its link to "WAL configuration",
which is a very popular link for pg_stat_io.

Attached is the rest, as of v3-0002.
--
Michael

Hi,

On Thu, Feb 20, 2025 at 02:37:18PM +0900, Michael Paquier wrote:
> On Wed, Feb 19, 2025 at 09:24:41AM +0000, Bertrand Drouvot wrote:
> > 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).
> 
> After sleeping on it, I still saw no reason to not apply the changes
> from 0002 in wal.sgml to describe that the stats for the writes/fsyncs
> are in pg_stat_io rather than pg_stat_wal for the "WAL configuration"
> section, so done that.

I see, I did not get that you wanted to get rid of the pg_stat_wal part before
removing the fields. Makes sense that way to "just" replace the pg_stat_wal
by pg_stat_io first in the doc as you've done in 4538bd3f1dd.

> and I've moved that as a paragraph not under the tuning part, to make
> it a more general statement with its link to "WAL configuration",
> which is a very popular link for pg_stat_io.

Makes more sense, agree.

> Attached is the rest, as of v3-0002.

LGTM.

Regards,

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