On Tue, Feb 18, 2025 at 10:43:15AM +0000, Bertrand Drouvot wrote: > === Remarks
As mentioned on a previous thread, I am siding with the removal of these fields. There are more benefits on terms of granularity of this data to encourage users to move away from pg_stat_wal and stick to pg_stat_io. Reasons are posted here: https://www.postgresql.org/message-id/z7phiayt6gi0_...@paquier.xyz > R1. The "bytes" fields differ (as pg_stat_wal.wal_bytes somehow "focus" on > the wal > records size while the pg_stat_io's unit is the wal_block_size) so we keep > them > in both places. Yes, I don't think we should drop that anyway. There is no equivalent mapping and pg_stat_wal.bytes is not I/O. This is also useful in the WAL stats for the backends. > R2. track_wal_io_timing becomes useless once those fields are gone > from pg_stat_wal > > R3. PendingWalStats becomes empty, so removes it. Sounds fine to me. The simplifications are in pgstat_wal.c, where we can now depend only on the diffs in the WalUsage, which we rely on for EXPLAIN, VACUUM/ANALYZE and pgss. This makes the implementation of the backend WAL stats much simpler. > === Patch structure > > PFA 3 sub patches: > > - 0001: Add details in the pg_stat_io doc about the wal object. That's > basically > more or less a copy/paste from the pg_stat_wal fields description that will be > removed in 0002. As we already track them in pg_stat_io, it's done in a > dedicated > patch. 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. > - 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. - 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.) The removal with PgStat_PendingWalStats and the GUC gone looks clean as presented. -- Michael
signature.asc
Description: PGP signature
