Fix comments related to pending statistics
Hi hackers,
while working on [1]/messages/by-id/ZlGYokUIlERemvpB@ip-10-97-1-34.eu-west-3.compute.internal, I came across 2 comments in pgstat.h that I think are
not correct.
1. One linked to PgStat_TableCounts pending stats, mentioning the use of
memcmp() against zeroes to detect whether there are any stats updates to apply.
This is not true anymore as of 07e9e28b56.
2. One linked to PgStat_FunctionCounts pending stats, mentioning the use of
memcmp() against zeroes to detect whether there are any pending stats: I think
it has never been the case.
Please find attached a patch to fix those comments.
[1]: /messages/by-id/ZlGYokUIlERemvpB@ip-10-97-1-34.eu-west-3.compute.internal
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Fix-comments-related-to-pending-statistics.patchtext/x-diff; charset=us-asciiDownload
From a7a026fa0f183bf4d66d85ea05463b69422d20a8 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Tue, 10 Dec 2024 12:11:35 +0000
Subject: [PATCH v1] Fix comments related to pending statistics
The comment linked to the PgStat_TableCounts pending stats mentioning the use of
memcmp() against zeroes to detect whether there are any stats updates to apply
is not true anymore as of 07e9e28b56.
The one linked to memcmp() usage for the PgStat_FunctionCounts pending stats has
probably never been correct.
---
src/include/pgstat.h | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
100.0% src/include/
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 59c28b4aca..795e45653e 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -128,8 +128,8 @@ typedef int64 PgStat_Counter;
/* ----------
* PgStat_FunctionCounts The actual per-function counts kept by a backend
*
- * This struct should contain only actual event counters, because we memcmp
- * it against zeroes to detect whether there are any pending stats.
+ * This struct should contain only actual event counters, because pending stats
+ * always has non-zero content.
*
* Note that the time counters are in instr_time format here. We convert to
* microseconds in PgStat_Counter format when flushing out pending statistics.
@@ -172,8 +172,9 @@ typedef struct PgStat_BackendSubEntry
/* ----------
* PgStat_TableCounts The actual per-table counts kept by a backend
*
- * This struct should contain only actual event counters, because we memcmp
- * it against zeroes to detect whether there are any stats updates to apply.
+ * This struct should contain only actual event counters, because we make use
+ * of pg_memory_is_all_zeros() to detect whether there are any stats updates to
+ * apply.
* It is a component of PgStat_TableStatus (within-backend state).
*
* Note: for a table, tuples_returned is the number of tuples successfully
--
2.34.1
On Tue, Dec 10, 2024 at 02:16:14PM +0000, Bertrand Drouvot wrote:
1. One linked to PgStat_TableCounts pending stats, mentioning the use of
memcmp() against zeroes to detect whether there are any stats updates to apply.This is not true anymore as of 07e9e28b56.
Oops, you're right. The comment has missed the memo about this
mention to memcpy().
2. One linked to PgStat_FunctionCounts pending stats, mentioning the use of
memcmp() against zeroes to detect whether there are any pending stats: I think
it has never been the case.
* PgStat_FunctionCounts The actual per-function counts kept by a backend
*
- * This struct should contain only actual event counters, because we memcmp
- * it against zeroes to detect whether there are any pending stats.
+ * This struct should contain only actual event counters, because pending stats
+ * always has non-zero content.pgstat_function_flush_cb() in pgstat_function.c says since
5891c7a8ed8f, so that seems like a remnant from the original patch
that has introduced this inconsistency across its reviews:
"/* localent always has non-zero content */"
Your suggestion does not look completely right to me. There is
nothing preventing us from using something else than event counters
since we don't use memcpy() and there is no comparison work, no? It
seems to me that we could remove the entire sentence instead.
--
Michael
Hi,
On Wed, Dec 11, 2024 at 02:56:08PM +0900, Michael Paquier wrote:
On Tue, Dec 10, 2024 at 02:16:14PM +0000, Bertrand Drouvot wrote:
2. One linked to PgStat_FunctionCounts pending stats, mentioning the use of
memcmp() against zeroes to detect whether there are any pending stats: I think
it has never been the case.* PgStat_FunctionCounts The actual per-function counts kept by a backend * - * This struct should contain only actual event counters, because we memcmp - * it against zeroes to detect whether there are any pending stats. + * This struct should contain only actual event counters, because pending stats + * always has non-zero content.pgstat_function_flush_cb() in pgstat_function.c says since
5891c7a8ed8f, so that seems like a remnant from the original patch
that has introduced this inconsistency across its reviews:
"/* localent always has non-zero content */"
Thanks for looking at it!
Yeah and same in pgstat_subscription_flush_cb().
Your suggestion does not look completely right to me. There is
nothing preventing us from using something else than event counters
since we don't use memcpy() and there is no comparison work, no? It
seems to me that we could remove the entire sentence instead.
Do you mean also remove the comments in pgstat_function_flush_cb() and
pgstat_subscription_flush_cb()? Those comments look fine to me given
the places where those pending entries are created meaning in
pgstat_init_function_usage() for the functions and pgstat_report_subscription_error()
and pgstat_report_subscription_conflict() for the subscriptions.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Wed, Dec 11, 2024 at 07:32:38AM +0000, Bertrand Drouvot wrote:
On Wed, Dec 11, 2024 at 02:56:08PM +0900, Michael Paquier wrote:
Your suggestion does not look completely right to me. There is
nothing preventing us from using something else than event counters
since we don't use memcpy() and there is no comparison work, no? It
seems to me that we could remove the entire sentence instead.Do you mean also remove the comments in pgstat_function_flush_cb() and
pgstat_subscription_flush_cb()? Those comments look fine to me given
the places where those pending entries are created meaning in
pgstat_init_function_usage() for the functions and pgstat_report_subscription_error()
and pgstat_report_subscription_conflict() for the subscriptions.
My apologies for the confusion. I see no problem with the existing
comments in pgstat_subscription_flush_cb() and
pgstat_function_flush_cb() because they still apply. The comment in
pgstat.h
The only thing we should do here is to remove the comment for
PgStat_FunctionCounts because we could add pointers or something else
than plain counters in this structure, and fix the comment of
PgStat_TableCounts in the lines of what you are suggesting.
--
Michael
Hi,
On Thu, Dec 12, 2024 at 09:20:15AM +0900, Michael Paquier wrote:
On Wed, Dec 11, 2024 at 07:32:38AM +0000, Bertrand Drouvot wrote:
On Wed, Dec 11, 2024 at 02:56:08PM +0900, Michael Paquier wrote:
Your suggestion does not look completely right to me. There is
nothing preventing us from using something else than event counters
since we don't use memcpy() and there is no comparison work, no? It
seems to me that we could remove the entire sentence instead.Do you mean also remove the comments in pgstat_function_flush_cb() and
pgstat_subscription_flush_cb()? Those comments look fine to me given
the places where those pending entries are created meaning in
pgstat_init_function_usage() for the functions and pgstat_report_subscription_error()
and pgstat_report_subscription_conflict() for the subscriptions.My apologies for the confusion. I see no problem with the existing
comments in pgstat_subscription_flush_cb() and
pgstat_function_flush_cb() because they still apply.
got it, thanks for the clarification.
The only thing we should do here is to remove the comment for
PgStat_FunctionCounts because we could add pointers or something else
than plain counters in this structure,
I see what you mean and you are right (I misread the initial comment so updated
it wrongly). Removed in v2 attached.
and fix the comment of
PgStat_TableCounts in the lines of what you are suggesting.
Yeap. I also just realized that the same kind of comments are missing for
PgStat_BgWriterStats and PgStat_CheckpointerStats (for which we make use
of pg_memory_is_all_zeros() to detect whether there are any stats updates to
apply). Adding those in passing.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Fix-and-add-comments-related-to-pending-statistic.patchtext/x-diff; charset=us-asciiDownload
From ad7b732a799381518de93a1857e7323d4985c4e6 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Tue, 10 Dec 2024 12:11:35 +0000
Subject: [PATCH v2] Fix and add comments related to pending statistics
The comment linked to the PgStat_TableCounts pending stats mentioning the use of
memcmp() against zeroes to detect whether there are any stats updates to apply
is not true anymore as of 07e9e28b56.
The one linked to memcmp() usage for the PgStat_FunctionCounts pending stats has
probably never been correct.
Adding comments around two other structs to mention that they should also contain
actual event counters because we make use of pg_memory_is_all_zeros() to detect
whether there are any stats updates to apply.
---
src/include/pgstat.h | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
100.0% src/include/
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 59c28b4aca..ce148b9872 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -128,9 +128,6 @@ typedef int64 PgStat_Counter;
/* ----------
* PgStat_FunctionCounts The actual per-function counts kept by a backend
*
- * This struct should contain only actual event counters, because we memcmp
- * it against zeroes to detect whether there are any pending stats.
- *
* Note that the time counters are in instr_time format here. We convert to
* microseconds in PgStat_Counter format when flushing out pending statistics.
* ----------
@@ -172,8 +169,9 @@ typedef struct PgStat_BackendSubEntry
/* ----------
* PgStat_TableCounts The actual per-table counts kept by a backend
*
- * This struct should contain only actual event counters, because we memcmp
- * it against zeroes to detect whether there are any stats updates to apply.
+ * This struct should contain only actual event counters, because we make use
+ * of pg_memory_is_all_zeros() to detect whether there are any stats updates to
+ * apply.
* It is a component of PgStat_TableStatus (within-backend state).
*
* Note: for a table, tuples_returned is the number of tuples successfully
@@ -282,6 +280,11 @@ typedef struct PgStat_ArchiverStats
TimestampTz stat_reset_timestamp;
} PgStat_ArchiverStats;
+/*
+ * This struct should contain only actual event counters, because we make use
+ * of pg_memory_is_all_zeros() to detect whether there are any stats updates to
+ * apply.
+ */
typedef struct PgStat_BgWriterStats
{
PgStat_Counter buf_written_clean;
@@ -290,6 +293,11 @@ typedef struct PgStat_BgWriterStats
TimestampTz stat_reset_timestamp;
} PgStat_BgWriterStats;
+/*
+ * This struct should contain only actual event counters, because we make use
+ * of pg_memory_is_all_zeros() to detect whether there are any stats updates to
+ * apply.
+ */
typedef struct PgStat_CheckpointerStats
{
PgStat_Counter num_timed;
--
2.34.1
On Thu, Dec 12, 2024 at 05:07:23AM +0000, Bertrand Drouvot wrote:
Yeap. I also just realized that the same kind of comments are missing for
PgStat_BgWriterStats and PgStat_CheckpointerStats (for which we make use
of pg_memory_is_all_zeros() to detect whether there are any stats updates to
apply). Adding those in passing.
Good point. I've slightly tweaked a few things for consistency and
the result looked OK, so applied.
--
Michael