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
