Add callback in pgstats for backend initialization
Hi all,
Currently, the backend-level initialization of pgstats happens in
pgstat_initialize(), where we are using a shortcut for the WAL stats,
with pgstat_init_wal().
I'd like to propose to add a callback for that, so as in-core or
custom pgstats kinds can assign custom actions when initializing a
backend. The use-case I had for this one are pretty close to what we
do for WAL, where we would rely on a difference of state depending on
what may have existed before reaching the initialization path. So
this can offer more precision. Another case, perhaps less
interesting, is to be able to initialize some static backend state.
I wanted to get that sent for the current commit fest, but did not get
back in time to it. Anyway, here it is. This gives the simple patch
attached.
Thanks,
--
Michael
Attachments:
0001-Add-callback-for-backend-initialization-in-pgstats.patchtext/x-diff; charset=us-asciiDownload
From 1e41a5155fb689f1d4ae8a5cfb244b824e913bae Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 3 Sep 2024 10:38:15 +0900
Subject: [PATCH] Add callback for backend initialization in pgstats
This is currently used by the WAL stats, and is now made available for
others as well.
---
src/include/utils/pgstat_internal.h | 7 +++++++
src/backend/utils/activity/pgstat.c | 14 ++++++++++++--
src/backend/utils/activity/pgstat_wal.c | 2 +-
3 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index fb132e439d..53f7d5c98c 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -229,6 +229,12 @@ typedef struct PgStat_KindInfo
*/
uint32 pending_size;
+ /*
+ * Perform custom actions when initializing a backend (standalone or under
+ * postmaster). Optional.
+ */
+ void (*init_backend_cb) (void);
+
/*
* For variable-numbered stats: flush pending stats. Required if pending
* data is used.
@@ -676,6 +682,7 @@ extern bool pgstat_flush_wal(bool nowait);
extern void pgstat_init_wal(void);
extern bool pgstat_have_pending_wal(void);
+extern void pgstat_wal_init_backend_cb(void);
extern void pgstat_wal_init_shmem_cb(void *stats);
extern void pgstat_wal_reset_all_cb(TimestampTz ts);
extern void pgstat_wal_snapshot_cb(void);
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index b2ca3f39b7..3b6983ff5d 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -441,6 +441,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_off = offsetof(PgStatShared_Wal, stats),
.shared_data_len = sizeof(((PgStatShared_Wal *) 0)->stats),
+ .init_backend_cb = pgstat_wal_init_backend_cb,
.init_shmem_cb = pgstat_wal_init_shmem_cb,
.reset_all_cb = pgstat_wal_reset_all_cb,
.snapshot_cb = pgstat_wal_snapshot_cb,
@@ -604,10 +605,19 @@ pgstat_initialize(void)
pgstat_attach_shmem();
- pgstat_init_wal();
-
pgstat_init_snapshot_fixed();
+ /* Backend initialization callbacks */
+ for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
+ {
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+ if (kind_info == NULL || kind_info->init_backend_cb == NULL)
+ continue;
+
+ kind_info->init_backend_cb();
+ }
+
/* Set up a process-exit hook to clean up */
before_shmem_exit(pgstat_shutdown_hook, 0);
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index e2a3f6b865..8c19c3f2fd 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -138,7 +138,7 @@ pgstat_flush_wal(bool nowait)
}
void
-pgstat_init_wal(void)
+pgstat_wal_init_backend_cb(void)
{
/*
* Initialize prevWalUsage with pgWalUsage so that pgstat_flush_wal() can
--
2.45.2
Hi,
On Tue, Sep 03, 2024 at 10:52:20AM +0900, Michael Paquier wrote:
Hi all,
Currently, the backend-level initialization of pgstats happens in
pgstat_initialize(), where we are using a shortcut for the WAL stats,
with pgstat_init_wal().I'd like to propose to add a callback for that, so as in-core or
custom pgstats kinds can assign custom actions when initializing a
backend. The use-case I had for this one are pretty close to what we
do for WAL, where we would rely on a difference of state depending on
what may have existed before reaching the initialization path. So
this can offer more precision. Another case, perhaps less
interesting, is to be able to initialize some static backend state.
I think the proposal makes sense and I can see the use cases too, so +1.
This gives the simple patch
attached.
Should we add a few words about this new callback (and the others in
PgStat_KindInfo while at it) in the "Custom Cumulative Statistics" section of
xfunc.sgml?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Tue, Sep 03, 2024 at 05:00:54AM +0000, Bertrand Drouvot wrote:
Should we add a few words about this new callback (and the others in
PgStat_KindInfo while at it) in the "Custom Cumulative Statistics" section of
xfunc.sgml?
Not sure if it is worth having. This adds extra maintenance and
there's already a mention of src/include/utils/pgstat_internal.h
telling where the callbacks are described.
--
Michael
At Tue, 3 Sep 2024 05:00:54 +0000, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote in
Hi,
On Tue, Sep 03, 2024 at 10:52:20AM +0900, Michael Paquier wrote:
Hi all,
Currently, the backend-level initialization of pgstats happens in
pgstat_initialize(), where we are using a shortcut for the WAL stats,
with pgstat_init_wal().I'd like to propose to add a callback for that, so as in-core or
custom pgstats kinds can assign custom actions when initializing a
backend. The use-case I had for this one are pretty close to what we
do for WAL, where we would rely on a difference of state depending on
what may have existed before reaching the initialization path. So
this can offer more precision. Another case, perhaps less
interesting, is to be able to initialize some static backend state.I think the proposal makes sense and I can see the use cases too, so +1.
+1, too.
The name "init_backend" makes it sound like the function initializes
the backend. backend_init might be a better choice, but I'm not sure.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Sep 04, 2024 at 02:15:43PM +0900, Kyotaro Horiguchi wrote:
The name "init_backend" makes it sound like the function initializes
the backend. backend_init might be a better choice, but I'm not sure.
We (kind of) tend to prefer $verb_$thing-or-action_cb for the name of
the callbacks, which is why I chose this naming. If you feel strongly
about "backend_init_cb", that's also fine by me; you are the original
author of this code area with Andres.
--
Michael
At Wed, 4 Sep 2024 15:04:09 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Wed, Sep 04, 2024 at 02:15:43PM +0900, Kyotaro Horiguchi wrote:
The name "init_backend" makes it sound like the function initializes
the backend. backend_init might be a better choice, but I'm not sure.We (kind of) tend to prefer $verb_$thing-or-action_cb for the name of
the callbacks, which is why I chose this naming. If you feel strongly
about "backend_init_cb", that's also fine by me; you are the original
author of this code area with Andres.
More accurately, I believe this applies when the sentence follows a
verb-object structure. In this case, the function’s meaning is “pgstat
initialization on backend,” which seems somewhat different from the
policy you mentioned. However, it could also be interpreted as
“initialize backend status related to pgstat.” Either way, I’m okay
with the current name if you are, based on the discussion above.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Sep 04, 2024 at 06:42:33PM +0900, Kyotaro Horiguchi wrote:
More accurately, I believe this applies when the sentence follows a
verb-object structure. In this case, the function’s meaning is “pgstat
initialization on backend,” which seems somewhat different from the
policy you mentioned. However, it could also be interpreted as
“initialize backend status related to pgstat.” Either way, I’m okay
with the current name if you are, based on the discussion above.
Not sure which one is better than the other, TBH. Either way, we
still have a full release cycle to finalize that, and I am OK to
switch the name to something else if there are more folks in favor of
one, the other or even something entirely different but descriptive
enough.
--
Michael
On Thu, Sep 05, 2024 at 12:41:09PM +0900, Michael Paquier wrote:
Not sure which one is better than the other, TBH. Either way, we
still have a full release cycle to finalize that, and I am OK to
switch the name to something else if there are more folks in favor of
one, the other or even something entirely different but descriptive
enough.
I've used init_backend_cb at the end in 1b373aed20e6. A change would
be OK by me, if needed.
--
Michael