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