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
