On Mon, Jan 27, 2025 at 05:13:39PM +0300, Nazir Bilal Yavuz wrote: > On Mon, 27 Jan 2025 at 16:59, Nazir Bilal Yavuz <byavu...@gmail.com> wrote: >> >> On Mon, 27 Jan 2025 at 03:52, Michael Paquier <mich...@paquier.xyz> wrote: >> We use PendingWalStats.wal_[ write | sync ]_time only to show timings >> on the pg_stat_wal view, right? And now these pg_stat_wal.wal_[ write >> | sync ]_time datas are fetched from the pg_stat_io view when the >> track_wal_io_timing is on. So, I think it is correct to remove these.
As you say, removing the counters in the second patch does not matter as if you are going to combine them and.. >> >> I made a mistake while splitting the patches. The places where >> 'PendingWalStats.wal_[ write | sync ]_time are incremented (the code >> piece you shared)' are removed in 0002 (0001 now), but they should be >> removed in 0003 (0002 now) instead. This is corrected in v11. My issue was in the first patch that should not have removed them. My apologies for the confusion, I should have pointed out that this was likely an incorrect rebase and/or patch split. > If we agree with removing PendingWalStats.wal_[ write | sync ]_time > variables, then it would make sense to remove PgStat_PendingWalStats > struct completely. We have that struct because [1] it is cheap to > store PendingWalStats.wal_[ write | sync ]_time as instr_time instead > of PgStat_Counter. > > [1] ca7b3c4c00 I agree that some simplification would be nice because it also makes Bertrand's patch around [1] to not have some special handling with PgStat_PendingWalStats without us losing monitoring capabilities, I hope. So maximizing simplifications before integrating more capabilities should make the whole implementation effort easier. What you doing in 0001 is a first good step towards this goal, as this also plugs in a few things for backend statistics with the calls to pgstat_count_io_op[_time](). + /* Report pending statistics to the cumulative stats system */ + pgstat_flush_io(false); This has been discussed previously under a pgstat_report_wal() call. Why do you need this specific call? Perhaps this should be documented as a comment? + if (io_object == IOOBJECT_WAL) + return track_wal_io_timing Hmm. Andres has commented in the past that we want pg_stat_io to server as a central place for all the I/O statistics. Thinking more about that, I am not really convinced that we actually need to make this area of the code in pgstat_io.c rely on two GUCs. How about simplifying things so as we just rely on track_io_timing for everything, without creating a strange dependency on the IOObject with more routines like pgstat_should_track_io_time()? I'd really want less of these GUCs, not more of them with cross-dependencies depending on the stats kinds we are dealing with. If we replace the timings from pg_stat_wal with the ones in pg_stat_io, we should be in a good position to remove track_wal_io_timing entirely, as well. This has the merit of making your patch a lot simpler, meaning extra bonus points. [1]: https://commitfest.postgresql.org/52/5492/ -- Michael
signature.asc
Description: PGP signature
