Hi,
On Wed, 29 Jan 2025 at 18:16, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Wed, Jan 29, 2025 at 02:57:21PM +0300, Nazir Bilal Yavuz wrote:
> > 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?
> >
> > 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.
>
> I'm -1 of removing track_wal_io_timing. I think that this code path is very
> sensible to performance to not add extra overhead when not necessary asked for.
>
> 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.
>
> That said, in:
>
> +static bool
> +pgstat_should_track_io_time(IOObject io_object)
> +{
> + if (io_object == IOOBJECT_WAL)
> + return track_wal_io_timing;
> +
> + return track_io_timing;
> +}
>
> I think it would make sense to return "track_io_timing && track_wal_io_timing"
> for the IOOBJECT_WAL case. That way it maintains track_io_timing as the master
> switch for all I/O timing in pg_stat_io.
I do not think that makes sense if we want to take performance into
account. This means if we want to track WAL IO timings, we must track
other IOs timings as well. Or perhaps did you mean that not fetching
pg_stat_wal's timings from pg_stat_io and instead: track_wal_io_timing
will track timings in the pg_stat_wal but it won't track the WAL IO
timings in the pg_stat_io unless track_io_timing is enabled?
> > 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.
>
> That's true but OTOH track_wal_io_timing is already there since years (it's not
> like we are adding it).
Yes, this makes sense.
--
Regards,
Nazir Bilal Yavuz
Microsoft
