Re: Add statistics to pg_stat_wal view for wal related parameter tuning

On 2020-12-22 11:16, Masahiro Ikeda wrote:
> 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.

I've done a little more research so I'll share the results.

IUCC, theoretically this leads to caliculate the statistics less,
but actually, it's not happened.

The above "wal_records", "wal_fpi" are accumulation values and when
WalUsageAccumDiff()
is called, we can know how many wals are generated for specific
executions,
for example, planning/executing a query, processing a utility command,
and vacuuming one relation.

The following variable has accumulated "wal_records" and "wal_fpi" per
process.

```
typedef struct WalUsage
{
long wal_records; /* # of WAL records produced */
long wal_fpi; /* # of WAL full page images produced */
uint64 wal_bytes; /* size of WAL records produced */
} WalUsage;

WalUsage pgWalUsage;
```

Although this may be overflow, it doesn't affect to caliculate the
difference
of wal usage between some execution points. If to generate over 2
billion wal
records per executions, 4 bytes is not enough and collected statistics
will be
lost, but I think it's not happened.

In addition, "wal_records" and "wal_fpi" values sent by processes are
accumulated in the statistic collector and their types are
PgStat_Counter(int64).

```
typedef struct PgStat_WalStats
{
PgStat_Counter wal_records;
PgStat_Counter wal_fpi;
uint64 wal_bytes;
PgStat_Counter wal_buffers_full;
TimestampTz stat_reset_timestamp;
} PgStat_WalStats;
```

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

I made an attached patch to rename the above variable names.
What do you think?

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION