On Wed, Jan 29, 2025 at 02:57:21PM +0300, Nazir Bilal Yavuz wrote: > On Tue, 28 Jan 2025 at 07:23, Michael Paquier <mich...@paquier.xyz> wrote: >> 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](). > > I agree. Do you think that we need to do this simplification in this > thread or does it need its own thread?
As far as I understand, the simplifications in PgStat_PendingWalStats require the changes of this thread first, so keeping them around for now sounds OK to me. > I agree with you but it was discussed before in this thread [2]. It > was decided to use both track_wal_io_timing and track_io_timing > because of the overhead that track_wal_io_timing creates but we can > still re-discuss it. Do you think that this discussion needs its own > thread? Let's decide it on this thread. You have done a benchmark with fsync disabled for something that only stresses WAL. And it is very dependent on the clock source. Would you really see a difference under a normal pgbench workload? For example, should we compare HEAD and the patch with track_io_timing=on but track_wal_io_timing=off with a modified version of the patch so as IOOBJECT_WAL timing data is controlled by track_io_timing=on? The previous results could have been also influenced by the timings of pg_stat_wal because track_wal_io_timing was on. > If we continue to discuss it in this thread, I am in favor of removing > track_wal_io_timing and using track_io_timing for all types of I/Os. > Like you said, this cross-dependency makes things more complex than > they used to be. Downside of removing track_wal_io_timing is affecting > people who: > > 1- Want to track timings of only WAL I/Os. > 2- Want to track timings of all IOs except WAL I/Os. > > I think the first group is more important than the second because > track_io_timing already creates overhead. > > One additional thing is that I think track_io_timing is a general > word. When it exists, I do not expect there to be another GUC like > track_wal_io_timing to track WAL I/Os' timings. Just to be clear here, I'd be okay to remove entirely the GUC track_wal_io_timing iff pg_stat_wal has no more need for it if we feed the data of pg_stat_io to pg_stat_wal. Having track_io_timing be used for all the timing information in pg_stat_io makes the whole design leaner, IMO, removing it from the patch and pgstat_io.c simplifies a lot the user history. -- Michael
signature.asc
Description: PGP signature
