MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Tue, 22 Dec 2020 11:16:43 +0900
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Andres Freund <andres@anarazel.de>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>, pgsql-hackers
 <pgsql-hackers@postgresql.org>
Subject: Re: Add statistics to pg_stat_wal view for wal related parameter
 tuning
In-Reply-To: <20201222003935.47aoxfmokltlrlf2@alap3.anarazel.de>
References: <e3135545ffe792d6d636ebc40ebf022d@oss.nttdata.com>
 <e28c5f5e-363e-70c1-a200-28a8798ed8e2@oss.nttdata.com>
 <11e81f0b7d90dd69077d3d964a9a4da9@oss.nttdata.com>
 <79620b5c-ecf1-3d4f-80b5-7e311b7a0662@oss.nttdata.com>
 <89de5056627ebdd144967959bd789bd6@oss.nttdata.com>
 <58ccc451-8429-ce44-f0b7-773c8dfedf45@oss.nttdata.com>
 <7eb4ebd487b53200bde98ba2f0a7e789@oss.nttdata.com>
 <f9197102-4cb4-dfb6-02a4-b8e237c7699f@oss.nttdata.com>
 <451365b7-9e22-211e-428f-fc90ebc715bc@oss.nttdata.com>
 <20201221211650.k7b53tcnadrciqjo@alap3.anarazel.de>
 <20201222003935.47aoxfmokltlrlf2@alap3.anarazel.de>
Message-ID: <dfc63678f9c55bd4fc5d7f15c0a631ad@oss.nttdata.com>
User-Agent: Roundcube Webmail/1.1.9
Precedence: bulk

Thanks for your comments.

On 2020-12-22 09:39, Andres Freund wrote:
> Hi,
> 
> On 2020-12-21 13:16:50 -0800, Andres Freund wrote:
>> On 2020-12-02 13:52:43 +0900, Fujii Masao wrote:
>> > Pushed. Thanks!
>> 
>> Why are wal_records/fpi long, instead of uint64?
>> 	long		wal_records;	/* # of WAL records produced */
>> 	long		wal_fpi;		/* # of WAL full page images produced */
>> 	uint64		wal_bytes;		/* size of WAL records produced */
>> 
>> long is only 4 byte e.g. on windows, and it is entirely possible to 
>> wrap
>> a 4 byte record counter. It's also somewhat weird that wal_bytes is
>> unsigned, but the others are signed?
>> 
>> This is made doubly weird because on the SQL level you chose to make
>> wal_records, wal_fpi bigint. And wal_bytes numeric?

I'm sorry I don't know the reason.

The following thread is related to the patch and the type of wal_bytes
is changed from long to uint64 because XLogRecPtr is uint64.
https://www.postgresql.org/message-id/flat/20200402144438.GF64485%40nol#1f0127c98df430104c63426fdc285c20

I assumed that the reason why the type of wal_records/fpi is long
is BufferUsage have the members (i.e, shared_blks_hit) of the same 
types.

So, I think it's better if to change the type of wal_records/fpi from 
long to uint64,
to change the types of BufferUsage's members too.


> Some more things:
> - There's both PgStat_MsgWal WalStats; and static PgStat_WalStats 
> walStats;
>   that seems *WAY* too confusing. And the former imo shouldn't be
>   global.

Sorry for the confusing name.
I referenced the following variable name.

  static PgStat_MsgSLRU SLRUStats[SLRU_NUM_ELEMENTS];
  static PgStat_SLRUStats slruStats[SLRU_NUM_ELEMENTS];

How about to change from "PgStat_MsgWal WalStats"
to "PgStat_MsgWal MsgWalStats"?

Is it better to change the following name too?
  "PgStat_MsgBgWriter BgWriterStats;"
  "static PgStat_MsgSLRU SLRUStats[SLRU_NUM_ELEMENTS];"

Since PgStat_MsgWal is called in xlog.c and pgstat.c,
I thought it's should be global.

> - AdvanceXLInsertBuffer() does WalStats.m_wal_buffers_full, but as far
>   as I can tell there's nothing actually sending that?

IIUC, when pgstat_send_wal() is called by backends and so on,
it is sent to the statistic collector and it is exposed via pg_stat_wal 
view.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION