Re: Show WAL write and fsync stats in pg_stat_io

Hi,

On Wed, 10 Jan 2024 at 08:25, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Jan 03, 2024 at 04:10:58PM +0300, Nazir Bilal Yavuz wrote:
> >
> > I thought removing op_bytes completely ( as you said "This patch
> > extends it with two more operation sizes, and there are even cases
> > where it may be a variable" ) from pg_stat_io view then adding
> > something like {read | write | extend}_bytes and {read | write |
> > extend}_calls could be better, so that we don't lose any information.
>
> But then you'd lose the possibility to analyze correlations between
> the size and the number of the operations, which is something that
> matters for more complex I/O scenarios. This does not need to be
> tackled in this patch, which is useful on its own, though I am really
> wondering if this is required for the recent work done by Thomas.
> Perhaps Andres, Thomas or Melanie could comment on that?

Yes, you are right.

> >> 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.
> >
> > I added that and disabled B_WAL_RECEIVER backend with comments
> > explaining why. v8 is attached.
>
> I can see that's what you have been adding here, which should be OK:
>
> > - if (track_io_timing)
> > + /*
> > + * B_WAL_RECEIVER backend does IOOBJECT_WAL IOObject & IOOP_READ IOOp IOs
> > + * but these IOs are not countable for now because IOOP_READ IOs' op_bytes
> > + * (number of bytes per unit of I/O) might not be the same all the time.
> > + * The current implementation requires that the op_bytes must be the same
> > + * for the same IOObject, IOContext and IOOp. To avoid confusion, the
> > + * B_WAL_RECEIVER backend & IOOBJECT_WAL IOObject IOs are disabled for
> > + * now.
> > + */
> > + if (MyBackendType == B_WAL_RECEIVER && io_object == IOOBJECT_WAL)
> > + return;
>
> This could be worded better, but that's one of these nits from me I
> usually tweak when committing stuff.

Thanks for doing that! Do you have any specific comments that can help
improve it?

> > +/*
> > + * Decide if IO timings need to be tracked. Timings associated to
> > + * IOOBJECT_WAL objects are tracked if track_wal_io_timing is enabled,
> > + * else rely on track_io_timing.
> > + */
> > +static bool
> > +pgstat_should_track_io_time(IOObject io_object)
> > +{
> > + if (io_object == IOOBJECT_WAL)
> > + return track_wal_io_timing;
> > +
> > + return track_io_timing;
> > +}
>
> One thing I was also considering is if eliminating this routine would
> make pgstat_count_io_op_time() more readable the result, but I cannot
> get to that.

I could not think of a way to eliminate pgstat_should_track_io_time()
route without causing performance regressions. What do you think about
moving inside of 'pgstat_should_track_io_time(io_object) if check' to
another function and call this function from
pgstat_count_io_op_time()? This does not change anything but IMO it
increases the readability.

> > if (io_op == IOOP_WRITE || io_op == IOOP_EXTEND)
> > {
> > - pgstat_count_buffer_write_time(INSTR_TIME_GET_MICROSEC(io_time));
> > + if (io_object != IOOBJECT_WAL)
> > + pgstat_count_buffer_write_time(INSTR_TIME_GET_MICROSEC(io_time));
> > +
> > if (io_object == IOOBJECT_RELATION)
> > INSTR_TIME_ADD(pgBufferUsage.shared_blk_write_time, io_time);
> > else if (io_object == IOOBJECT_TEMP_RELATION)
> > @@ -139,7 +177,9 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
> > }
> > else if (io_op == IOOP_READ)
> > {
> > - pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));
> > + if (io_object != IOOBJECT_WAL)
> > + pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));
> > +
> > if (io_object == IOOBJECT_RELATION)
> > INSTR_TIME_ADD(pgBufferUsage.shared_blk_read_time, io_time);
> > else if (io_object == IOOBJECT_TEMP_RELATION)
>
> A second thing is if this would be better with more switch/cases, say:
> switch (io_op):
> {
> case IOOP_EXTEND:
> case IOOP_WRITE:
> switch (io_object):
> {
> case WAL:
> /* do nothing */
> break;
> case RELATION:
> case TEMP:
> .. blah ..
> }
> break;
> case IOOP_READ:
> switch (io_object):
> {
> .. blah ..
> }
> break;
> }
>
> Or just this one to make it clear that nothing happens for WAL
> objects:
> switch (io_object):
> {
> case WAL:
> /* do nothing */
> break;
> case RELATION:
> switch (io_op):
> {
> case IOOP_EXTEND:
> case IOOP_WRITE:
> .. blah ..
> case IOOP_READ:
> .. blah ..
> }
> break;
> case TEMP:
> /* same switch as RELATION */
> break;
> }
>
> This duplicates a bit things, but at least in the second case it's
> clear which counters are updated when I/O timings are tracked. It's
> OK by me if people don't like this suggestion, but that would avoid
> bugs like the one I found upthread.

I am more inclined towards the second one because it is more likely
that a new io_object will be introduced rather than a new io_op. So, I
think the second one is a bit more future proof.

--
Regards,
Nazir Bilal Yavuz
Microsoft