On Tue, Dec 26, 2023 at 03:35:52PM +0300, Nazir Bilal Yavuz wrote: > On Tue, 26 Dec 2023 at 13:10, Michael Paquier <mich...@paquier.xyz> wrote: >> I am not sure while the whole point of the exercise is to have all the >> I/O related data in a single view. Something that I've also found a >> bit disturbing yesterday while looking at your patch is the fact that >> the operation size is guessed from the context and object type when >> querying the view because now everything is tied to BLCKSZ. This >> patch extends it with two more operation sizes, and there are even >> cases where it may be a variable. Could it be a better option to >> extend pgstat_count_io_op_time() so as callers can themselves give the >> size of the operation? > > Do you mean removing the op_bytes column and tracking the number of > bytes in reads, writes, and extends? If so, that makes sense to me but > I don't want to remove the number of operations; I believe that has a > value too. We can extend the pgstat_count_io_op_time() so it can both > track the number of bytes and the number of operations.
Apologies if my previous wording sounded confusing. The idea I had in mind was to keep op_bytes in pg_stat_io, and extend it so as a value of NULL (or 0, or -1) is a synonym as "writes", "extends" and "reads" as a number of bytes. > Also, it is not directly related to this patch but vectored IO [1] is > coming soon; so the number of operations could be wrong since vectored > IO could merge a couple of operations. Hmm. I have not checked this patch series so I cannot say for sure, but we'd likely just want to track the number of bytes if a single operation has a non-equal size rather than registering in pg_stat_io N rows with different op_bytes, no? I am looping in Thomas Munro in CC for comments. >> The whole patch is kind of itself complicated enough, so I'd be OK to >> discard the case of the WAL receiver for now. Now, if we do so, the >> code stack of pgstat_io.c should handle WAL receivers as something >> entirely disabled until all the known issues are solved. There is >> still a lot of value in tracking WAL data associated to the WAL >> writer, normal backends and WAL senders. > > Why can't we add comments and leave it as it is? Is it because this > could cause misunderstandings? > > If we want to entirely disable it, we can add > > if (MyBackendType == B_WAL_RECEIVER && io_object == IOOBJECT_WAL) > return; > > to the top of the pgstat_count_io_op_time() since all IOOBJECT_WAL > calls are done by this function, then we can disable it at > pgstat_tracks_io_bktype(). Yeah, a limitation like that may be acceptable for now. Tracking the WAL writer and WAL sender activities can be relevant in a lot of cases even if we don't have the full picture for the WAL receiver yet. -- Michael
signature.asc
Description: PGP signature
