From: | Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, "bharath(dot)rupireddyforpostgres(at)gmail(dot)com" <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Melanie Plageman <melanieplageman(at)gmail(dot)com> |
Subject: | Re: Show WAL write and fsync stats in pg_stat_io |
Date: | 2023-12-12 11:29:03 |
Message-ID: | CAN55FZ1kOwa=VZnAPpyrvKn1C13+zX=10ZaTZgM30CCrui4kMQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
Thanks for the feedback! The new version of the patch is attached.
On Tue, 5 Dec 2023 at 09:16, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> - if (io_op == IOOP_WRITE || io_op == IOOP_EXTEND)
> + if (io_op == IOOP_EXTEND || io_op == IOOP_WRITE)
>
> Unrelated diff.
Done.
>
> + if (io_object == IOOBJECT_WAL && io_context == IOCONTEXT_NORMAL &&
> + io_op == IOOP_FSYNC)
> + PendingWalStats.wal_sync += cnt;
>
> Nah, I really don't think that adding this dependency within
> pg_stat_io is a good idea.
>
> - PendingWalStats.wal_sync++;
> + pgstat_count_io_op_time(IOOBJECT_WAL, IOCONTEXT_NORMAL, IOOP_FSYNC,
> + io_start, 1);
>
> This is the only caller where this matters, and the count is always 1.
I reverted that, pgstat_count_io_op_n doesn't count
PendingWalStats.wal_sync now.
>
> + no_wal_normal_read = bktype == B_AUTOVAC_LAUNCHER ||
> + bktype == B_AUTOVAC_WORKER || bktype == B_BACKEND ||
> + bktype == B_BG_WORKER || bktype == B_BG_WRITER ||
> + bktype == B_CHECKPOINTER || bktype == B_WAL_RECEIVER ||
> + bktype == B_WAL_SENDER || bktype == B_WAL_WRITER;
> +
> + if (no_wal_normal_read &&
> + (io_object == IOOBJECT_WAL &&
> + io_op == IOOP_READ))
> + return false;
>
> This may be more readable if an enum is applied, without a default
> clause so as it would not be forgotten if a new type is added, perhaps
> in its own little routine.
Done.
>
> - if (track_io_timing)
> + if (track_io_timing || track_wal_io_timing)
> INSTR_TIME_SET_CURRENT(io_start);
> else
>
> This interface from pgstat_prepare_io_time() is not really good,
> because we could finish by setting io_start in the existing code paths
> calling this routine even if track_io_timing is false when
> track_wal_io_timing is true. Why not changing this interface a bit
> and pass down a GUC (track_io_timing or track_wal_io_timing) as an
> argument of the function depending on what we expect to trigger the
> timings?
Done in 0001.
>
> - /* Convert counters from microsec to millisec for display */
> - values[6] = Float8GetDatum(((double) wal_stats->wal_write_time) / 1000.0);
> - values[7] = Float8GetDatum(((double) wal_stats->wal_sync_time) / 1000.0);
> + /*
> + * There is no need to calculate timings for both pg_stat_wal and
> + * pg_stat_io. So, fetch timings from pg_stat_io to make stats gathering
> + * cheaper. Note that, since timings are fetched from pg_stat_io;
> + * pg_stat_reset_shared('io') will reset pg_stat_wal's timings too.
> + *
> + * Convert counters from microsec to millisec for display
> + */
> + values[6] = Float8GetDatum(pg_stat_get_io_time(IOOBJECT_WAL,
> + IOCONTEXT_NORMAL,
> + IOOP_WRITE));
> + values[7] = Float8GetDatum(pg_stat_get_io_time(IOOBJECT_WAL,
> + IOCONTEXT_NORMAL,
> + IOOP_FSYNC));
>
> Perhaps it is simpler to remove these columns from pg_stat_get_wal()
> and plug an SQL upgrade to the view definition of pg_stat_wal?
Done in 0003 but I am not sure if that is what you expected.
> Finding a good balance between the subroutines, the two GUCs, the
> contexts, the I/O operation type and the objects is the tricky part of
> this patch. If the dependency to PendingWalStats is removed and if
> the interface of pgstat_prepare_io_time is improved, things are a bit
> cleaner, but it feels like we could do more.. Nya.
I agree. The patch is not logically complicated but it is hard to
select the best way.
Any kind of feedback would be appreciated.
--
Regards,
Nazir Bilal Yavuz
Microsoft
Attachment | Content-Type | Size |
---|---|---|
v6-0006-Add-IOOBJECT_WAL-IOCONTEXT_NORMAL-read-tests.patch | application/x-patch | 1.8 KB |
v6-0003-Fetch-pg_stat_wal-s-timings-from-pg_stat_io.patch | application/x-patch | 7.2 KB |
v6-0005-Add-IOOBJECT_WAL-IOCONTEXT_INIT-write-and-fsync-t.patch | application/x-patch | 2.1 KB |
v6-0004-Add-IOOBJECT_WAL-IOCONTEXT_NORMAL-write-and-fsync.patch | application/x-patch | 4.2 KB |
v6-0002-Show-WAL-stats-on-pg_stat_io-except-streaming-rep.patch | application/x-patch | 17.3 KB |
v6-0001-Use-timing-GUCs-on-pgstat_prepare_io_time-functio.patch | application/x-patch | 4.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Euler Taveira | 2023-12-12 11:47:34 | Re: Add --check option to pgindent |
Previous Message | Tomas Vondra | 2023-12-12 11:25:23 | Re: brininsert optimization opportunity |