On 2021-05-13 09:05, Masahiro Ikeda wrote:
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,
Thanks for updating the patch!
+ * is executed, wal records aren't nomally generated (although HOT
makes
nomally -> normally?
+ * 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
You use both 'wal' and 'WAL' in the comments, but are there any
intension?
Regards,
