Hi, On Mon, 3 Feb 2025 at 11:50, Bertrand Drouvot <bertranddrouvot...@gmail.com> wrote: > > Hi, > > On Mon, Feb 03, 2025 at 01:07:26PM +0900, Michael Paquier wrote: > > On Fri, Jan 31, 2025 at 11:29:31AM +0300, Nazir Bilal Yavuz wrote: > > > On Wed, 29 Jan 2025 at 18:16, Bertrand Drouvot > > > <bertranddrouvot...@gmail.com> wrote: > > >> I think that's the main reason why ff99918c625 added this new GUC > > >> (looking at > > >> the commit message). I'd feel more comfortable if we keep it. > > > > > > As Michael suggested, I will run a couple of benchmarks to see the > > > actual effect of this change. Then let's see if this affects anything. > > > > I've looked at bit at all that today, and something like the attached > > is what seems like the best streamlined version to me for the main > > feature. I am also planning to run some short benchmarks with > > track_io_timing=on on HEAD and with the patch, then see the > > difference, without any relationship to track_wal_io_timing. > > Thanks! > > I've a few comments:
Thank you both for the v13 and the review! > === 1 > > + pgstat_count_io_op_time(IOOBJECT_WAL, IOCONTEXT_INIT, IOOP_WRITE, > + io_start, 1, > wal_segment_size); > > In case wal_init_zero is false, then we're only seeking to the end and write a > solitary byte. Then, is reporting "wal_segment_size" correct? I think you are right. It would make sense to have two pgstat_count_io_op_time() calls here. One for wal_segment_size and one for solitary byte. > === 2 > > + /* > + * Measure I/O timing to write WAL data, for pg_stat_wal > + * and/or pg_stat_io. > + */ > + start = pgstat_prepare_io_time(track_wal_io_timing || track_io_timing); > > I think that makes sense done that way (as track_wal_io_timing does not have > any effect in pgstat_count_io_op_time()). Nit: maybe change the order in the > comment to reflect the code ordering? (I mean to say re-word to "for > pg_stat_io > and/or pg_stat_wal). The order is ok in issue_xlog_fsync() though. > > === 3 > > What about adding a message in the doc as mentioned in [1]? (I'd not be > surprised > if some people wonder why the "bytes" fields differ). > > === 4 > > pgstat_tracks_io_object() starts to be hard to read. I wonder if it could be > simplified with switch but that could be done after this one goes in. > > === 5 > > I think this patch will help simplify the per-backend WAL related patch, > that's > nice. And I agree with the other comments you mentioned. > === 6 > > I'll also do some benchmark on my side. Thanks! -- Regards, Nazir Bilal Yavuz Microsoft
