On Wed, Sep 20, 2023 at 10:57:48AM +0300, Nazir Bilal Yavuz wrote:
> Any kind of feedback would be appreciated.
This was registered in the CF, so I have given it a look. Note that
0001 has a conflict with pgstat_count_io_op_time(), so it cannot be
applied.
+pgstat_should_track_io_time(IOObject io_object, IOContext io_context)
+{
+ /*
+ * io times of IOOBJECT_WAL IOObject needs to be tracked when
+ * 'track_wal_io_timing' is set regardless of 'track_io_timing'.
+ */
+ if (io_object == IOOBJECT_WAL)
+ return track_wal_io_timing;
+
+ return track_io_timing;
I can see the temptation to do that, but I have mixed feelings about
the approach of mixing two GUCs in a code path dedicated to pg_stat_io
where now we only rely on track_io_timing. The result brings
confusion, while making pg_stat_io, which is itself only used for
block-based operations, harder to read.
The suggestion I am seeing here to have a pg_stat_io_wal (with a SRF)
is quite tempting, actually, creating a neat separation between the
existing pg_stat_io and pg_stat_wal (not a SRF), with a third view
that provides more details about the contexts and backend types for
the WAL stats with its relevant fields:
https://www.postgresql.org/message-id/CAAKRu_bM55pj3pPRW0nd_-paWHLRkOU69r816AeztBBa-N1HLA@mail.gmail.com
And perhaps just putting that everything that calls
pgstat_count_io_op_time() under track_io_timing is just natural?
What's the performance regression you would expect if both WAL and
block I/O are controlled by that, still one would expect only one of
them?
On top of that pg_stat_io is now for block-based I/O operations, so
that does not fit entirely in the picture, though I guess that Melanie
has thought more on the matter than me. That may be also a matter of
taste.
+ /* Report pending statistics to the cumulative stats system */
+ pgstat_report_wal(false);
This is hidden in 0001, still would be better if handled as a patch on
its own and optionally backpatch it as we did for the bgwriter with
e64c733bb1?
Side note: I think that we should spend more efforts in documenting
what IOContext and IOOp mean. Not something directly related to this
patch, still this patch or things similar make it a bit harder which
part of it is used for what by reading pgstat.h.
--
Michael