On 2021/05/12 19:19, Fujii Masao wrote: > > > On 2021/05/11 18:46, Masahiro Ikeda wrote: >> >> >> On 2021/05/11 16:44, Fujii Masao wrote: >>> >>> >>> On 2021/04/28 9:10, Masahiro Ikeda wrote: >>>> >>>> >>>> On 2021/04/27 21:56, Fujii Masao wrote: >>>>> >>>>> >>>>> On 2021/04/26 10:11, Masahiro Ikeda wrote: >>>>>> >>>>>> First patch has only the changes for pg_stat_wal view. >>>>>> ("v6-0001-performance-improvements-of-reporting-wal-stats-without-introducing-a-new-variable.patch") >>>>>> >>>>>> >>>>>> >>>>> >>>>> + pgWalUsage.wal_records == prevWalUsage.wal_records && >>>>> + walStats.wal_write == 0 && walStats.wal_sync == 0 && >>>>>> WalStats.m_wal_write should be checked here instead of >>>>>> walStats.wal_write? >>>> >>>> Thanks! Yes, I'll fix it. >>> >>> Thanks! >> >> Thanks for your comments! >> I fixed them. > > Thanks for updating the patch! > > if ((pgStatTabList == NULL || pgStatTabList->tsa_used == 0) && > pgStatXactCommit == 0 && pgStatXactRollback == 0 && > + pgWalUsage.wal_records == prevWalUsage.wal_records && > + WalStats.m_wal_write == 0 && WalStats.m_wal_sync == 0 && > > I'm just wondering if the above WAL activity counters need to be checked. > Maybe it's not necessary because "pgStatXactCommit == 0 && pgStatXactRollback > == 0" > is checked? IOW, is there really the case where WAL activity counters are > updated > even when both pgStatXactCommit and pgStatXactRollback are zero?
Thanks for checking. I haven't found the case yet. But, I added the condition because there is a discussion that it's safer. (It seems the mail thread chain is broken, Sorry...) I copy the discussion at the time below. https://www.postgresql.org/message-id/20210330.172843.267174731834281075.horikyota.ntt%40gmail.com >>>> 3) Doing if (memcmp(&WalStats, &all_zeroes, sizeof(PgStat_MsgWal)) == 0) >>>> just to figure out if there's been any changes isn't all that >>>> cheap. This is regularly exercised in read-only workloads too. Seems >>>> adding a boolean WalStats.have_pending = true or such would be >>>> better. >>>> 4) For plain backends pgstat_report_wal() is called by >>>> pgstat_report_stat() - but it is not checked as part of the "Don't >>>> expend a clock check if nothing to do" check at the top. It's >>>> probably rare to have pending wal stats without also passing one of >>>> the other conditions, but ... >>> >>> I added the logic to check if the stats counters are updated or not in >>> pgstat_report_stat() using not only generated wal record but also write/sync >>> counters, and it can skip to call reporting function. >> >> I removed the checking code whether the wal stats counters were updated or >> not >> in pgstat_report_stat() since I couldn't understand the valid case yet. >> pgstat_report_stat() is called by backends when the transaction is finished. >> This means that the condition seems always pass. > > Doesn't the same holds for all other counters? If you are saying that > "wal counters should be zero if all other stats counters are zero", we > need an assertion to check that and a comment to explain that. > > I personally find it safer to add the WAL-stats condition to the > fast-return check, rather than addin such assertion. > + if (pgWalUsage.wal_records != prevWalUsage.wal_records) > + { > + WalUsage walusage; > + > + /* > + * Calculate how much WAL usage counters were increased by > substracting > + * the previous counters from the current ones. Fill the results in > + * WAL stats message. > + */ > + MemSet(&walusage, 0, sizeof(WalUsage)); > + WalUsageAccumDiff(&walusage, &pgWalUsage, &prevWalUsage); > > Isn't it better to move the code "prevWalUsage = pgWalUsage" into here? > Because it's necessary only when pgWalUsage.wal_records != > prevWalUsage.wal_records. Yes, I fixed it. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
diff --git a/src/backend/executor/instrument.c b/src/backend/executor/instrument.c index 2b106d8473..355778028e 100644 --- a/src/backend/executor/instrument.c +++ b/src/backend/executor/instrument.c @@ -17,6 +17,12 @@ #include "executor/instrument.h" +/* + * Buffer and generated WAL usage counters. + * + * The counters are accumulated values. There are infrastructures + * to add the values and calculate the difference within a specific period. + */ BufferUsage pgBufferUsage; static BufferUsage save_pgBufferUsage; WalUsage pgWalUsage; diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index e7e6a2a459..1761694a5b 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -505,7 +505,7 @@ CheckpointerMain(void) pgstat_send_bgwriter(); /* Send WAL statistics to the stats collector. */ - pgstat_report_wal(); + pgstat_send_wal(true); /* * If any checkpoint flags have been set, redo the loop to handle the @@ -583,7 +583,7 @@ HandleCheckpointerInterrupts(void) BgWriterStats.m_requested_checkpoints++; ShutdownXLOG(0, 0); pgstat_send_bgwriter(); - pgstat_report_wal(); + pgstat_send_wal(true); /* Normal exit from the checkpointer is here */ proc_exit(0); /* done */ diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index e94f5f55c7..b65bcd5c9b 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -133,8 +133,8 @@ PgStat_MsgWal WalStats; /* * WAL usage counters saved from pgWALUsage at the previous call to - * pgstat_report_wal(). This is used to calculate how much WAL usage - * happens between pgstat_report_wal() calls, by substracting + * pgstat_send_wal(). This is used to calculate how much WAL usage + * happens between pgstat_send_wal() calls, by substracting * the previous counters from the current ones. */ static WalUsage prevWalUsage; @@ -852,9 +852,19 @@ pgstat_report_stat(bool disconnect) TabStatusArray *tsa; int i; - /* Don't expend a clock check if nothing to do */ + /* + * Don't expend a clock check if nothing to do. + * + * Note: regarding to WAL statistics counters, it's not enough to check + * only whether the wal record is generated or not. If a read transaction + * is executed, wal records aren't nomally generated (although HOT makes + * wal records). But, just writes and syncs the wal data may happen when + * to flush buffers. + */ if ((pgStatTabList == NULL || pgStatTabList->tsa_used == 0) && pgStatXactCommit == 0 && pgStatXactRollback == 0 && + pgWalUsage.wal_records == prevWalUsage.wal_records && + WalStats.m_wal_write == 0 && WalStats.m_wal_sync == 0 && !have_function_stats && !disconnect) return; @@ -948,7 +958,7 @@ pgstat_report_stat(bool disconnect) pgstat_send_funcstats(); /* Send WAL statistics */ - pgstat_report_wal(); + pgstat_send_wal(true); /* Finally send SLRU statistics */ pgstat_send_slru(); @@ -2918,7 +2928,7 @@ void pgstat_initialize(void) { /* - * Initialize prevWalUsage with pgWalUsage so that pgstat_report_wal() can + * Initialize prevWalUsage with pgWalUsage so that pgstat_send_wal() can * calculate how much pgWalUsage counters are increased by substracting * prevWalUsage from pgWalUsage. */ @@ -3030,44 +3040,6 @@ pgstat_send_bgwriter(void) MemSet(&BgWriterStats, 0, sizeof(BgWriterStats)); } -/* ---------- - * pgstat_report_wal() - - * - * Calculate how much WAL usage counters are increased and send - * WAL statistics to the collector. - * - * Must be called by processes that generate WAL. - * ---------- - */ -void -pgstat_report_wal(void) -{ - WalUsage walusage; - - /* - * Calculate how much WAL usage counters are increased by substracting the - * previous counters from the current ones. Fill the results in WAL stats - * message. - */ - 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; - - /* - * Send WAL stats message to the collector. - */ - if (!pgstat_send_wal(true)) - return; - - /* - * Save the current counters for the subsequent calculation of WAL usage. - */ - prevWalUsage = pgWalUsage; -} - /* ---------- * pgstat_send_wal() - * @@ -3075,24 +3047,38 @@ pgstat_report_wal(void) * * If 'force' is not set, WAL stats message is only sent if enough time has * passed since last one was sent to reach PGSTAT_STAT_INTERVAL. - * - * Return true if the message is sent, and false otherwise. * ---------- */ -bool +void pgstat_send_wal(bool force) { - /* We assume this initializes to zeroes */ - static const PgStat_MsgWal all_zeroes; static TimestampTz sendTime = 0; /* - * 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. + * First, to check the WAL stats counters were updated. + * + * Even if the 'force' is true, we don't need to send the stats if the + * counters were not updated. + * + * We can know whether the counters were updated or not to check only + * three counters. So, for performance, we don't allocate another memory + * spaces and check the all stats like pgstat_send_slru(). + * + * The current 'wal_records' is the same as the previous one means that + * wal records weren't generated. So, the counters of 'wal_fpi', + * 'wal_bytes', 'm_wal_buffers_full' are not updated neither. + * + * It's not enough to check the number of generated wal records, for + * example the walwriter may write/sync the WAL although it doesn't + * generate wal records. 'm_wal_write' and 'm_wal_sync' are zero means the + * counters of time spent are zero too. */ - if (memcmp(&WalStats, &all_zeroes, sizeof(PgStat_MsgWal)) == 0) - return false; + if (pgWalUsage.wal_records == prevWalUsage.wal_records && + WalStats.m_wal_write == 0 && WalStats.m_wal_sync == 0) + { + Assert(WalStats.m_wal_buffers_full == 0); + return; + } if (!force) { @@ -3100,13 +3086,40 @@ pgstat_send_wal(bool force) /* * Don't send a message unless it's been at least PGSTAT_STAT_INTERVAL - * msec since we last sent one. + * msec since we last sent one to avoid overloading the stats + * collector. */ if (!TimestampDifferenceExceeds(sendTime, now, PGSTAT_STAT_INTERVAL)) - return false; + return; sendTime = now; } + /* + * Set the counters related to generated WAL data if the counters were + * updated. + */ + if (pgWalUsage.wal_records != prevWalUsage.wal_records) + { + WalUsage walusage; + + /* + * Calculate how much WAL usage counters were increased by substracting + * the previous counters from the current ones. Fill the results in + * WAL stats message. + */ + 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; + + /* + * Save the current counters for the subsequent calculation of WAL usage. + */ + prevWalUsage = pgWalUsage; + } + /* * Prepare and send the message */ @@ -3117,8 +3130,6 @@ pgstat_send_wal(bool force) * Clear out the statistics buffer, so it can be re-used. */ MemSet(&WalStats, 0, sizeof(WalStats)); - - return true; } /* ---------- diff --git a/src/include/executor/instrument.h b/src/include/executor/instrument.h index fc87eed4fb..31dbfc832a 100644 --- a/src/include/executor/instrument.h +++ b/src/include/executor/instrument.h @@ -16,6 +16,12 @@ #include "portability/instr_time.h" +/* + * The accumulated counters for buffer usage. + * + * The reason the counters are accumulated values is to avoid unexpected + * reset because many callers may use them. + */ typedef struct BufferUsage { int64 shared_blks_hit; /* # of shared buffer hits */ @@ -32,6 +38,15 @@ typedef struct BufferUsage instr_time blk_write_time; /* time spent writing */ } BufferUsage; +/* + * The accumulated counters for generated WAL usage. + * + * The reason the counters are accumulated values is the same as BufferUsage's one. + * And the reason to store only generated WAL usage and doesn't store WAL I/O + * activity, is that this is assumed for knowing the WAL usage in per query or + * transaction. So, common resources for the cluster like WAL I/O activity is + * not stored. + */ typedef struct WalUsage { int64 wal_records; /* # of WAL records produced */ diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 72ff4a06d6..7727e83455 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -1091,8 +1091,7 @@ extern void pgstat_twophase_postabort(TransactionId xid, uint16 info, extern void pgstat_send_archiver(const char *xlog, bool failed); extern void pgstat_send_bgwriter(void); -extern void pgstat_report_wal(void); -extern bool pgstat_send_wal(bool force); +extern void pgstat_send_wal(bool force); /* ---------- * Support functions for the SQL-callable functions to