public inbox for [email protected]
help / color / mirror / Atom feedFrom: Sami Imseih <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: [Proposal] Adding callback support for custom statistics kinds
Date: Wed, 22 Oct 2025 15:24:11 -0500
Message-ID: <CAA5RZ0s9SDOu+Z6veoJCHWk+kDeTktAtC-KY9fQ9Z6BJdDUirQ@mail.gmail.com> (raw)
Hi,
I'd like to propose $SUBJECT to serialize additional per-entry data beyond
the standard statistics entries. Currently, custom statistics kinds can store
their standard entry data in the main "pgstat.stat" file, but there is no
mechanism for extensions to persist extra data stored in the entry. A common
use case is extensions that register a custom kind and, besides
standard counters,
need to track variable-length data stored in a dsa_pointer.
This proposal adds optional "to_serialized_extra" and
"from_serialized_extra" callbacks to "PgStat_KindInfo" that allow custom kinds
to write and read from extra data in a separate files
(pgstat.<kind>.stat). The callbacks
give extensions direct access to the file pointer so they can read and write
data in any format, while the core "pgstat" infrastructure manages
opening, closing, renaming, and cleanup, just as it does with "pgstat.stat".
A concrete use case is pg_stat_statements. If it were to use custom
stats kinds to track statement counters, it could also track query text
stored in DSA. The callbacks allow saving the query text referenced by the
dsa_pointer and restoring it after a clean shutdown. Since DSA
(and more specifically DSM) cannot be attached by the postmaster, an
extension cannot use "on_shmem_exit" or "shmem_startup_hook"
to serialize or restore this data. This is why pgstat handles
serialization during checkpointer shutdown and startup, allowing a single
backend to manage it safely.
I considered adding hooks to the existing pgstat code paths
(pgstat_before_server_shutdown, pgstat_discard_stats, and
pgstat_restore_stats), but that felt too unrestricted. Using per-kind
callbacks provides more control.
There are already "to_serialized_name" and "from_serialized_name"
callbacks used to store and read entries by "name" instead of
"PgStat_HashKey", currently used by replication slot stats. Those
remain unchanged, as they serve a separate purpose.
Other design points:
1. Filenames use "pgstat.<kind>.stat" based on the numeric kind ID.
This avoids requiring extensions to provide names and prevents issues
with spaces or special characters.
2. Both callbacks must be registered together. Serializing without
deserializing would leave orphaned files behind, and I cannot think of a
reason to allow this.
3. "write_chunk", "read_chunk", "write_chunk_s", and
"read_chunk_s" are renamed to "pgstat_write_chunk", etc., and
moved to "pgstat_internal.h" so extensions can use them without
re-implementing these functions.
4. These callbacks are valid only for custom, variable-numbered statistics
kinds. Custom fixed kinds may not benefit, but could be considered in the
future.
Attached 0001 is the proposed change, still in POC form. The second patch
contains tests in "injection_points" to demonstrate this proposal, and is not
necessarily intended for commit.
Looking forward to your feedback!
--
Sami Imseih
Amazon Web Services (AWS)
Attachments:
[application/octet-stream] 0002-test-custom-stats-callbacks.patch (14.4K, ../CAA5RZ0s9SDOu+Z6veoJCHWk+kDeTktAtC-KY9fQ9Z6BJdDUirQ@mail.gmail.com/2-0002-test-custom-stats-callbacks.patch)
download | inline diff:
From c4a26749dc69ba546c5c5a942afbcf5684189b01 Mon Sep 17 00:00:00 2001
From: Ubuntu <[email protected]>
Date: Wed, 22 Oct 2025 18:58:18 +0000
Subject: [PATCH 2/2] test custom stats callbacks
---
.../injection_points--1.0.sql | 14 +-
.../injection_points/injection_points.c | 8 +-
.../injection_points/injection_stats.c | 177 +++++++++++++++++-
.../injection_points/injection_stats.h | 3 +-
.../modules/injection_points/t/001_stats.pl | 86 +++++++++
5 files changed, 283 insertions(+), 5 deletions(-)
diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
index a7b61fbdfe6..0486eea4aee 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -9,10 +9,20 @@
-- Attaches the action to the given injection point.
--
CREATE FUNCTION injection_points_attach(IN point_name TEXT,
- IN action text)
+ IN action text, IN description text DEFAULT NULL)
RETURNS void
AS 'MODULE_PATHNAME', 'injection_points_attach'
-LANGUAGE C STRICT PARALLEL UNSAFE;
+LANGUAGE C PARALLEL UNSAFE;
+
+--
+-- injection_points_stats_description()
+--
+-- Reports statistics, if any, related to the given injection point.
+--
+CREATE FUNCTION injection_points_stats_description(IN point_name TEXT)
+RETURNS TEXT
+AS 'MODULE_PATHNAME', 'injection_points_stats_description'
+LANGUAGE C STRICT;
--
-- injection_points_load()
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 31138301117..a2356836f7f 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -353,9 +353,15 @@ injection_points_attach(PG_FUNCTION_ARGS)
{
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
char *action = text_to_cstring(PG_GETARG_TEXT_PP(1));
+ char *description = NULL; /* optional description */
char *function;
InjectionPointCondition condition = {0};
+ if (PG_NARGS() > 2 && !PG_ARGISNULL(2))
+ {
+ description = text_to_cstring(PG_GETARG_TEXT_PP(2));
+ }
+
if (strcmp(action, "error") == 0)
function = "injection_error";
else if (strcmp(action, "notice") == 0)
@@ -386,7 +392,7 @@ injection_points_attach(PG_FUNCTION_ARGS)
}
/* Add entry for stats */
- pgstat_create_inj(name);
+ pgstat_create_inj(name, description);
PG_RETURN_VOID();
}
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index 158e1631af9..d15ef65fb39 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -19,6 +19,7 @@
#include "common/hashfn.h"
#include "injection_stats.h"
#include "pgstat.h"
+#include "storage/dsm_registry.h"
#include "utils/builtins.h"
#include "utils/pgstat_internal.h"
@@ -26,6 +27,7 @@
typedef struct PgStat_StatInjEntry
{
PgStat_Counter numcalls; /* number of times point has been run */
+ dsa_pointer description; /* injection point description */
} PgStat_StatInjEntry;
typedef struct PgStatShared_InjectionPoint
@@ -35,6 +37,12 @@ typedef struct PgStatShared_InjectionPoint
} PgStatShared_InjectionPoint;
static bool injection_stats_flush_cb(PgStat_EntryRef *entry_ref, bool nowait);
+void injection_stats_serialize_extra(const PgStat_HashKey *key,
+ const PgStatShared_Common *header,
+ FILE *fd);
+bool injection_stats_deserialize_extra(PgStat_HashKey *key,
+ const PgStatShared_Common *header,
+ FILE *fd);
static const PgStat_KindInfo injection_stats = {
.name = "injection_points",
@@ -50,6 +58,8 @@ static const PgStat_KindInfo injection_stats = {
.shared_data_len = sizeof(((PgStatShared_InjectionPoint *) 0)->stats),
.pending_size = sizeof(PgStat_StatInjEntry),
.flush_pending_cb = injection_stats_flush_cb,
+ .to_serialized_extra = injection_stats_serialize_extra,
+ .from_serialized_extra = injection_stats_deserialize_extra,
};
/*
@@ -65,6 +75,9 @@ static const PgStat_KindInfo injection_stats = {
/* Track if stats are loaded */
static bool inj_stats_loaded = false;
+/* DSA area to store an injection points description */
+dsa_area *inj_description_dsa = NULL;
+
/*
* Callback for stats handling
*/
@@ -122,10 +135,13 @@ pgstat_register_inj(void)
* Report injection point creation.
*/
void
-pgstat_create_inj(const char *name)
+pgstat_create_inj(const char *name, const char *description)
{
PgStat_EntryRef *entry_ref;
PgStatShared_InjectionPoint *shstatent;
+ size_t len;
+ char *desc_copy;
+ bool found;
/* leave if disabled */
if (!inj_stats_loaded || !inj_stats_enabled)
@@ -138,6 +154,25 @@ pgstat_create_inj(const char *name)
/* initialize shared memory data */
memset(&shstatent->stats, 0, sizeof(shstatent->stats));
+
+ if (!description)
+ return;
+
+ len = strlen(description) + 1;
+
+ if (!inj_description_dsa)
+ inj_description_dsa = GetNamedDSA("injection_points_description", &found);
+
+ if (inj_description_dsa)
+ {
+ shstatent->stats.description = dsa_allocate(inj_description_dsa, len);
+
+ desc_copy = dsa_get_address(inj_description_dsa,
+ shstatent->stats.description);
+ desc_copy[len - 1] = '\0';
+
+ memcpy(desc_copy, description, len);
+ }
}
/*
@@ -180,6 +215,146 @@ pgstat_report_inj(const char *name)
pending->numcalls++;
}
+void
+injection_stats_serialize_extra(const PgStat_HashKey *key,
+ const PgStatShared_Common *header,
+ FILE *fd)
+{
+ char *description;
+ size_t qlen;
+ PgStatShared_InjectionPoint *entry = (PgStatShared_InjectionPoint *) header;
+
+ /* Exit early if stats aren't available or enabled */
+ if (!inj_stats_loaded || !inj_stats_enabled || !key)
+ return;
+
+ /* Write hash key */
+ pgstat_write_chunk(fd, (void *) key, sizeof(PgStat_HashKey));
+
+ /* Handle missing description */
+ if (!DsaPointerIsValid(entry->stats.description))
+ {
+ fputc('\0', fd);
+ return;
+ }
+
+ /* Ensure DSA area is loaded */
+ if (!inj_description_dsa)
+ {
+ bool found;
+
+ inj_description_dsa = GetNamedDSA("injection_points_description", &found);
+ }
+
+ if (!inj_description_dsa)
+ {
+ fputc('\0', fd);
+ return;
+ }
+
+ /* Get description and write it */
+ description = dsa_get_address(inj_description_dsa, entry->stats.description);
+ qlen = strlen(description) + 1; /* include null terminator */
+
+ pgstat_write_chunk(fd, description, qlen);
+}
+
+bool
+injection_stats_deserialize_extra(PgStat_HashKey *key,
+ const PgStatShared_Common *header,
+ FILE *fd)
+{
+ PgStatShared_InjectionPoint *entry;
+ dsa_pointer dp;
+ size_t bufcap;
+ size_t len;
+ char *buffer;
+ int c;
+
+ if (!inj_stats_loaded || !inj_stats_enabled)
+ return true;
+
+ /* Read the key */
+ if (!pgstat_read_chunk(fd, (void *) key, sizeof(PgStat_HashKey)))
+ return feof(fd) ? true : false;
+
+ /* Ensure DSA is ready */
+ if (!inj_description_dsa)
+ {
+ bool found;
+
+ inj_description_dsa = GetNamedDSA("injection_points_description", &found);
+ }
+
+ entry =
+ (PgStatShared_InjectionPoint *) header;
+
+ /* Read null-terminated description */
+ bufcap = 128;
+ len = 0;
+ buffer = palloc(bufcap);
+
+ while ((c = fgetc(fd)) != EOF)
+ {
+ if (len + 1 >= bufcap)
+ {
+ bufcap *= 2;
+ buffer = repalloc(buffer, bufcap);
+ }
+
+ buffer[len++] = (char) c;
+
+ if (c == '\0')
+ break;
+ }
+
+ /* EOF reached unexpectedly */
+ if (c == EOF)
+ {
+ pfree(buffer);
+ return false;
+ }
+
+ /* Copy into DSA */
+ dp = dsa_allocate(inj_description_dsa, len);
+
+ memcpy(dsa_get_address(inj_description_dsa, dp), buffer, len);
+ entry->stats.description = dp;
+
+ pfree(buffer);
+ return true;
+}
+
+PG_FUNCTION_INFO_V1(injection_points_stats_description);
+Datum
+injection_points_stats_description(PG_FUNCTION_ARGS)
+{
+ char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ PgStat_StatInjEntry *entry = pgstat_fetch_stat_injentry(name);
+
+ if (entry == NULL)
+ PG_RETURN_NULL();
+
+ if (DsaPointerIsValid(entry->description))
+ {
+ char *description = NULL;
+ bool found;
+
+ if (!inj_description_dsa)
+ inj_description_dsa = GetNamedDSA("injection_points_description", &found);
+
+ if (inj_description_dsa)
+ description = dsa_get_address(inj_description_dsa, entry->description);
+
+ if (!description)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(cstring_to_text(description));
+ }
+ else
+ PG_RETURN_NULL();
+}
+
/*
* SQL function returning the number of times an injection point
* has been called.
diff --git a/src/test/modules/injection_points/injection_stats.h b/src/test/modules/injection_points/injection_stats.h
index ba310c52c7f..e50a256d91e 100644
--- a/src/test/modules/injection_points/injection_stats.h
+++ b/src/test/modules/injection_points/injection_stats.h
@@ -20,7 +20,8 @@ extern bool inj_stats_enabled;
/* injection_stats.c */
extern void pgstat_register_inj(void);
-extern void pgstat_create_inj(const char *name);
+extern void pgstat_create_inj(const char *name,
+ const char *description);
extern void pgstat_drop_inj(const char *name);
extern void pgstat_report_inj(const char *name);
diff --git a/src/test/modules/injection_points/t/001_stats.pl b/src/test/modules/injection_points/t/001_stats.pl
index 47ab58d0e9b..ac3961320a1 100644
--- a/src/test/modules/injection_points/t/001_stats.pl
+++ b/src/test/modules/injection_points/t/001_stats.pl
@@ -94,6 +94,92 @@ $entrycount =
$node->safe_psql('postgres', "SELECT injection_points_stats_count();");
is($entrycount, '0', 'number of entries after drop via SQL function');
+# Crash to reset stats
+$node->stop('immediate');
+$node->start;
+
+# Test custom stats serialization/de-serialization callbacks
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('stats-notice1', 'notice',
+ 'this description is for stats-notice1 injection point');");
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('stats-notice2', 'notice',
+ 'this description is for stats-notice2 injection point');");
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('stats-notice3', 'notice');");
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('stats-notice4', 'notice',
+ 'this description is for stats-notice4 injection point');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice1');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice1');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice2');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice2');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice3');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice3');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice4');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice4');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice4');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice4');");
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice1');");
+is($numcalls, '2', 'number of stats calls for stats-notice1');
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice2');");
+is($numcalls, '2', 'number of stats calls for stats-notice2');
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice3');");
+is($numcalls, '2', 'number of stats calls for stats-notice3');
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice4');");
+is($numcalls, '4', 'number of stats calls for stats-notice4');
+my $description = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_description('stats-notice1');");
+is($description, 'this description is for stats-notice1 injection point',
+ 'description of stats-notice1 perserved before clean restart');
+$description = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_description('stats-notice2');");
+is($description, 'this description is for stats-notice2 injection point',
+ 'description of stats-notice2 perserved before clean restart');
+$description = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_description('stats-notice3');");
+is($description, '',
+ 'description of stats-notice3 perserved before clean restart');
+$description = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_description('stats-notice4');");
+is($description, 'this description is for stats-notice4 injection point',
+ 'description of stats-notice4 perserved before clean restart');
+# clean restart
+$node->restart;
+
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice1');");
+is($numcalls, '2', 'number of stats calls for stats-notice1');
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice2');");
+is($numcalls, '2', 'number of stats calls for stats-notice2');
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice3');");
+is($numcalls, '2', 'number of stats calls for stats-notice3');
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice4');");
+is($numcalls, '4', 'number of stats calls for stats-notice4');
+$description = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_description('stats-notice1');");
+is($description, 'this description is for stats-notice1 injection point',
+ 'description of stats-notice1 perserved after clean restart');
+$description = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_description('stats-notice2');");
+is($description, 'this description is for stats-notice2 injection point',
+ 'description of stats-notice2 perserved after clean restart');
+$description = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_description('stats-notice3');");
+is($description, '',
+ 'description of stats-notice3 perserved after clean restart');
+$description = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_description('stats-notice4');");
+is($description, 'this description is for stats-notice4 injection point',
+ 'description of stats-notice4 perserved after clean restart');
+
# Stop the server, disable the module, then restart. The server
# should be able to come up.
$node->stop;
--
2.43.0
[application/octet-stream] 0001-Add-callback-support-for-custom-statistics-extra-dat.patch (20.8K, ../CAA5RZ0s9SDOu+Z6veoJCHWk+kDeTktAtC-KY9fQ9Z6BJdDUirQ@mail.gmail.com/3-0001-Add-callback-support-for-custom-statistics-extra-dat.patch)
download | inline diff:
From 5cd825f048c4104231d2ce94b87d3e7d5a9a1506 Mon Sep 17 00:00:00 2001
From: Ubuntu <[email protected]>
Date: Wed, 22 Oct 2025 18:45:43 +0000
Subject: [PATCH 1/2] Add callback support for custom statistics extra data
serialization
Allow custom statistics kinds to serialize additional per-entry data
beyond the standard statistics entries. Custom kinds can register
to_serialized_extra and from_serialized_extra callbacks to write
and read extra data to separate files (pgstat.<kind>.stat).
The callbacks have access to the file pointer and the extension
that registers this custom statistic can write/read the data as it
understands it. The core pgstat infrastructure manages the files as
it does with pgstat.stat.
The callbacks are optional and only valid for custom,
variable-numbered statistics kinds. Using separate files keeps the
main statistics file format stable while enabling more complex
custom statistics.
This will allow extensions like pg_stat_statements to use custom
kinds and track additional data per entry such as query text, which
can be stored in a dsa_pointer. Using these callbacks, the
dsa_pointer can be serialized and deserialized across server
restarts.
---
src/backend/utils/activity/pgstat.c | 391 +++++++++++++++++++++-------
src/include/utils/pgstat_internal.h | 18 ++
2 files changed, 313 insertions(+), 96 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 7ef06150df7..d4a724d23bd 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -146,6 +146,21 @@
#define PGSTAT_FILE_ENTRY_HASH 'S' /* stats entry identified by
* PgStat_HashKey */
+/* ---------
+ * Limits for on-disk stats files.
+ *
+ * We maintain separate files for:
+ * - One built-in stats file (all standard PostgreSQL statistics)
+ * - One file per custom stats kind that uses extra serialization callbacks
+ *
+ * PGSTAT_BUILTIN_FILE is the array index for the built-in stats file.
+ * Custom stats files occupy indices 1 through PGSTAT_MAX_CUSTOM_FILES.
+ * ---------
+ */
+#define PGSTAT_MAX_CUSTOM_FILES PGSTAT_KIND_CUSTOM_MAX - PGSTAT_KIND_CUSTOM_MIN + 1
+#define PGSTAT_MAX_FILES PGSTAT_MAX_CUSTOM_FILES + 1
+#define PGSTAT_BUILTIN_FILE PGSTAT_KIND_MIN - 1
+
/* hash table for statistics snapshots entry */
typedef struct PgStat_SnapshotEntry
{
@@ -174,6 +189,16 @@ typedef struct PgStat_SnapshotEntry
#define SH_DECLARE
#include "lib/simplehash.h"
+/*
+ * Macros for custom stats file paths.
+ * Each custom stats kind with extra serialization data gets its own file
+ * in the format pgstat.<kind>.{tmp,stat}
+ */
+#define CUSTOM_TMPFILE_PATH(kind) \
+ psprintf(PGSTAT_STAT_PERMANENT_DIRECTORY "/pgstat.%d.tmp", (kind))
+
+#define CUSTOM_STATFILE_PATH(kind) \
+ psprintf(PGSTAT_STAT_PERMANENT_DIRECTORY "/pgstat.%d.stat", (kind))
/* ----------
* Local function forward declarations
@@ -1525,6 +1550,31 @@ pgstat_register_kind(PgStat_Kind kind, const PgStat_KindInfo *kind_info)
errdetail("Existing cumulative statistics with ID %u has the same name.", existing_kind)));
}
+ /*
+ * Ensure that both serialization and de-serialization callbacks are
+ * either registered together or not at all, and only for custom,
+ * variable-numbered stats.
+ */
+ if ((kind_info->to_serialized_extra && !kind_info->from_serialized_extra) ||
+ (!kind_info->to_serialized_extra && kind_info->from_serialized_extra))
+ {
+ ereport(ERROR,
+ (errmsg("invalid custom statistics callbacks for \"%s\" (ID %u)",
+ kind_info->name, kind),
+ errdetail("Both to_serialized_extra and from_serialized_extra "
+ "must either be set or unset.")));
+ }
+
+ if (kind_info->to_serialized_extra != NULL &&
+ (!pgstat_is_kind_custom(kind) || kind_info->fixed_amount))
+ {
+ ereport(ERROR,
+ (errmsg("invalid custom statistics callbacks for \"%s\" (ID %u)",
+ kind_info->name, kind),
+ errdetail("to_serialized_extra and from_serialized_extra callbacks are only allowed for "
+ "custom, variable-numbered statistics kinds.")));
+ }
+
/* Register it */
pgstat_kind_custom_infos[idx] = kind_info;
ereport(LOG,
@@ -1551,9 +1601,65 @@ pgstat_assert_is_up(void)
* ------------------------------------------------------------
*/
-/* helpers for pgstat_write_statsfile() */
+/* helpers for pgstat_read|write_statsfile() */
static void
-write_chunk(FILE *fpout, void *ptr, size_t len)
+pgstat_close_files(FILE **fp, int nfiles)
+{
+ /*
+ * Free all the allocated files.
+ */
+ for (int i = 0; i < nfiles; i++)
+ {
+ if (fp[i])
+ {
+ FreeFile(fp[i]);
+ fp[i] = NULL;
+ }
+ }
+}
+
+static bool
+pgstat_get_filenames_for_kind(const PgStat_Kind kind,
+ char **tmpfile, char **statfile)
+{
+ /*
+ * If this is a custom kind that serializes extra data, or the built-in
+ * pgstats file, return the appropriate name.
+ */
+ if (kind >= PGSTAT_KIND_CUSTOM_MIN)
+ {
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+ if (!kind_info || !kind_info->from_serialized_extra)
+ return false;
+
+ *tmpfile = CUSTOM_TMPFILE_PATH(kind);
+ *statfile = CUSTOM_STATFILE_PATH(kind);
+ return true;
+ }
+ else
+ {
+ *tmpfile = pstrdup(PGSTAT_STAT_PERMANENT_TMPFILE);
+ *statfile = pstrdup(PGSTAT_STAT_PERMANENT_FILENAME);
+ return true;
+ }
+}
+
+static void
+pgstat_report_statfile_error(PgStat_Kind kind, const char *statfile)
+{
+ ereport(LOG,
+ (errmsg("corrupted statistics file \"%s\"", statfile)));
+
+ if (kind == PGSTAT_BUILTIN_FILE)
+ pgstat_reset_after_failure();
+ else
+ pgstat_reset_of_kind(kind);
+}
+
+/* helpers for pgstat_write_statsfile() */
+void
+pgstat_write_chunk(FILE *fpout, void *ptr, size_t len)
{
int rc;
@@ -1563,7 +1669,16 @@ write_chunk(FILE *fpout, void *ptr, size_t len)
(void) rc;
}
-#define write_chunk_s(fpout, ptr) write_chunk(fpout, ptr, sizeof(*ptr))
+/* helpers for pgstat_read_statsfile() */
+static void
+pgstat_close_statfile(FILE *fp, const char *statfile)
+{
+ if (fp)
+ FreeFile(fp);
+
+ elog(DEBUG2, "removing permanent stats file \"%s\"", statfile);
+ unlink(statfile);
+}
/*
* This function is called in the last process that is accessing the shared
@@ -1572,10 +1687,8 @@ write_chunk(FILE *fpout, void *ptr, size_t len)
static void
pgstat_write_statsfile(void)
{
- FILE *fpout;
+ FILE *fpout[PGSTAT_MAX_FILES] = {NULL};
int32 format_id;
- const char *tmpfile = PGSTAT_STAT_PERMANENT_TMPFILE;
- const char *statfile = PGSTAT_STAT_PERMANENT_FILENAME;
dshash_seq_status hstat;
PgStatShared_HashEntry *ps;
@@ -1587,26 +1700,44 @@ pgstat_write_statsfile(void)
/* we're shutting down, so it's ok to just override this */
pgstat_fetch_consistency = PGSTAT_FETCH_CONSISTENCY_NONE;
- elog(DEBUG2, "writing stats file \"%s\"", statfile);
-
/*
- * Open the statistics temp file to write out the current values.
+ * Open temporary statistics files: one for built-in stats (idx=0), and
+ * one for each custom kind with extra serialization callbacks.
*/
- fpout = AllocateFile(tmpfile, PG_BINARY_W);
- if (fpout == NULL)
+ for (int idx = 0; idx <= PGSTAT_MAX_CUSTOM_FILES; idx++)
{
- ereport(LOG,
- (errcode_for_file_access(),
- errmsg("could not open temporary statistics file \"%s\": %m",
- tmpfile)));
- return;
+ char *tmpfile;
+ char *statfile;
+
+ if (!pgstat_get_filenames_for_kind(PGSTAT_KIND_CUSTOM_MIN + idx - 1,
+ &tmpfile, &statfile))
+ continue;
+
+ elog(DEBUG2, "writing stats file \"%s\"", statfile);
+
+ fpout[idx] = AllocateFile(tmpfile, PG_BINARY_W);
+
+ if (fpout[idx] == NULL)
+ {
+ pgstat_close_files(fpout, PGSTAT_MAX_FILES);
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not open temporary statistics file \"%s\": %m",
+ tmpfile)));
+ pfree(tmpfile);
+ pfree(statfile);
+ return;
+ }
+
+ pfree(tmpfile);
+ pfree(statfile);
}
/*
* Write the file header --- currently just a format ID.
*/
format_id = PGSTAT_FILE_FORMAT_ID;
- write_chunk_s(fpout, &format_id);
+ pgstat_write_chunk_s(fpout[PGSTAT_BUILTIN_FILE], &format_id);
/* Write various stats structs for fixed number of objects */
for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
@@ -1630,9 +1761,9 @@ pgstat_write_statsfile(void)
else
ptr = pgStatLocal.snapshot.custom_data[kind - PGSTAT_KIND_CUSTOM_MIN];
- fputc(PGSTAT_FILE_ENTRY_FIXED, fpout);
- write_chunk_s(fpout, &kind);
- write_chunk(fpout, ptr, info->shared_data_len);
+ fputc(PGSTAT_FILE_ENTRY_FIXED, fpout[PGSTAT_BUILTIN_FILE]);
+ pgstat_write_chunk_s(fpout[PGSTAT_BUILTIN_FILE], &kind);
+ pgstat_write_chunk(fpout[PGSTAT_BUILTIN_FILE], ptr, info->shared_data_len);
}
/*
@@ -1685,8 +1816,8 @@ pgstat_write_statsfile(void)
if (!kind_info->to_serialized_name)
{
/* normal stats entry, identified by PgStat_HashKey */
- fputc(PGSTAT_FILE_ENTRY_HASH, fpout);
- write_chunk_s(fpout, &ps->key);
+ fputc(PGSTAT_FILE_ENTRY_HASH, fpout[PGSTAT_BUILTIN_FILE]);
+ pgstat_write_chunk_s(fpout[PGSTAT_BUILTIN_FILE], &ps->key);
}
else
{
@@ -1695,58 +1826,80 @@ pgstat_write_statsfile(void)
kind_info->to_serialized_name(&ps->key, shstats, &name);
- fputc(PGSTAT_FILE_ENTRY_NAME, fpout);
- write_chunk_s(fpout, &ps->key.kind);
- write_chunk_s(fpout, &name);
+ fputc(PGSTAT_FILE_ENTRY_NAME, fpout[PGSTAT_BUILTIN_FILE]);
+ pgstat_write_chunk_s(fpout[PGSTAT_BUILTIN_FILE], &ps->key.kind);
+ pgstat_write_chunk_s(fpout[PGSTAT_BUILTIN_FILE], &name);
}
/* Write except the header part of the entry */
- write_chunk(fpout,
- pgstat_get_entry_data(ps->key.kind, shstats),
- pgstat_get_entry_len(ps->key.kind));
+ pgstat_write_chunk(fpout[PGSTAT_BUILTIN_FILE],
+ pgstat_get_entry_data(ps->key.kind, shstats),
+ pgstat_get_entry_len(ps->key.kind));
+
+ /* A plug-in is saving extra data */
+ if (kind_info->to_serialized_extra)
+ {
+ FILE *fp = fpout[(ps->key.kind - PGSTAT_KIND_CUSTOM_MIN) + 1];
+
+ Assert(fp);
+
+ kind_info->to_serialized_extra(&ps->key, shstats, fp);
+ }
}
dshash_seq_term(&hstat);
/*
* No more output to be done. Close the temp file and replace the old
- * pgstat.stat with it. The ferror() check replaces testing for error
- * after each individual fputc or fwrite (in write_chunk()) above.
+ * pgstat.stat or custom stats file with it. The ferror() check replaces
+ * testing for errors after each individual fputc or fwrite (in
+ * pgstat_write_chunk()) above.
*/
- fputc(PGSTAT_FILE_ENTRY_END, fpout);
+ fputc(PGSTAT_FILE_ENTRY_END, fpout[PGSTAT_BUILTIN_FILE]);
- if (ferror(fpout))
+ for (int idx = 0; idx <= PGSTAT_MAX_CUSTOM_FILES; idx++)
{
- ereport(LOG,
- (errcode_for_file_access(),
- errmsg("could not write temporary statistics file \"%s\": %m",
- tmpfile)));
- FreeFile(fpout);
- unlink(tmpfile);
- }
- else if (FreeFile(fpout) < 0)
- {
- ereport(LOG,
- (errcode_for_file_access(),
- errmsg("could not close temporary statistics file \"%s\": %m",
- tmpfile)));
- unlink(tmpfile);
- }
- else if (durable_rename(tmpfile, statfile, LOG) < 0)
- {
- /* durable_rename already emitted log message */
- unlink(tmpfile);
+ char *tmpfile;
+ char *statfile;
+
+ if (!pgstat_get_filenames_for_kind(PGSTAT_KIND_CUSTOM_MIN + idx - 1,
+ &tmpfile, &statfile))
+ continue;
+
+ if (ferror(fpout[idx]))
+ {
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not write temporary statistics file \"%s\": %m",
+ tmpfile)));
+ FreeFile(fpout[idx]);
+ unlink(tmpfile);
+ }
+ else if (FreeFile(fpout[idx]) < 0)
+ {
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not close temporary statistics file \"%s\": %m",
+ tmpfile)));
+ unlink(tmpfile);
+ }
+ else if (durable_rename(tmpfile, statfile, LOG) < 0)
+ {
+ /* durable_rename already emitted log message */
+ unlink(tmpfile);
+ }
+
+ pfree(tmpfile);
+ pfree(statfile);
}
}
/* helpers for pgstat_read_statsfile() */
-static bool
-read_chunk(FILE *fpin, void *ptr, size_t len)
+bool
+pgstat_read_chunk(FILE *fpin, void *ptr, size_t len)
{
return fread(ptr, 1, len, fpin) == len;
}
-#define read_chunk_s(fpin, ptr) read_chunk(fpin, ptr, sizeof(*ptr))
-
/*
* Reads in existing statistics file into memory.
*
@@ -1756,7 +1909,7 @@ read_chunk(FILE *fpin, void *ptr, size_t len)
static void
pgstat_read_statsfile(void)
{
- FILE *fpin;
+ FILE *fpin[PGSTAT_MAX_FILES] = {NULL};
int32 format_id;
bool found;
const char *statfile = PGSTAT_STAT_PERMANENT_FILENAME;
@@ -1765,32 +1918,48 @@ pgstat_read_statsfile(void)
/* shouldn't be called from postmaster */
Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
- elog(DEBUG2, "reading stats file \"%s\"", statfile);
-
- /*
- * Try to open the stats file. If it doesn't exist, the backends simply
- * returns zero for anything and statistics simply starts from scratch
- * with empty counters.
- *
- * ENOENT is a possibility if stats collection was previously disabled or
- * has not yet written the stats file for the first time. Any other
- * failure condition is suspicious.
- */
- if ((fpin = AllocateFile(statfile, PG_BINARY_R)) == NULL)
+ for (int idx = 0; idx <= PGSTAT_MAX_CUSTOM_FILES; idx++)
{
- if (errno != ENOENT)
- ereport(LOG,
- (errcode_for_file_access(),
- errmsg("could not open statistics file \"%s\": %m",
- statfile)));
- pgstat_reset_after_failure();
- return;
+ char *tmpfile;
+ char *statfile;
+
+ if (!pgstat_get_filenames_for_kind(PGSTAT_KIND_CUSTOM_MIN + idx - 1,
+ &tmpfile, &statfile))
+ continue;
+
+ elog(DEBUG2, "reading stats file \"%s\"", statfile);
+
+ /*
+ * Try to open the stats file. If it doesn't exist, the backends
+ * simply returns zero for anything and statistics simply starts from
+ * scratch with empty counters.
+ *
+ * ENOENT is a possibility if stats collection was previously disabled
+ * or has not yet written the stats file for the first time. Any
+ * other failure condition is suspicious.
+ */
+ if ((fpin[idx] = AllocateFile(statfile, PG_BINARY_R)) == NULL)
+ {
+ pgstat_close_files(fpin, PGSTAT_MAX_FILES);
+ if (errno != ENOENT)
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not open statistics file \"%s\": %m",
+ statfile)));
+ pgstat_reset_after_failure();
+ pfree(tmpfile);
+ pfree(statfile);
+ return;
+ }
+
+ pfree(tmpfile);
+ pfree(statfile);
}
/*
* Verify it's of the expected format.
*/
- if (!read_chunk_s(fpin, &format_id))
+ if (!pgstat_read_chunk_s(fpin[PGSTAT_BUILTIN_FILE], &format_id))
{
elog(WARNING, "could not read format ID");
goto error;
@@ -1809,7 +1978,7 @@ pgstat_read_statsfile(void)
*/
for (;;)
{
- int t = fgetc(fpin);
+ int t = fgetc(fpin[PGSTAT_BUILTIN_FILE]);
switch (t)
{
@@ -1820,7 +1989,7 @@ pgstat_read_statsfile(void)
char *ptr;
/* entry for fixed-numbered stats */
- if (!read_chunk_s(fpin, &kind))
+ if (!pgstat_read_chunk_s(fpin[PGSTAT_BUILTIN_FILE], &kind))
{
elog(WARNING, "could not read stats kind for entry of type %c", t);
goto error;
@@ -1860,7 +2029,7 @@ pgstat_read_statsfile(void)
info->shared_data_off;
}
- if (!read_chunk(fpin, ptr, info->shared_data_len))
+ if (!pgstat_read_chunk(fpin[PGSTAT_BUILTIN_FILE], ptr, info->shared_data_len))
{
elog(WARNING, "could not read data of stats kind %u for entry of type %c with size %u",
kind, t, info->shared_data_len);
@@ -1875,13 +2044,14 @@ pgstat_read_statsfile(void)
PgStat_HashKey key;
PgStatShared_HashEntry *p;
PgStatShared_Common *header;
+ const PgStat_KindInfo *kind_info = NULL;
CHECK_FOR_INTERRUPTS();
if (t == PGSTAT_FILE_ENTRY_HASH)
{
/* normal stats entry, identified by PgStat_HashKey */
- if (!read_chunk_s(fpin, &key))
+ if (!pgstat_read_chunk_s(fpin[PGSTAT_BUILTIN_FILE], &key))
{
elog(WARNING, "could not read key for entry of type %c", t);
goto error;
@@ -1895,7 +2065,8 @@ pgstat_read_statsfile(void)
goto error;
}
- if (!pgstat_get_kind_info(key.kind))
+ kind_info = pgstat_get_kind_info(key.kind);
+ if (!kind_info)
{
elog(WARNING, "could not find information of kind for entry %u/%u/%" PRIu64 " of type %c",
key.kind, key.dboid,
@@ -1906,16 +2077,15 @@ pgstat_read_statsfile(void)
else
{
/* stats entry identified by name on disk (e.g. slots) */
- const PgStat_KindInfo *kind_info = NULL;
PgStat_Kind kind;
NameData name;
- if (!read_chunk_s(fpin, &kind))
+ if (!pgstat_read_chunk_s(fpin[PGSTAT_BUILTIN_FILE], &kind))
{
elog(WARNING, "could not read stats kind for entry of type %c", t);
goto error;
}
- if (!read_chunk_s(fpin, &name))
+ if (!pgstat_read_chunk_s(fpin[PGSTAT_BUILTIN_FILE], &name))
{
elog(WARNING, "could not read name of stats kind %u for entry of type %c",
kind, t);
@@ -1946,7 +2116,7 @@ pgstat_read_statsfile(void)
if (!kind_info->from_serialized_name(&name, &key))
{
/* skip over data for entry we don't care about */
- if (fseek(fpin, pgstat_get_entry_len(kind), SEEK_CUR) != 0)
+ if (fseek(fpin[PGSTAT_BUILTIN_FILE], pgstat_get_entry_len(kind), SEEK_CUR) != 0)
{
elog(WARNING, "could not seek \"%s\" of stats kind %u for entry of type %c",
NameStr(name), kind, t);
@@ -1990,9 +2160,9 @@ pgstat_read_statsfile(void)
key.objid, t);
}
- if (!read_chunk(fpin,
- pgstat_get_entry_data(key.kind, header),
- pgstat_get_entry_len(key.kind)))
+ if (!pgstat_read_chunk(fpin[PGSTAT_BUILTIN_FILE],
+ pgstat_get_entry_data(key.kind, header),
+ pgstat_get_entry_len(key.kind)))
{
elog(WARNING, "could not read data for entry %u/%u/%" PRIu64 " of type %c",
key.kind, key.dboid,
@@ -2000,6 +2170,28 @@ pgstat_read_statsfile(void)
goto error;
}
+ /*
+ * A plug-in is reading extra data. If the plug-in fails
+ * to read the file, close the file, report the error, and
+ * move on.
+ */
+ if (kind_info->from_serialized_extra)
+ {
+ FILE *fp = fpin[(key.kind - PGSTAT_KIND_CUSTOM_MIN) + 1];
+
+ Assert(fp);
+
+ if (!kind_info->from_serialized_extra(&key, header, fp))
+ {
+ char *statfile = CUSTOM_STATFILE_PATH(key.kind);
+
+ pgstat_report_statfile_error(key.kind, statfile);
+
+ fpin[(key.kind - PGSTAT_KIND_CUSTOM_MIN) + 1] = NULL;
+ pfree(statfile);
+ }
+ }
+
break;
}
case PGSTAT_FILE_ENTRY_END:
@@ -2008,7 +2200,7 @@ pgstat_read_statsfile(void)
* check that PGSTAT_FILE_ENTRY_END actually signals end of
* file
*/
- if (fgetc(fpin) != EOF)
+ if (fgetc(fpin[PGSTAT_BUILTIN_FILE]) != EOF)
{
elog(WARNING, "could not read end-of-file");
goto error;
@@ -2023,18 +2215,25 @@ pgstat_read_statsfile(void)
}
done:
- FreeFile(fpin);
+ for (int idx = 0; idx <= PGSTAT_MAX_CUSTOM_FILES; idx++)
+ {
+ char *tmpfile;
+ char *statfile;
- elog(DEBUG2, "removing permanent stats file \"%s\"", statfile);
- unlink(statfile);
+ if (!pgstat_get_filenames_for_kind(PGSTAT_KIND_CUSTOM_MIN + idx - 1,
+ &tmpfile, &statfile))
+ continue;
+
+ pgstat_close_statfile(fpin[idx], statfile);
+
+ pfree(tmpfile);
+ pfree(statfile);
+ }
return;
error:
- ereport(LOG,
- (errmsg("corrupted statistics file \"%s\"", statfile)));
-
- pgstat_reset_after_failure();
+ pgstat_report_statfile_error(PGSTAT_BUILTIN_FILE, statfile);
goto done;
}
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 4d2b8aa6081..c317bf883a0 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -303,6 +303,15 @@ typedef struct PgStat_KindInfo
const PgStatShared_Common *header, NameData *name);
bool (*from_serialized_name) (const NameData *name, PgStat_HashKey *key);
+ /*
+ * For custom, variable-numbered stats, serialize/de-serialize extra data
+ * per entry. Optional.
+ */
+ bool (*from_serialized_extra) (PgStat_HashKey *key,
+ const PgStatShared_Common *header, FILE *fd);
+ void (*to_serialized_extra) (const PgStat_HashKey *key,
+ const PgStatShared_Common *header, FILE *fd);
+
/*
* For fixed-numbered statistics: Initialize shared memory state.
*
@@ -984,4 +993,13 @@ pgstat_get_custom_snapshot_data(PgStat_Kind kind)
return pgStatLocal.snapshot.custom_data[idx];
}
+/*
+ * Helpers for reading/writing stats files.
+ */
+extern void pgstat_write_chunk(FILE *fpout, void *ptr, size_t len);
+extern bool pgstat_read_chunk(FILE *fpin, void *ptr, size_t len);
+
+#define pgstat_write_chunk_s(fpout, ptr) pgstat_write_chunk(fpout, ptr, sizeof(*ptr))
+#define pgstat_read_chunk_s(fpin, ptr) pgstat_read_chunk(fpin, ptr, sizeof(*ptr))
+
#endif /* PGSTAT_INTERNAL_H */
--
2.43.0
view thread (47+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [Proposal] Adding callback support for custom statistics kinds
In-Reply-To: <CAA5RZ0s9SDOu+Z6veoJCHWk+kDeTktAtC-KY9fQ9Z6BJdDUirQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox