Re: New statistics for tuning WAL buffer size

On 2020/09/15 15:52, Masahiro Ikeda wrote:
> On 2020-09-11 01:40, Fujii Masao wrote:
>> On 2020/09/09 13:57, Masahiro Ikeda wrote:
>>> On 2020-09-07 16:19, Fujii Masao wrote:
>>>> On 2020/09/07 9:58, Masahiro Ikeda wrote:
>>>>> 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(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>>>>>
>>>>>>> On 2020/09/04 11:50, tsunakawa(dot)takay(at)fujitsu(dot)com wrote:
>>>>>>>> From: Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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.
>>>>
>>>> Good point! Thanks for updating the patch!
>>>>
>>>>
>>>> @@ -604,6 +604,7 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
>>>>                           onerel->rd_rel->relisshared,
>>>>                           Max(new_live_tuples, 0),
>>>>                           vacrelstats->new_dead_tuples);
>>>> +    pgstat_send_wal();
>>>>
>>>> I guess that you changed heap_vacuum_rel() as above so that autovacuum
>>>> workers can send WAL stats. But heap_vacuum_rel() can be called by
>>>> the processes (e.g., backends) other than autovacuum workers? Also
>>>> what happens if autovacuum workers just do ANALYZE only? In that case,
>>>> heap_vacuum_rel() may not be called.
>>>>
>>>> Currently autovacuum worker reports the stats at the exit via
>>>> pgstat_beshutdown_hook(). Unlike other processes, autovacuum worker
>>>> is not the process that basically keeps running during the service. It exits
>>>> after it does vacuum or analyze. So ISTM that it's not bad to report the stats
>>>> only at the exit, in autovacuum worker case. There is no need to add extra
>>>> code for WAL stats report by autovacuum worker. Thought?
>>>
>>> Thanks, I understood. I removed this code.
>>>
>>>>
>>>> @@ -1430,6 +1430,9 @@ WalSndWaitForWal(XLogRecPtr loc)
>>>>          else
>>>>              RecentFlushPtr = GetXLogReplayRecPtr(NULL);
>>>>  +        /* Send wal statistics */
>>>> +        pgstat_send_wal();
>>>>
>>>> AFAIR logical walsender uses three loops in WalSndLoop(), WalSndWriteData()
>>>> and WalSndWaitForWal(). But could you tell me why added pgstat_send_wal()
>>>> into WalSndWaitForWal()? I'd like to know why WalSndWaitForWal() is the best
>>>> for that purpose.
>>>
>>> I checked what function calls XLogBackgroundFlush() which calls
>>> AdvanceXLInsertBuffer() to increment m_wal_buffers_full.
>>>
>>> I found that WalSndWaitForWal() calls it, so I added it.
>>
>> Ok. But XLogBackgroundFlush() calls AdvanceXLInsertBuffer() wit the
>> second argument opportunistic=true, so in this case WAL write by
>> wal_buffers full seems to never happen. Right? If this understanding
>> is right, WalSndWaitForWal() doesn't need to call pgstat_send_wal().
>> Probably also walwriter doesn't need to do that.

Thanks for updating the patch! This patch adds pgstat_send_wal() in
walwriter main loop. But isn't this unnecessary because of the above reason?
That is, since walwriter calls AdvanceXLInsertBuffer() with
the second argument "opportunistic" = true via XLogBackgroundFlush(),
the event of full wal_buffers will never happen. No?

>>
>> The logical rep walsender can generate WAL and call
>> AdvanceXLInsertBuffer() when it executes the replication commands like
>> CREATE_REPLICATION_SLOT. But this case is already covered by
>> pgstat_report_activity()->pgstat_send_wal() called in PostgresMain(),
>> with your patch. So no more calls to pgstat_send_wal() seems necessary
>> for logical rep walsender.
>
> Thanks for your reviews. I didn't notice that.
> I updated the patches.

Sorry, the above my analysis might be incorrect. During logical replication,
walsender may access to the system table. Which may cause HOT pruning
or killing of dead index tuple. Also which can cause WAL and
full wal_buffers event. Thought?

Regards,

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