Re: New statistics for tuning WAL buffer size

On 2020-09-26 19:18, Amit Kapila wrote:
> On Fri, Sep 25, 2020 at 11:06 PM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>
>> On 2020/09/25 12:06, Masahiro Ikeda wrote:
>> > On 2020-09-18 11:11, Kyotaro Horiguchi wrote:
>> >> At Fri, 18 Sep 2020 09:40:11 +0900, Masahiro Ikeda
>> >> <ikedamsh(at)oss(dot)nttdata(dot)com> wrote in
>> >>> Thanks. I confirmed that it causes HOT pruning or killing of
>> >>> dead index tuple if DecodeCommit() is called.
>> >>>
>> >>> As you said, DecodeCommit() may access the system table.
>> >> ...
>> >>> The wals are generated only when logical replication is performed.
>> >>> So, I added pgstat_send_wal() in XLogSendLogical().
>> >>>
>> >>> But, I concerned that it causes poor performance
>> >>> since pgstat_send_wal() is called per wal record,
>> >>
>> >> I think that's too frequent. If we want to send any stats to the
>> >> collector, it is usually done at commit time using
>> >> pgstat_report_stat(), and the function avoids sending stats too
>> >> frequently. For logrep-worker, apply_handle_commit() is calling it. It
>> >> seems to be the place if we want to send the wal stats. Or it may be
>> >> better to call pgstat_send_wal() via pgstat_report_stat(), like
>> >> pg_stat_slru().
>> >
>> > Thanks for your comments.
>> > Since I changed to use pgstat_report_stat() and DecodeCommit() is calling it,
>> > the frequency to send statistics is not so high.
>>
>> On second thought, it's strange to include this change in pg_stat_wal
>> patch.
>> Because pgstat_report_stat() sends various stats and that change would
>> affect not only pg_stat_wal but also other stats views. That is, if we
>> really
>> want to make some processes call pgstat_report_stat() newly, which
>> should be implemented as a separate patch. But I'm not sure how useful
>> this change is because probably the stats are almost negligibly small
>> in those processes.
>>
>> This thought seems valid for pgstat_send_wal(). I changed the thought
>> and am inclined to be ok not to call pgstat_send_wal() in some
>> background
>> processes that are very unlikely to generate WAL.
>>

OK, I removed to pgstat_report_stat() for autovaccum launcher,
logrep-worker and logrep-launcher.

> This makes sense to me. I think even if such background processes have
> to write WAL due to wal_buffers, it will be accounted next time the
> backend sends the stats.

Thanks for your comments.

IIUC, since each process counts WalStats.m_wal_buffers_full,
backend can't send the counter which other background processes have to
write WAL due to wal_buffers.
Although we can't track all WAL activity, the impact on the statistics
is minimal so we can ignore it.

> One minor point, don't we need to reset the counter
> WalStats.m_wal_buffers_full once we sent the stats, otherwise the same
> stats will be accounted multiple times.

Now, the counter is reset in pgstat_send_wal.
Isn't it enough?

>> The checkpointer doesn't seem to call pgstat_report_stat() currently,
>> but since there is a possibility to send wal statistics, I added
>> pgstat_report_stat().
>
> IMO it's better to call pgstat_send_wal() in the checkpointer, instead,
> because of the above reason.

Ok, I changed.

> Thanks for updating the patch! I'd like to share my review comments.
>
> + <xref linkend="monitoring-pg-stat-wal-view"/> for details.
>
> Like the description for pg_stat_bgwriter, <link> tag should be used
> instead of <xref>.

Thanks, fixed.

> + <para>
> + Number of WAL writes when the <xref linkend="guc-wal-buffers"/>
> are full
> + </para></entry>
>
> I prefer the following description. Thought?
>
> "Number of times WAL data was written to the disk because wal_buffers
> got full"

Ok, I changed.

> + the <structname>pg_stat_archiver</structname> view ,or
> <literal>wal</literal>
>
> A comma should be just after "view" (not just before "or").

Sorry, anyway I think a comma is not necessary.
I removed it.

> +/*
> + * WAL global statistics counter.
> + * This counter is incremented by both each backend and background.
> + * And then, sent to the stat collector process.
> + */
> +PgStat_MsgWal WalStats;
>
> What about merging the comments for BgWriterStats and WalStats into
> one because they are almost the same? For example,
>
> -------------------------------
> /*
> * BgWriter and WAL global statistics counters.
> * Stored directly in a stats message structure so they can be sent
> * without needing to copy things around. We assume these init to
> zeroes.
> */
> PgStat_MsgBgWriter BgWriterStats;
> PgStat_MsgWal WalStats;
> -------------------------------
>
> BTW, originally there was the comment "(unused in other processes)"
> for BgWriterStats. But it seems not true, so I removed it from
> the above example.

Thanks, I changed.

> + rc = fwrite(&walStats, sizeof(walStats), 1, fpout);
> + (void) rc; /* we'll check for error with ferror */
>
> Since the patch changes the pgstat file format,
> PGSTAT_FILE_FORMAT_ID should also be changed?

Sorry about that.
I incremented PGSTAT_FILE_FORMAT_ID by +1.

> - * Clear out global and archiver statistics so they start from zero
> in
> + * Clear out global, archiver and wal statistics so they start from
> zero in
>
> This is not the issue of this patch, but isn't it better to mention
> also SLRU stats here? That is, what about "Clear out global, archiver,
> WAL and SLRU statistics so they start from zero in"?

Thanks, I changed.

> I found "wal statistics" and "wal stats" in some comments in the patch,
> but isn't it better to use "WAL statistics" and "WAL stats", instead,
> if there is no special reason to use lowercase?

OK. I fixed it.

> + /*
> + * Read wal stats struct
> + */
> + if (fread(&walStats, 1, sizeof(walStats), fpin) != sizeof(walStats))
>
> In pgstat_read_db_statsfile_timestamp(), the local variable myWalStats
> should be declared and be used to store the WAL stats read via fread(),
> instead.

Thanks, I changed it to declare myWalStats.

> +{ oid => '1136', descr => 'statistics: number of WAL writes when the
> wal buffers are full',
>
> If we change the description of wal_buffers_full column in the document
> as I proposed, we should also use the proposed description here.

OK, I fixed it.

> +{ oid => '1137', descr => 'statistics: last reset for the walwriter',
>
> "the walwriter" should be "WAL" or "WAL activity", etc?

Thanks, I fixed it.

> + * PgStat_MsgWal Sent by each backend and background workers to
> update WAL statistics.
>
> If your intention here is to mention background processes like
> checkpointer,
> "each backend and background workers" should be "backends and
> background
> processes"?

Thanks, I fixed it.

> + PgStat_Counter m_wal_buffers_full; /* number of WAL write caused by
> full of WAL buffers */
>
> I don't think this comment is necessary.

OK, I removed.

> + PgStat_Counter wal_buffers_full; /* number of WAL write caused by
> full of WAL buffers */
> + TimestampTz stat_reset_timestamp; /* last time when the stats reset
> */
>
> I don't think these comments are necessary.

OK, I removed

> +/*
> + * WAL writes statistics counter is updated by backend and background
> workers
>
> Same as above.

I fixed it.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION