Re: About to add WAL write/fsync statistics to pg_stat_wal view

Hi, Japin

Thanks for your comments.

On 2021-01-23 01:46, japin wrote:
> Hi, Masahiro
>
> Thanks for you update the v4 patch. Here are some comments:
>
> (1)
> + char *msg = NULL;
> + bool sync_called; /* whether to sync
> data to the disk. */
> + instr_time start;
> + instr_time duration;
> +
> + /* check whether to sync data to the disk is really occurred.
> */
> + sync_called = false;
>
> Maybe we can initialize the "sync_called" variable when declare it.

Yes, I fixed it.

> (2)
> + if (sync_called)
> + {
> + /* increment the i/o timing and the number of times to
> fsync WAL data */
> + if (track_wal_io_timing)
> + {
> + INSTR_TIME_SET_CURRENT(duration);
> + INSTR_TIME_SUBTRACT(duration, start);
> + WalStats.m_wal_sync_time =
> INSTR_TIME_GET_MICROSEC(duration);
> + }
> +
> + WalStats.m_wal_sync++;
> + }
>
> There is an extra space before INSTR_TIME_GET_MICROSEC(duration).

Yes, I removed it.

> In the issue_xlog_fsync(), the comment says that if sync_method is
> SYNC_METHOD_OPEN or SYNC_METHOD_OPEN_DSYNC, it already write synced.
> Does that mean it synced when write the WAL data? And for those cases,
> we
> cannot get accurate write/sync timing and number of write/sync times,
> right?
>
> case SYNC_METHOD_OPEN:
> case SYNC_METHOD_OPEN_DSYNC:
> /* write synced it already */
> break;

Yes, I add the following comments in the document.

@@ -3515,6 +3515,9 @@ SELECT pid, wait_event_type, wait_event FROM
pg_stat_activity WHERE wait_event i
</para>
<para>
Total number of times WAL data was synced to disk
+ (if <xref linkend="guc-wal-sync-method"/> is
<literal>open_datasync</literal> or
+ <literal>open_sync</literal>, this value is zero because WAL
data is synced
+ when to write it).
</para></entry>
</row>

@@ -3525,7 +3528,10 @@ SELECT pid, wait_event_type, wait_event FROM
pg_stat_activity WHERE wait_event i
<para>
Total amount of time that has been spent in the portion of
WAL data was synced to disk, in milliseconds
- (if <xref linkend="guc-track-wal-io-timing"/> is enabled,
otherwise zero)
+ (if <xref linkend="guc-track-wal-io-timing"/> is enabled,
otherwise zero.
+ if <xref linkend="guc-wal-sync-method"/> is
<literal>open_datasync</literal> or
+ <literal>open_sync</literal>, this value is zero too because WAL
data is synced
+ when to write it).
</para></entry>
</row>

I attached a modified patch.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION