Re: New statistics for tuning WAL buffer size - Mailing list pgsql-hackers

On 2020/09/02 18:56, Masahiro Ikeda wrote:
>> +/* ----------
>> + * Backend types
>> + * ----------
>>
>> You seem to forget to add "*/" into the above comment.
>> This issue could cause the following compiler warning.
>> ../../src/include/pgstat.h:761:1: warning: '/*' within block comment [-Wcomment]
> 
> Thanks for the comment. I fixed.

Thanks for the fix! But why are those comments necessary?


> 
>> The contents of pg_stat_walwrites are reset when the server
>> is restarted. Isn't this problematic? IMO since pg_stat_walwrites
>> is a collected statistics view, basically its contents should be
>> kept even in the case of server restart.
> 
> I agree your opinion.
> I modified to use the statistics collector and persist the wal statistics.
> 
> 
> I changed the view name from pg_stat_walwrites to pg_stat_walwriter.
> I think it is better to match naming scheme with other views like pg_stat_bgwriter,
> which is for bgwriter statistics but it has the statistics related to backend.

I prefer the view name pg_stat_walwriter for the consistency with
other view names. But we also have pg_stat_wal_receiver. Which
makes me think that maybe pg_stat_wal_writer is better for
the consistency. Thought? IMO either of them works for me.
I'd like to hear more opinons about this.


> 
> The pg_stat_walwriter is not security restricted now, so ordinary users can access it.
> I has the same security level as pg_stat_archiver.If you have any comments, please let me know.

+       <structfield>dirty_writes</structfield> <type>bigint</type>

I guess that the column name "dirty_writes" derived from
the DTrace probe name. Isn't this name confusing? We should
rename it to "wal_buffers_full" or something?


+/* ----------
+ * PgStat_MsgWalWriter            Sent by the walwriter to update statistics.

This comment seems not accurate because backends also send it.

+/*
+ * WAL writes statistics counter is updated in XLogWrite function
+ */
+extern PgStat_MsgWalWriter WalWriterStats;

This comment seems not right because the counter is not updated in XLogWrite().

+-- There will surely and maximum one record
+select count(*) = 1 as ok from pg_stat_walwriter;

What about changing this comment to "There must be only one record"?

+                    WalWriterStats.m_xlog_dirty_writes++;
                      LWLockRelease(WALWriteLock);

Since WalWriterStats.m_xlog_dirty_writes doesn't need to be protected
with WALWriteLock, isn't it better to increment that after releasing the lock?

+CREATE VIEW pg_stat_walwriter AS
+    SELECT
+        pg_stat_get_xlog_dirty_writes() AS dirty_writes,
+        pg_stat_get_walwriter_stat_reset_time() AS stats_reset;
+
  CREATE VIEW pg_stat_progress_vacuum AS

In system_views.sql, the definition of pg_stat_walwriter should be
placed just after that of pg_stat_bgwriter not pg_stat_progress_analyze.

      }
-
      /*
       * We found an existing collector stats file. Read it and put all the

You seem to accidentally have removed the empty line here.

-                 errhint("Target must be \"archiver\" or \"bgwriter\".")));
+                 errhint("Target must be \"archiver\" or \"bgwriter\" or \"walwriter\".")));

There are two "or" in the message, but the former should be replaced with ","?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION