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

Thanks for the review and advice!

On 2020-09-03 16:05, Fujii Masao wrote:
> 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?

Sorry about that. This comment is not necessary.
I removed it.

>> The pg_stat_walwriter is not security restricted now, so ordinary 
>> users can access it.
>> It 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?

I agree and rename it to "wal_buffers_full".

> +/* ----------
> + * 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().

Right. I fixed it to "Sent by each backend and background workers to 
update WAL statistics."
In the future, other statistics will be included so I remove the 
function's name.


> +-- 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"?

Thanks, I fixed.

> +                    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?

Thanks, I fixed.

> +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.

OK, I fixed it.

>      }
> -
>      /*
>       * We found an existing collector stats file. Read it and put all the
> 
> You seem to accidentally have removed the empty line here.

Sorry about that. I fixed it.

> -                 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 ","?

Thanks, I fixed.


On 2020-09-05 18:40, Magnus Hagander wrote:
> On Fri, Sep 4, 2020 at 5:42 AM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
> 
>> On 2020/09/04 11:50, tsunakawa.takay@fujitsu.com wrote:
>>> From: Fujii Masao <masao.fujii@oss.nttdata.com>
>>>>> 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.
>>> 
>>> I think pg_stat_bgwriter is now a misnomer, because it contains
>> the backends' activity.  Likewise, pg_stat_walwriter leads to
>> misunderstanding because its information is not limited to WAL
>> writer.
>>> 
>>> How about simply pg_stat_wal?  In the future, we may want to
>> include WAL reads in this view, e.g. reading undo logs in zheap.
>> 
>> Sounds reasonable.
> 
> +1.
> 
> pg_stat_bgwriter has had the "wrong name" for quite some time now --
> it became even more apparent when the checkpointer was split out to
> it's own process, and that's not exactly a recent change. And it had
> allocs in it from day one...
> 
> I think naming it for what the data in it is ("wal") rather than which
> process deals with it ("walwriter") is correct, unless the statistics
> can be known to only *ever* affect one type of process. (And then
> different processes can affect different columns in the view). As a
> general rule -- and that's from what I can tell exactly what's being
> proposed.

Thanks for your comments. I agree with your opinions.
I changed the view name to "pg_stat_wal".


I fixed the code to send the WAL statistics from not only backend and 
walwriter
but also checkpointer, walsender and autovacuum worker.


Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION