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
From 8bde948e5e91dbfbcf79b091af51f022aa32191a Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikeda...@oss.nttdata.com>
Date: Wed, 20 Jan 2021 12:13:24 +0900
Subject: [PATCH] Refactor variable names of global statistics messages
Refactor the variable names of global statistics messages
for bgwriter, wal, and SLRU because the names are too confusing.
Now, their names are BgWriterStats, WalStats, and SLRUStats. But,
there are alike names that are defined in the statistic collector.
Their names are walStats and slruStats. Since this is confusing,
this patch renames variable names of messages to make it easy to
understand that they are messages.
Author: Masahiro Ikeda
Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/20201222003935.47aoxfmokltlr...@alap3.anarazel.de
---
src/backend/access/transam/xlog.c | 6 ++--
src/backend/postmaster/checkpointer.c | 8 +++---
src/backend/postmaster/pgstat.c | 40 +++++++++++++--------------
src/backend/storage/buffer/bufmgr.c | 8 +++---
src/include/pgstat.h | 4 +--
5 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 470e113b33..a64ad34f21 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2209,7 +2209,7 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic)
WriteRqst.Flush = 0;
XLogWrite(WriteRqst, false);
LWLockRelease(WALWriteLock);
- WalStats.m_wal_buffers_full++;
+ MsgWalStats.m_wal_buffers_full++;
TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY_DONE();
}
/* Re-acquire WALBufMappingLock and retry */
@@ -8658,8 +8658,8 @@ LogCheckpointEnd(bool restartpoint)
CheckpointStats.ckpt_sync_end_t);
/* Accumulate checkpoint timing summary data, in milliseconds. */
- BgWriterStats.m_checkpoint_write_time += write_msecs;
- BgWriterStats.m_checkpoint_sync_time += sync_msecs;
+ MsgBgWriterStats.m_checkpoint_write_time += write_msecs;
+ MsgBgWriterStats.m_checkpoint_sync_time += sync_msecs;
/*
* All of the published timing statistics are accounted for. Only
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 54a818bf61..9735e4e163 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -360,7 +360,7 @@ CheckpointerMain(void)
if (((volatile CheckpointerShmemStruct *) CheckpointerShmem)->ckpt_flags)
{
do_checkpoint = true;
- BgWriterStats.m_requested_checkpoints++;
+ MsgBgWriterStats.m_requested_checkpoints++;
}
/*
@@ -374,7 +374,7 @@ CheckpointerMain(void)
if (elapsed_secs >= CheckPointTimeout)
{
if (!do_checkpoint)
- BgWriterStats.m_timed_checkpoints++;
+ MsgBgWriterStats.m_timed_checkpoints++;
do_checkpoint = true;
flags |= CHECKPOINT_CAUSE_TIME;
}
@@ -1257,8 +1257,8 @@ AbsorbSyncRequests(void)
LWLockAcquire(CheckpointerCommLock, LW_EXCLUSIVE);
/* Transfer stats counts into pending pgstats message */
- BgWriterStats.m_buf_written_backend += CheckpointerShmem->num_backend_writes;
- BgWriterStats.m_buf_fsync_backend += CheckpointerShmem->num_backend_fsync;
+ MsgBgWriterStats.m_buf_written_backend += CheckpointerShmem->num_backend_writes;
+ MsgBgWriterStats.m_buf_fsync_backend += CheckpointerShmem->num_backend_fsync;
CheckpointerShmem->num_backend_writes = 0;
CheckpointerShmem->num_backend_fsync = 0;
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index f75b52719d..34556920cb 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -141,8 +141,8 @@ char *pgstat_stat_tmpname = NULL;
* 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;
+PgStat_MsgBgWriter MsgBgWriterStats;
+PgStat_MsgWal MsgWalStats;
/*
* WAL usage counters saved from pgWALUsage at the previous call to
@@ -176,7 +176,7 @@ static const char *const slru_names[] = {
* to copy things around. We assume this variable inits to zeroes. Entries
* are one-to-one with slru_names[].
*/
-static PgStat_MsgSLRU SLRUStats[SLRU_NUM_ELEMENTS];
+static PgStat_MsgSLRU MsgSLRUStats[SLRU_NUM_ELEMENTS];
/* ----------
* Local data
@@ -4651,19 +4651,19 @@ pgstat_send_bgwriter(void)
* this case, avoid sending a completely empty message to the stats
* collector.
*/
- if (memcmp(&BgWriterStats, &all_zeroes, sizeof(PgStat_MsgBgWriter)) == 0)
+ if (memcmp(&MsgBgWriterStats, &all_zeroes, sizeof(PgStat_MsgBgWriter)) == 0)
return;
/*
* Prepare and send the message
*/
- pgstat_setheader(&BgWriterStats.m_hdr, PGSTAT_MTYPE_BGWRITER);
- pgstat_send(&BgWriterStats, sizeof(BgWriterStats));
+ pgstat_setheader(&MsgBgWriterStats.m_hdr, PGSTAT_MTYPE_BGWRITER);
+ pgstat_send(&MsgBgWriterStats, sizeof(MsgBgWriterStats));
/*
* Clear out the statistics buffer, so it can be re-used.
*/
- MemSet(&BgWriterStats, 0, sizeof(BgWriterStats));
+ MemSet(&MsgBgWriterStats, 0, sizeof(MsgBgWriterStats));
}
/* ----------
@@ -4688,23 +4688,23 @@ pgstat_send_wal(void)
MemSet(&walusage, 0, sizeof(WalUsage));
WalUsageAccumDiff(&walusage, &pgWalUsage, &prevWalUsage);
- WalStats.m_wal_records = walusage.wal_records;
- WalStats.m_wal_fpi = walusage.wal_fpi;
- WalStats.m_wal_bytes = walusage.wal_bytes;
+ MsgWalStats.m_wal_records = walusage.wal_records;
+ MsgWalStats.m_wal_fpi = walusage.wal_fpi;
+ MsgWalStats.m_wal_bytes = walusage.wal_bytes;
/*
* This function can be called even if nothing at all has happened. In
* this case, avoid sending a completely empty message to the stats
* collector.
*/
- if (memcmp(&WalStats, &all_zeroes, sizeof(PgStat_MsgWal)) == 0)
+ if (memcmp(&MsgWalStats, &all_zeroes, sizeof(PgStat_MsgWal)) == 0)
return;
/*
* Prepare and send the message
*/
- pgstat_setheader(&WalStats.m_hdr, PGSTAT_MTYPE_WAL);
- pgstat_send(&WalStats, sizeof(WalStats));
+ pgstat_setheader(&MsgWalStats.m_hdr, PGSTAT_MTYPE_WAL);
+ pgstat_send(&MsgWalStats, sizeof(MsgWalStats));
/*
* Save the current counters for the subsequent calculation of WAL usage.
@@ -4714,7 +4714,7 @@ pgstat_send_wal(void)
/*
* Clear out the statistics buffer, so it can be re-used.
*/
- MemSet(&WalStats, 0, sizeof(WalStats));
+ MemSet(&MsgWalStats, 0, sizeof(MsgWalStats));
}
/* ----------
@@ -4736,22 +4736,22 @@ pgstat_send_slru(void)
* this case, avoid sending a completely empty message to the stats
* collector.
*/
- if (memcmp(&SLRUStats[i], &all_zeroes, sizeof(PgStat_MsgSLRU)) == 0)
+ if (memcmp(&MsgSLRUStats[i], &all_zeroes, sizeof(PgStat_MsgSLRU)) == 0)
continue;
/* set the SLRU type before each send */
- SLRUStats[i].m_index = i;
+ MsgSLRUStats[i].m_index = i;
/*
* Prepare and send the message
*/
- pgstat_setheader(&SLRUStats[i].m_hdr, PGSTAT_MTYPE_SLRU);
- pgstat_send(&SLRUStats[i], sizeof(PgStat_MsgSLRU));
+ pgstat_setheader(&MsgSLRUStats[i].m_hdr, PGSTAT_MTYPE_SLRU);
+ pgstat_send(&MsgSLRUStats[i], sizeof(PgStat_MsgSLRU));
/*
* Clear out the statistics buffer, so it can be re-used.
*/
- MemSet(&SLRUStats[i], 0, sizeof(PgStat_MsgSLRU));
+ MemSet(&MsgSLRUStats[i], 0, sizeof(PgStat_MsgSLRU));
}
}
@@ -7361,7 +7361,7 @@ slru_entry(int slru_idx)
Assert((slru_idx >= 0) && (slru_idx < SLRU_NUM_ELEMENTS));
- return &SLRUStats[slru_idx];
+ return &MsgSLRUStats[slru_idx];
}
/*
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 561c212092..c7a28078eb 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -2059,7 +2059,7 @@ BufferSync(int flags)
if (SyncOneBuffer(buf_id, false, &wb_context) & BUF_WRITTEN)
{
TRACE_POSTGRESQL_BUFFER_SYNC_WRITTEN(buf_id);
- BgWriterStats.m_buf_written_checkpoints++;
+ MsgBgWriterStats.m_buf_written_checkpoints++;
num_written++;
}
}
@@ -2169,7 +2169,7 @@ BgBufferSync(WritebackContext *wb_context)
strategy_buf_id = StrategySyncStart(&strategy_passes, &recent_alloc);
/* Report buffer alloc counts to pgstat */
- BgWriterStats.m_buf_alloc += recent_alloc;
+ MsgBgWriterStats.m_buf_alloc += recent_alloc;
/*
* If we're not running the LRU scan, just stop after doing the stats
@@ -2359,7 +2359,7 @@ BgBufferSync(WritebackContext *wb_context)
reusable_buffers++;
if (++num_written >= bgwriter_lru_maxpages)
{
- BgWriterStats.m_maxwritten_clean++;
+ MsgBgWriterStats.m_maxwritten_clean++;
break;
}
}
@@ -2367,7 +2367,7 @@ BgBufferSync(WritebackContext *wb_context)
reusable_buffers++;
}
- BgWriterStats.m_buf_written_clean += num_written;
+ MsgBgWriterStats.m_buf_written_clean += num_written;
#ifdef BGW_DEBUG
elog(DEBUG1, "bgwriter: recent_alloc=%u smoothed=%.2f delta=%ld ahead=%d density=%.2f reusable_est=%d upcoming_est=%d scanned=%d wrote=%d reusable=%d",
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 724068cf87..f65dfc8569 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -1375,12 +1375,12 @@ extern char *pgstat_stat_filename;
/*
* BgWriter statistics counters are updated directly by bgwriter and bufmgr
*/
-extern PgStat_MsgBgWriter BgWriterStats;
+extern PgStat_MsgBgWriter MsgBgWriterStats;
/*
* WAL statistics counter is updated by backends and background processes
*/
-extern PgStat_MsgWal WalStats;
+extern PgStat_MsgWal MsgWalStats;
/*
* Updated by pgstat_count_buffer_*_time macros
--
2.25.1
