Subject: [Proposal] Adding callback support for custom statistics kinds

From: Sami Imseih <[email-masked]>
Date: Wed, 22 Oct 2025 15:24:11 -0500

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)

---

From: Michael Paquier <[email-masked]>
Date: Thu, 23 Oct 2025 12:33:02 +0900

> need to track variable-length data stored in a dsa_pointer.

Thanks for sending a proposal in this direction.

> backend to manage it safely.

Agreed that it would be better to split the query text in a file of
its own and now bloat the "main" pgstats file with this data,  A real
risk is that many PGSS entries with a bunch of queries would cause the
file to be just full of the PGSS contents.

> callbacks provides more control.

Per-kind callbacks to control all that makes sense here.

> with spaces or special characters.

Hmm.  Is that really what we want here?  This pretty says that one
single custom kind would never be able use multiple files, ever.

> deserializing would leave orphaned files behind, and I cannot think of a

Hmm.  Okay.

> re-implementing these functions.

Exposing the write and read chunk APIs and renaming them sounds good
here, designed as they are now with a FILE* defined by the caller.
It's good to share these for consistency across custom and built-in
stats kinds.

> kinds. Custom fixed kinds may not benefit, but could be considered in the

Pushing custom data for fixed-sized stats may be interesting, though
like you I am not sure what a good use-case would look like.  So
discarding this case for now sounds fine to me.

> Attached 0001 is the proposed change, still in POC form.

Hmm.  I would like to propose something a bit more flexible,
refactoring and reusing some of the existing callbacks, among the
following lines:
- Rather than introducing a second callback able to do more
serialization work, let's expand a bit the responsibility of
to_serialized_name and from_serialized_name to be able to work in a
more extended way, renaming them to "to/from_serialized_entry", which
are now limited to return a NameData with pgstat.c enforcing the data
written to the pgstats to be of NAMEDATALEN.  The idea would be to let
the callbacks push some custom data where they want.
- The to_serialized_name path of pgstat_write_statsfile() would then
be changed as follows:
-- push a PGSTAT_FILE_ENTRY_NAME
-- Write the key write_chunk_s.
-- Call the callback to push some custom per-entry data.
-- Finish with the main chunk of data, of size pgstat_get_entry_len().
- The fd or FILE* of the "main" pgstats file should be added as
argument of both routines (not mandatory, but we are likely going to
need that if we want to add more "custom" data in the main pgstats
file before writing or reading a chunk).  For example, for a PGSS text
file, we would likely write two fields to the main data file: an
offset and a length to be able to retrieve a query string, from a
secondary file. 
- FDs where the data is written while we are in the to/from serialize
can be handled within the code paths specific to the stats kind code.
The first time a serialized callback of a stats kind is called, the
extra file(s) is(are) opened.  This may come at the cost of one new
callback: at the end of the read and writes of the stats data, we
would need an extra look that's able to perform cleanup actions, which
would be here to make sure that the fds opened for the extra files are
closed when we are done.  The close of each file is equivalent to the
pgstat_close_file() done in the patch, except that we'd loop over a
callback that would do the cleanup job once we are done reading or
writing a file.  One step that can be customized in this new "end"
callback is if a stats kind may decide to unlink() a previous file, as
we do for the main pgstats file, or keep one or more files around.
That would be up to the extension developer.  We should be able to
reuse or rework reset_all_cb() with a status given to it, depending on
if we are dealing with a failure or a success path.  Currently,
reset_all_cb() is only used in a failure path, the idea would be to
extend it for the success case.

> The second patch

Having coverage for these kinds of APIs is always good, IMO.  We need
coverage for extension code.
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Thu, 23 Oct 2025 16:35:58 -0500

Thanks for the feedback!

> single custom kind would never be able use multiple files, ever.

Perhaps if someone wants to have separate files for each different
types of data,
we should be able to support multiple files. I think we can add an
option for the
number of files and they can then be named "pgstat.<kind>.1.stat",
pgstat.<kind>.2.stat",
etc. I rather avoid having the extension provide a set of files names.
So as arguments to the callback, besides the main file pointer ( as
you mention below),
we also provide the list of custom file pointers.

what do you think?

> more extended way, renaming them to "to/from_serialized_entry", which

Sure, we can go that route.

> offset and a length to be able to retrieve a query string, from a

Yeah, that could be a good idea for pg_s_s, if we don't want to store the key
alongside the query text. Make more sense.

> extend it for the success case.

I will provide a patch with the recommendations.

--
Sami

---

From: Michael Paquier <[email-masked]>
Date: Fri, 24 Oct 2025 08:33:17 +0900

> we also provide the list of custom file pointers.

My worry here is the lack of flexibility regarding stats that could be
split depending on the objects whose data needs to be flushed.  For
example, stats split across multiple databases (like our good-old
pre-v14 pgstats, but on a per-kind basis).  So I don't think that we
can really assume that the list of file names should be fixed when we
begin the read/write process of the main pgstats file.
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Thu, 23 Oct 2025 19:57:38 -0500

> begin the read/write process of the main pgstats file.

I was trying to avoid an extra field in PgStat_KindInfo if possible, but
it's worthwhile to provide more flexibility to an extension. I will go
with this.

---

From: Michael Paquier <[email-masked]>
Date: Fri, 24 Oct 2025 14:54:22 +0900

> it's worthwhile to provide more flexibility to an extension. I will go

Yes, I don't think that we will be able to avoid some refactoring of
the existing callbacks.  The introduction of a new one may not be
completely necessary, though, especially if we reuse the reset
callback to be called when the stats read and write finish to close
any fds we may have opened when processing.

Maintaining the state of the files opened within each stat kind code
across multiple calls of the new "serialized" callback feels a bit
more natural and more flexible, at least it's my take on the matter.
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Mon, 10 Nov 2025 13:56:23 -0600

> Sure, we can go that route.

I started reworking the patch, but then I realized that I don't like this
approach of using the same callback to support serializing NameData and
serializing extra data. In the existing "to_serialized_name" callback
, NameData is serialized instead of the hash key, meaning that the
"from_serialized_name" must be called before we create an entry. The
callback translates the NameData to an objid as is the case with replication
slots, and the key is then used to create the entry.

However, in the case of serializing extra data, we want to have already
created the entry by the time we call the callback. For example populating
non-key fields of an entry with a dsa_pointer after reading some serialized
data into dsa.

If we do want to support a single callback, we would need extra metadata in
the Kind registration to let the extension tell us what the callback is used
for and to either trigger the callback before or after entry creation. I am
not very thrilled about doing something like this, as I see 2 very different
use-cases here.

What do you think?

---

From: Michael Paquier <[email-masked]>
Date: Tue, 11 Nov 2025 08:21:26 +0900

> slots, and the key is then used to create the entry.

Thanks for looking at that.

> not very thrilled about doing something like this, as I see 2 very different

Ah, I see your point.  By keeping two callbacks, one to translate a
key to/from a different field (NameData currently, but it could be
something else with a different size), we would for example be able to
keep very simple the checks for duplicated entries when reading the
file.  Agreed that it would be good to keep the key lookups as stable
as we can.

So, what you are suggested is a second callback once we have called
read_chunk() and write_chunk() for a PGSTAT_FILE_ENTRY_HASH or a
PGSTAT_FILE_ENTRY_NAME and let a stats kind write in the main file
and/or one or more extra files the data they want?  I'd be fine with
that, yes, and that should work with the PGSS case in mind.
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Wed, 19 Nov 2025 14:10:42 -0600

Sorry for the delay here.

v1 is the first attempt to address the feedback from the POC.

1/ A user is now able to register as many extra files as they
wish, and the files will be named pgstat.<kind_id>.<file no>.stat,
where file_no starts at 0 up to the number of files specified
by the user with .num_serialized_extra_files.

2/ The callbacks now provide both the core stat file as a FILE
pointer and an array of FILE pointers for the extra files.
IN the write callback, the extra file pointer is accessed
like extra_files[0], extra_files[1], etc., and the same for
the read callback.

3/ The patch centralizes the creation and cleanup of the files
with 2 new routines pgstat_allocate_files and pgstat_cleanup_files,
which both operate on a new local struct which tracks the file
names and descriptors in the read and write stats routines.

```
typedef struct PgStat_SerializeFiles
{
    char      **tmpfiles;
    char      **statfiles;
    FILE      **fd;
    int            num_files;
}            PgStat_SerializeFiles;
```

plug-ins are not made aware of this struct because they don't need
to. The callbacks are provided the FILE pointers they need to care
about for their kind only.

4/ In terms of testing, patch 0002, I did not want to invent a new module
for custom kinds, so I piggybacked off og injection_points as I did in the
POC, but I added on the existing recovery tests, because essentially that
is what we care. Does the data persist after a clean shutdown? do the
.tmp files get removed properly? etc. So I added tests in
recovery/t/029_stats_restart.pl for this.

---

From: Sami Imseih <[email-masked]>
Date: Wed, 19 Nov 2025 20:10:43 -0600

It just occurred to me that the documentation [0] should be
updated to describe the callbacks. I will do that in the next
revision.

[0] https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-ADDIN-CUSTOM-CUMULATIVE-STATISTICS

---

From: Michael Paquier <[email-masked]>
Date: Mon, 24 Nov 2025 09:10:29 +0900

> [0] https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-ADDIN-CUSTOM-CUMULATIVE-STATISTICS

Hmm.  Based on what I can read from the patch, you are still enforcing
file name patterns in the backend, as of:
+          extra->statfiles[i] = psprintf("%s/pgstat.%d.%d.stat",
+                        PGSTAT_STAT_PERMANENT_DIRECTORY, kind, i);

My take (also mentioned upthread) is that this design should go the
other way around, where modules have the possibility to define their
own file names, and some of them could be generated on-the-fly when
writing the files, for example for a per-file database split, or the
object ID itself.

The important part for variable-numbered stats is that the keys of the
entries have to be in the main pgstats file.  Then, the extra data is
loaded back based on the data in the entry key, based on a file name
that only a custom stats kind knows about (fd and file name).  It
means that the custom stats kind needs to track the files it has to
clean up by itself in this scheme.  We could pass back to the startup
process some fds that it cleans up, but it feels simpler here to let
the custom code do what they want, instead, rather than having an
array that tracks the file names and/or their fds.
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Sun, 23 Nov 2025 22:43:17 -0600

> writing the files, for example for a per-file database split, or the

The way I thought about it is that extension developer can just provide the
number of files they need and the they are then given a list of
file pointers that they need. They can then manage what each file is
used for. They also don't need to worry about naming the files, all they
need to do is track what each file in the list does.

> array that tracks the file names and/or their fds.

yeah, I was leaning towards putting more responsibility on pgstat to
manage these extra files, but you are suggesting that we just let the
extension manage the create/cleanup of these files as well.

After re-reading your earlier suggestions, this sounds like a third
callback that is used for file cleanup, and this callback could be
the existing reset_all_cb. Also, instead of reset_all_cb being called
during pgstat_reset_after_failure, it can be called during the success
case, i.e, a new pgstat_reset_after_success. reset_all_cb also
carries a status argument so the extension knows what to do
in the case of success or failure.

This also means we need to also update all existing callbacks to
do work in the failed status.

Is that correct?

--
Sami

---

From: Sami Imseih <[email-masked]>
Date: Mon, 24 Nov 2025 18:18:26 -0600

> do work in the failed status.

After second thought, I am not too thrilled with extending reset_all_cb
to take responsibility for file cleanup, etc. I think it should just remain
used to reset stats only.

I think the best way forward will be to introduce a callback to be used by
custom kinds only. This callback will be responsible for cleaning up files
and related resources at the end of the write stats, read stats, and discard
stats paths. The callback will provide back to the extension a status
(READ, WRITE, DISCARD) and the extension will know how to clean up the
resources it created depending on the situation.

So, unlike my original proposal, this puts more responsibility on the
extension to track and clean up its files, but this seems like the best
approach to take here.

Also, I am now leaning towards creating a separate test module rather than
trying to do too much unrelated testing in injection points. It is definitely
convenient to use injection points, but I think we can do better testing with
a separate module. This module can also serve as an example for extension
developers.

what do you think?

---

From: Michael Paquier <[email-masked]>
Date: Tue, 25 Nov 2025 11:03:22 +0900

> resources it created depending on the situation.

I guess that READ and WRITE are the cases that happen on success of
these respective operations.  DISCARD is the failure case when one of
these fail.

> extension to track and clean up its files, but this seems like the best

That may be something we should do anyway.  It means that the modules
are responsible for the tracking the file(s) they open, still they
could also decide operations different than the backend for the main
pgstats file, optionally, depending on the state of the reads and
writes (aka success or failure of these).

> a separate module. This module can also serve as an example for extension

You are right that it may be cleaner this way.  Do you think that it
could make sense to move some of the existing "template" code of
injection_points there?

One part of the proposed patch that felt independent to me was the
renaming and publishing of the two write/read routines for the stats
files, so I have extracted that in your first patch to reduce the
blast, and applied that as it can also be useful on its own.
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Tue, 2 Dec 2025 14:58:32 -0600

> could make sense to move some of the existing "template" code of

By "template" code, do you mean Something like?

include/utils/custom_statkinds.h
backend/utils/misc/custom_statkinds.c

Where the template code here is PgStat_kind definition, callbacks, etc. for
injection_points or the new test module that is using a custom kind.

A few benefits I see for this is we can point extension developers to
this as an example in [0] and we are also maintaining the kind ids in
a single place. These may not be strong points, but may be worth while.

v2 attached is something that may be closer to what we've been discussing

v2-0001 are much simplified changes to pgstat.c that simply invoke the callbacks
and all the work is on the extension to implement what it needs to do.
This includes
a callback at the end of WRITE, READ, DISCARD with a flag passed to the caller
so they can perform the necessary clean-up actions.

v2-0002 implements a new test module that tests mainly that the recovery,
clean and crash, are working as expected.

I created a new tap test for this which performs a test similar to what is
done in recovery/029_stats_restart.pl. I could merge the new test there, but
I am reluctant to add a dependency on a new module to recovery. What
do you think?

---

From: Michael Paquier <[email-masked]>
Date: Wed, 3 Dec 2025 09:01:14 +0900

> injection_points or the new test module that is using a custom kind.

I am not completely sure that we need a separate C file for a portion
of the code related to custom stats kinds.  At least, I am not sure to
see which part of pgstat.c, pgstat_internal.h and pgstat_shmem.c could
be extracted so as a custom_statkinds.c could have value when taken
independently.  A test module makes the most sense for such templates
IMO, as they can be compiled and checked directly.

> so they can perform the necessary clean-up actions.

+	void		(*to_serialized_extra_stats) (const PgStat_HashKey *key,
+											  const PgStatShared_Common *header, FILE *statfile);
+	void		(*from_serialized_extra_stats) (const PgStat_HashKey *key,
+												const PgStatShared_Common *header, FILE *statfile);
+	void		(*end_extra_stats) (PgStat_StatsFileOp status);
[...]
+typedef struct PgStatShared_CustomEntry
+{
+	PgStatShared_Common header;
+	PgStat_StatCustomEntry stats;
+	char		name[NAMEDATALEN];
+	dsa_pointer description;
+}			PgStatShared_CustomEntry;

I'm cool with this design, including your point about using a DSA
pointer in a stats entry, manipulating this data through the
serialization callback.  Your module does not use the FILE* which
points to the main stats file for the to/from extra serialized
callbacks, it seems important to document in pgstat_internal.h that
this points to the "main" pgstats file manipulated by the startup
process when loading or by the checkpointer when flushing.

Perhaps the callback in the module for end_extra_stats should use a
switch based on PgStat_StatsFileOp.  Minor point.

+/* File handle for statistics serialization */
+static FILE *fd = NULL;

Using a fd tracked directly by the module sounds good to me, yes.
That gives to the modules the flexibility to decide what should be the
extra files to know about, some file name patterns being possible to
decide based on the stats entry keys that need to be written, with
files opened when actually required.

> clean and crash, are working as expected.

That looks like a good direction to me.  The only differences I can
see with the stats module in injection_points for variable-sized stats
is that this new module does not check pgstat_drop_entry() and
pgstat_fetch_entry() when working on a custom stats kind.  If we had
SQL interfaces calling these two, we could just remove
injection_stats.c entirely, moving everything to this new test module.
I should have invented a new module from the start, perhaps, but well,
that was good enough to check the basic APIs when working on the
custom APIs.  Removing this duplication would be my own business with
your module in the tree, no need for you to worry about that.  That
would also remove the tweak you have used regarding the duplicated
kind ID.

Perhaps we should do the same for the fixed-sized kind at the end, and
instead of using one .so for both of them, we could just create a
separate .so with multiple entries in MODULES?  What do you think?
What you have here is better than what's in the tree in terms of
module separation for HEAD.

> I am reluctant to add a dependency on a new module to recovery. What

Adding an extra item to recovery's EXTRA_INSTALL would be OK for me,
but it seems cleaner to me to keep the tests related to custom stats
in their own area like your patch 0002 is doing with its new test
module test_custom_statkind.  And 029_stats_restart.pl is already
covering a lot of ground.

+     if (pgstat_is_kind_custom(key.kind) && kind_info->from_serialized_extra_stats)
+         kind_info->from_serialized_extra_stats(&key, header, fpin);
[...]
+     if (pgstat_is_kind_custom(ps->key.kind) && kind_info->to_serialized_extra_stats)
+         kind_info->to_serialized_extra_stats(&ps->key, shstats, fpout);

These restrictions based on custom kinds do not seem mandatory.
Why not allowing built-in kinds the same set of operations?

+	/* Read and verify the hash key */
+	if (!pgstat_read_chunk(fd, (void *) key, sizeof(PgStat_HashKey)))
+		return;
[...]
+	/* Write the hash key to identify this entry */
+	pgstat_write_chunk(fd, (void *) key, sizeof(PgStat_HashKey));

I am puzzled by this part of 0002.  Why are you overwriting the key
once after loading it from the main pgstats file?  Writing the key to
cross-check that the data matches with what is in the main file is OK,
and this should be ensured because of the ordering of the data.  I
would have done it in a slightly different way, I guess, with the data
stored on disk in the main pgstats file including an offset to know
where to search in the secondary file.  That's what we would do for
PGSS as well, I guess, with the secondary file including data
structured as a set of:
- Entry key, cross-checked with the data read from the main file,
based on the offset stored in the main file.
- Length of extra data.
- The extra data contents.

As a whole, I find this patch pretty cool, particularly the point
about extending stats entries with DSAs, something that would be
essential for PGSS and move it to use pgstats because we don't want
the query strings in the main pgstats file and bloat it.  Nice.
--
Michael

---

From: Chao Li <[email-masked]>
Date: Wed, 3 Dec 2025 13:41:44 +0800

> On Dec 3, 2025, at 04:58, Sami Imseih <[email-masked]> wrote:
> 
> <v2-0002-Tests-for-custom-stat-kinds.patch><v2-0001-pgstat-support-custom-serialization-files-and-cal.patch>

Thanks for the patch, I do think the feature will be useful. After reading the patch, I got a concern on the design:

This patch provides callbacks that requests (also allows) custom extensions to write stat files on their own behalf, which I think it’s unsafe. The problems coming out to my head includes:

* An extension can write to any where on the storage, that what about it writes to /tmp and the files are deleted by other process or by a user manually incidentally?
* pgstat has a pattern of writing files like: writing to tmp file first, then durable_rename(), how to ensure extensions to do the same pattern? Without this pattern, how to ensure reliability of stat files?
* In the current path, pgstat performs its own write, then call callbacks. What about if a callback fails? Will that leave pgstat in a stale state?
* As extensions own file creation and deletion, in some case, staled file might be left on storage, who will be responsible for cleaning up them?

Given the goal of the feature is to allow extensions to serialize custom data, the callback should just return serialized/deserialized data, maybe together with some metadata, then pgstat should be responsible for writing the data. In other words, IMO, pgstat should always own stat files.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Michael Paquier <[email-masked]>
Date: Wed, 3 Dec 2025 14:54:52 +0900

On Wed, Dec 03, 2025 at 01:41:44PM +0800, Chao Li wrote:
> This patch provides callbacks that requests (also allows) custom
> extensions to write stat files on their own behalf, which I think
> it’s unsafe. The problems coming out to my head includes:
> 
> * An extension can write to any where on the storage, that what
> * about it writes to /tmp and the files are deleted by other process
> * or by a user manually incidentally?

I mean, just don't do that.  It's up to the extension developer to
decide what is safe or not, within the scope of the data folder.

> * pgstat has a pattern of writing files like: writing to tmp file
> * first, then durable_rename(), how to ensure extensions to do the
> * same pattern? Without this pattern, how to ensure reliability of
> * stat files?

Extension code would be responsible for ensuring that.

> * In the current path, pgstat performs its own write, then call
> * callbacks. What about if a callback fails? Will that leave pgstat
> * in a stale state?

For the write state, end_extra_stats() would take care of that.  It
depends on what kind of errors you would need to deal with, but as
proposed the patch would offer the same level of protection for the
writes of the stats, where we'd check for an error on the fd saved by
an extension for an extra file.

I think that you have a fair point about the stats read path though,
shouldn't we make the callback from_serialized_extra_stats() return a
state to be able to trigger a full-scale cleanup, at least?

> * As extensions own file creation and deletion, in some case, staled
> * file might be left on storage, who will be responsible for
> * cleaning up them? 

The extension should be able to handle that, I guess.

> Given the goal of the feature is to allow extensions to serialize
> custom data, the callback should just return serialized/deserialized
> data, maybe together with some metadata, then pgstat should be
> responsible for writing the data. In other words, IMO, pgstat should
> always own stat files. 

That's where my view of the matter differs, actually, pushing down the
responsibility into the extension code itself.  A key argument,
mentioned upthread, is that the file paths could depend on the stats
entry *keys*, which may not be known in advance when beginning the
flush of the stats.  Think about per-database file stats, or just
some per-object file stats, for example, which is an option that would
matter so as we do not bloat the main pgstats file.
--
Michael

---

From: Chao Li <[email-masked]>
Date: Wed, 3 Dec 2025 14:16:54 +0800

> On Dec 3, 2025, at 13:54, Michael Paquier <[email-masked]> wrote:
> 

Yes, they of course can do, but that’s out of pgstat’s control. How can we ensure that?

> matter so as we do not bloat the main pgstats file.

If we push down the responsibility into the extension code, then all extensions that want to enjoy the callbacks have to handle the same complexities of dealing with stat files, which sounds big duplicate efforts.

Best regards,

---

From: Sami Imseih <[email-masked]>
Date: Thu, 4 Dec 2025 17:00:10 -0600

> If we push down the responsibility into the extension code, then all extensions
> that want to enjoy the callbacks have to handle the same complexities of dealing
> with stat files, which sounds big duplicate efforts.

Thanks for the input! Yes, this is a trade-off between putting
responsibility on the
extension vs core. The initial thought I had was exactly like yours, but it will
be easier to get something pushed if we make the core changes as minimal as
possible. If there are enough complaints in the future, this can be revisited.
Particularly if there is a common patterns for file cleanup, this
could be turned
into a core utility.

> would also remove the tweak you have used regarding the duplicated

I plan on addressing the other comments.

However, as discussed off-list, I do think moving the custom kind tests from
injection points to the new test module is a prerequisite. I rather
not  have us
push a new test module that is doing duplicate work as the injection
stats tests.
I worked on this refactoring today and plan to have a patch ready for review
by tomorrow.

---

From: Michael Paquier <[email-masked]>
Date: Fri, 5 Dec 2025 08:14:58 +0900

> Particularly if there is a common patterns for file cleanup, this

Another way to shape it would be to have an in-core routine that
provides a default logic for the actions to take depending on the
write, read or discard state, with the state and a FILE* as arguments.
The main pgstats file would call that, modules may decide to use it.

> not have us push a new test module that is doing duplicate work as the injection
> stats tests.

Cool, thanks!
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Fri, 5 Dec 2025 19:27:50 -0600

> > I worked on this refactoring today and plan to have a patch ready for review

Attached is the new test module that replaces the custom statistics
tests currently in the injection points tests. Under test_custom_stats, there
are two separate modules: one for variable-amount stats and one for
fixed-amount stats. With this, we can completely remove the
stats-related tests and supporting code under
src/test/modules/injection_points/.

A few notes on the tests:

1. Variable stats: pgstat_drop_entry() and pgstat_fetch_entry() are
exercised here, addressing an earlier point raised in the thread.

2. Fixed-amount stats: I added specific tests for reset behavior; both
during crash recovery and during manual resets.

3. In test_custom_fixed_stats.c, you will see this comment:
```
/* see explanation above PgStatShared_Archiver for the reset protocol */
LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
```
This is intentional, as the reset protocol is documented at the
referenced location [0]. I wanted to call that out for the patch review.

Once this gets pushed, it will simplify the remaining work needed
for the remaining serialization callbacks work.

[0] https://github.com/postgres/postgres/blob/master/src/include/utils/pgstat_internal.h#L362-L382

---

From: Sami Imseih <[email-masked]>
Date: Sun, 7 Dec 2025 11:42:38 -0600

I took a look at the earlier comments.

> once after loading it from the main pgstats file?

Yes, this is not necessary. I removed it.

> I would have done it in a slightly different way, I guess, with the data
> where to search in the secondary file.

that's a much better approach and the pattern we would want to use
going forward. Since this does not require we read the entires
back in the same order as written, so it's much more flexible.
I not did this in the test module.

> switch based on PgStat_StatsFileOp.  Minor point.

Agree. Done.

> state to be able to trigger a full-scale cleanup, at least?

In this case, even if the callback does not return a state, the cleanup
will eventually occur at the end of the read, see

```
done:
    /* First, cleanup the main stats file, PGSTAT_STAT_PERMANENT_FILENAME */
    FreeFile(fpin);

    elog(DEBUG2, "removing permanent stats file \"%s\"", statfile);
    unlink(statfile);

    /* Let each stats kind run its cleanup callback, if it provides one */
    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 && kind_info->end_extra_stats)
            kind_info->end_extra_stats(STATS_READ);
    }
```

However, this could also mean some entries could be read back correctly, and
others not, so maybe it's not such a good idea. So, I did what is suggested
and allow the callback to return a bool which will raise an error and trigger
the cleanup code.

> Why not allowing built-in kinds the same set of operations?

No good reason not to. In fact, maybe a follow-up will be to move the
replslot to this infrastructure and remove reliance on PGSTAT_FILE_ENTRY_NAME.

attached is the v4 patch set which includes:

0001 - which is just moving the tests out of injection points into a new
test module. This is similar to v3 [0].

0002 - Is the code changes to implement the callbacks and the necessary
tests in the new test module.

[0] https://www.postgresql.org/message-id/CAA5RZ0sG2RUKg%3DOLY%2B6-e4q%3DX9rsLfK3pKn03d%3DRZQppEDR%3DBg%40mail.gmail.com

---

From: Michael Paquier <[email-masked]>
Date: Mon, 8 Dec 2025 15:35:23 +0900

> src/test/modules/injection_points/.

Yes, thanks.  Structurally, this is better and more flexible than what
we had originally, and I have noticed that you have copied the
original files while adding more comments and renaming a bit things:
the structure of the functions was exactly the same.  Anyway, I have
worked on that for a good portion of the day, splitting the module
drop and the new module into two commits, and applied the result after
tweaking quite a few things in terms of names and comments (no
pgstat_*, a bit more "Var" and "Fixed", etc.), applying a much more
consistent set of names across the board for the functions and the
structures.  This cleanup part is moved out of the way now, so that
you ease the introduction of the next pieces you are proposing. 

The tests for the reset of fixed-sized stats was a nice addition,
indeed.  If you have more areas that you think could be improved,
ideas are of course welcome.
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Mon, 8 Dec 2025 13:09:14 -0600

> you ease the introduction of the next pieces you are proposing.

Thanks for getting these committed!

I rebased the custom callbacks patch in v5.

One very minor thing from the earlier commits that I corrected here is
the test for entry 2 after a clean restart.

-is($result, "entry1|2", "variable-sized stats persist after clean restart");
+is($result, "entry1|2|Test entry 1", "variable-sized stats persist
after clean restart");
+
+$result = $node->safe_psql('postgres', q(select * from
test_custom_stats_var_report('entry2')));
+is($result, "entry2|3|Test entry 2", "variable-sized stats persist
after clean restart");
+

---

From: Chao Li <[email-masked]>
Date: Tue, 9 Dec 2025 11:38:53 +0800

> On Dec 9, 2025, at 03:09, Sami Imseih <[email-masked]> wrote:
> 
> <v5-0001-Allow-cumulative-statistics-to-serialize-auxiliar.patch>

```
+					if (kind_info->from_serialized_extra_stats)
+					{
+						if (!kind_info->from_serialized_extra_stats(&key, header, fpin))
+						{
+							elog(WARNING, "could not read extra stats for entry %u/%u/%" PRIu64,
+								 key.kind, key.dboid, key.objid);
+							goto error;
+						}
+					}
```

When deserialize failed, it goes to error. In the error clause, it calls pgstat_reset_after_failure(), so do we want to give extensions a chance to do some reset operations? If yes, then we can add a reset_after_failure() callback.

Best regards,

---

From: Sami Imseih <[email-masked]>
Date: Mon, 8 Dec 2025 21:57:15 -0600

> When deserialize failed, it goes to error. In the error clause, it calls pgstat_reset_after_failure(), so do we want to give extensions a chance to do some reset operations? If yes, then we can add…

The way v5 is dealing with a deserialize failure is that when
it goes to error, the pgstat_reset_after_failure() will reset the
stats for all kinds, since pgstat_drop_all_entries() is called
during that call. So there is nothing for an extension to have
to do on its own. The extension will then clean-up resources
at the end when  all the kinds are iterated over and
kind_info->end_extra_stats(STATS_READ) is called for each
kind.

Let me know if I'm still missing something?

---

From: Michael Paquier <[email-masked]>
Date: Tue, 9 Dec 2025 13:45:17 +0900

> Let me know if I'm still missing something?

It seems to me that you are missing nothing here, and that Chao has
missed the fact that the end of pgstat_read_statsfile() does a "goto
done", meaning that we would take a round of
end_extra_stats(STATS_READ) to do all the cleanup after resetting all
the stats.  That's what I would expect.

+static inline bool pgstat_check_extra_callbacks(PgStat_Kind kind);
[...]
@@ -645,6 +656,13 @@ pgstat_initialize(void)
+	/* Check a kind's extra-data callback setup */
+	for (PgStat_Kind kind = PGSTAT_KIND_BUILTIN_MIN; kind <= PGSTAT_KIND_BUILTIN_MAX; kind++)
+		if (!pgstat_check_extra_callbacks(kind))
+			ereport(ERROR,
+					errmsg("incomplete extra serialization callbacks for stats kind %d",
+						   kind));

Why does this part need to run each time a backend initializes its
access to pgstats?  Shouldn't this happen only once when a stats kind
is registered?  pgstat_register_kind() should be the only code path
that does such sanity checks.

By the way, checking that to_serialized_extra_stats and
kind_info->from_serialized_extra_stats need to be both defined is
fine as these are coupled together, but I am not following the reason
why end_extra_stats would need to be included in the set?  For
example, a stats kind could decide to add some data to the main
pgstats file without creating extra files, hence they may not need to
define end_extra_stats.
--
Michael

---

From: Chao Li <[email-masked]>
Date: Tue, 9 Dec 2025 13:23:56 +0800

> On Dec 9, 2025, at 12:45, Michael Paquier <[email-masked]> wrote:
> 

No, I didn’t miss that part. But in the “done” clause:

```
done:

end_extra_stats(STATS_READ) has no failure indication.

Best regards,

---

From: Chao Li <[email-masked]>
Date: Tue, 9 Dec 2025 13:35:47 +0800

> On Dec 9, 2025, at 13:23, Chao Li <[email-masked]> wrote:
> 
> 
> 

Sorry, I incidentally clicked “send” too quickly.

My point is that, there are many places jumping to “error”, then from “error” goto “done”, if an error didn’t happen from the deserialize callback, how end_extra_stats() can know if failure happened and takes action accordingly?

Best regards,

---

From: Sami Imseih <[email-masked]>
Date: Tue, 9 Dec 2025 15:54:58 -0600

> My point is that, there are many places jumping to “error”, then from “error” goto “done”,
> if an error didn’t happen from the deserialize callback, how end_extra_stats()
> can know if failure happened and takes action accordingly?

IIUC, if *any* error occurs outside of a deserialize callback, first the "error"
code will be called, followed by "done" which will then trigger the
end_extra_stats
callback that will perform the cleanup.

Attached is v6 with a few minor indentation fixes and a correction to
freeing the file in the cleanup callback.

---

From: Chao Li <[email-masked]>
Date: Wed, 10 Dec 2025 07:05:42 +0800

> On Dec 10, 2025, at 05:54, Sami Imseih <[email-masked]> wrote:
> 

That is true. But problem is, without an error indication, end_extra_stats(STATS_READ) can only blindly perform cleanup works. As you are providing general purposed callbacks, who knows what scenarios extensions would do, so it’s better to provide more information to callbacks. IMO, letting end_extra_stats() know current situation (normal or failure, even error code) is very meaningful. For example, my extension may want to log “I am forced to quite due to outside error” or “I am done successfully” in end_extra_stats(). Anyway, that’s my own opinion. If you and Michael still consider that’s not a problem, I won’t argue more.

Best reagards,

---

From: Sami Imseih <[email-masked]>
Date: Tue, 9 Dec 2025 17:15:45 -0600

> That is true. But problem is, without an error indication,
> end_extra_stats(STATS_READ) can only blindly perform cleanup works. As you
> are providing general purposed callbacks, who knows what scenarios
> extensions would do, so it’s better to provide more information to
> callbacks. IMO, letting end_extra_stats() know current situation (normal or
> failure, even error code) is very meaningful. For example, my extension may
> want to log “I am forced to quite due to outside error” or “I am done
> successfully” in end_extra_stats(). Anyway, that’s my own opinion. If you
> and Michael still consider that’s not a problem, I won’t argue more.

Thanks for explaining. If there is a good use-case to add more detail to
the “end” callback, it’s not very obvious yet. Maybe in the future, there
will be a convincing reason to do so.

When we hit the clean-up code on any “error”, it should be accompanied by
an error log. That is
done in all cases inside pgstat.c, and I expect an extension to log the
error as well.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 10 Dec 2025 17:04:16 +0900

> will be a convincing reason to do so.

The step taken by test_custom_var_stats_file_cleanup() for the
STATS_READ case shines for its simplicity.  The STATS_DISCARD case is
also simple: we know that we want to ditch the stats.

Now, it is kind of true that the STATS_WRITE case feels a bit
disturbing written this way: we let a module take an action, but we
don't actually know the state of the main pgstats file when inside the
callback.  I mean, you can know how things are going on, but it means
that a module can just rely on a check if
PGSTAT_STAT_PERMANENT_FILENAME is on disk, but an unlink() could have
failed as well.  So, yes, I am wondering whether we should do what
Chao is suggesting, passing an extra state to the callback to let the
module know if we have actually succeeded or failed the operations
that have been taken on the main stats file before the callback
end_extra_stats is called in the three cases.  It does not matter for
the STATS_READ case, but it may matter for the STATS_DISCARD or
STATS_WRITE case.

> done in all cases inside pgstat.c, and I expect an extension to log the

FWIW, I still have the same question as the one posted here about the
business in pgstat_initialize(), still present in v6:
https://www.postgresql.org/message-id/[email-masked]

This remains unanswered.
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Wed, 10 Dec 2025 12:36:36 -0600

> the STATS_READ case, but it may matter for the STATS_DISCARD or

I am having a hard time being convinced that this extra status is needed.
I am not expecting an extension to operate on the main stats file inside
the end_extra_stats callback, and even if some operation failed on the
main stats file, the cleanup callback will need to take the steps to
perform the cleanup on its own resources.

Is there a concrete example?

> https://www.postgresql.org/message-id/[email-masked]

Responding to the questions from the thread above.

> access to pgstats?

Good point. This is unnecessary. This validation should really be
done inside StatsShmemInit by postmaster.

> pgstats file without creating extra files, hence they may not need to

.. and after giving this more thought, I actually don't think we should
do any validation for any of the callbacks. If an extension is writing
to any file ( core or custom ), naturally they will want to read it back.
Now I am not sure what these validations are protecting us against.
Also, maybe the extension wants to just read data from the main stats
file, I could see that use-case, perhaps.

So, I am proposing removing the validation altogether. What do
you think?

---

From: Michael Paquier <[email-masked]>
Date: Fri, 12 Dec 2025 11:58:54 +0900

> So, I am proposing removing the validation altogether. What do

The to and from callbacks are coupled with each other, so there may be
a point in making sure that if one is defined so is the other.  Now, I
have never done any enforcement for the existing from/to serialization
callbacks either because it would be quickly clear for one what needs
to be done when implementing a custom kind.  So I'd agree with just
removing these checks and keep the code simpler.

FWIW, I have begun putting my hands on your patch, editing it at some
degree.  I am not sure that I will be able to finish that today, but
I'm working towards getting something done.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Fri, 12 Dec 2025 18:39:49 +0900

> I'm working towards getting something done.

Well, I have been able to do enough progress to have something to
share, and I'm getting pretty happy about how things are shaping.  As
you will notice, I have edited quite a few things..  In details:
- Less fwrite() and fread(), more read_chunk() and write_chunk().  We
are exposing these APIs, let's use them.
- Comments, much more comments and documentation.
- The callbacks are renamed, to be more generic: "finish" for the
end-of-operation actions and to/from_serialized_data.
- The format of the extra data in the main pgstats file and the
secondary file was a bit strange.  Mainly, why adding the length to
the main file and not the secondary file?  I have extended that a
little bit:
-- Addition of a magic number in the main file, to provide an extra
layer of safety in the read callback, letting the callback know that
it needs to read some data.
-- The offset of the secondary file follows immeditely.
-- The secondary file includes at the offset a copy of the hash key,
the description length, and the description.
- Reorganization of the read/write flow for the callbacks in the
modules, tracking the offset at write more precisely.  The handling of
the empty descriptions becomes simpler than what you have proposed
previously.

This way, we can make sure that the main stats file is OK with the
magic number, and we have a sanity check in the secondary file based
on the hash key whose copy is in the main stats file.

Regarding the error state that could be sent to the "end" callback, I
think that you are right.  We are not gaining much with that as by
design we are already pretty loose on the write side, hoping for the
best, relying on the read side to enforce all sanity checks.  So a
status in the "from" callback sounds like a good enough balance.

At the end of the day, I'm feeling pretty much OK with the core
changes and the layer we have here.  The module changes need an extra
round of lookup (did as well some tests with corrupted and empty
secondary files to test the stability at reload), and I'm pretty tired
so I may have missed something there.  The patch needs to be split in
two parts: one for the backend changes, and one for the module itself.
The backend changes are feeling pretty good, the module changes feel
better.
--
Michael
From cafc8251e6f56e113ad4ad19ffb72fa9ad312def Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Fri, 12 Dec 2025 18:38:05 +0900
Subject: [PATCH v7] Allow cumulative statistics to serialize auxiliary data to
 disk.

Cumulative Statistics kinds can now write additional per-entry data to
the statistics file that doesn't fit in shared memory. This is useful
for statistics with variable-length auxiliary data.

Three new optional callbacks are added to PgStat_KindInfo:

* to_serialized_data: writes auxiliary data for an entry
* from_serialized_data: reads auxiliary data for an entry
* finish: performs actions after read/write/discard operations.

The finish callback is invoked after processing all entries of a kind,
allowing extensions to close file handles and clean up resources.

Tests are also added to test_custom_stats.pl

Discussion: https://postgr.es/m/[email-masked]
---
 src/include/executor/executor.h               |   2 +-
 src/include/libpq/libpq-be-fe-helpers.h       |   2 +-
 src/include/utils/pgstat_internal.h           |  47 ++
 src/backend/libpq/pqformat.c                  |   3 +-
 src/backend/rewrite/rewriteHandler.c          |   2 +-
 src/backend/storage/file/fileset.c            |   3 +-
 src/backend/utils/activity/pgstat.c           |  50 ++-
 src/backend/utils/adt/numutils.c              |   2 +-
 src/backend/utils/cache/relcache.c            |   2 +-
 src/backend/utils/mb/conv.c                   |   4 +-
 src/backend/utils/mb/mbutils.c                |   6 +-
 src/backend/utils/misc/guc.c                  |   2 +-
 src/backend/utils/mmgr/alignedalloc.c         |   4 +-
 src/backend/utils/mmgr/portalmem.c            |   3 +-
 src/backend/utils/sort/tuplesortvariants.c    |   1 -
 .../test_custom_stats/t/001_custom_stats.pl   |  39 +-
 .../test_custom_var_stats--1.0.sql            |   7 +-
 .../test_custom_stats/test_custom_var_stats.c | 400 +++++++++++++++++-
 src/tools/pgindent/typedefs.list              |   1 +
 19 files changed, 542 insertions(+), 38 deletions(-)

diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index fa2b657fb2ff..7cd6a49309f0 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -404,7 +404,7 @@ ExecEvalExpr(ExprState *state,
  * Like ExecEvalExpr(), but for cases where no return value is expected,
  * because the side-effects of expression evaluation are what's desired. This
  * is e.g. used for projection and aggregate transition computation.
-
+ *
  * Evaluate expression identified by "state" in the execution context
  * given by "econtext".
  *
diff --git a/src/include/libpq/libpq-be-fe-helpers.h b/src/include/libpq/libpq-be-fe-helpers.h
index 1c4a342090c3..d2f6b3b13484 100644
--- a/src/include/libpq/libpq-be-fe-helpers.h
+++ b/src/include/libpq/libpq-be-fe-helpers.h
@@ -69,7 +69,7 @@ libpqsrv_connect(const char *conninfo, uint32 wait_event_info)
 /*
  * Like libpqsrv_connect(), except that this is a wrapper for
  * PQconnectdbParams().
-  */
+ */
 static inline PGconn *
 libpqsrv_connect_params(const char *const *keywords,
 						const char *const *values,
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index ca1ba6420ca1..031fd34ad788 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -63,6 +63,21 @@ typedef struct PgStat_HashKey
 								 * identifier. */
 } PgStat_HashKey;
 
+/*
+ * Tracks if the stats file is being read, written or discarded, used in
+ * combination with the finish callback.
+ *
+ * These states allow plugins that create extra statistics files
+ * to determine the current operation and perform any necessary
+ * file cleanup.
+ */
+typedef enum PgStat_StatsFileOp
+{
+	STATS_WRITE,
+	STATS_READ,
+	STATS_DISCARD,
+} PgStat_StatsFileOp;
+
 /*
  * PgStat_HashKey should not have any padding.  Checking that the structure
  * size matches with the sum of each field is a check simple enough to
@@ -303,6 +318,38 @@ typedef struct PgStat_KindInfo
 									   const PgStatShared_Common *header, NameData *name);
 	bool		(*from_serialized_name) (const NameData *name, PgStat_HashKey *key);
 
+	/*
+	 * For variable-numbered stats: read or write additional data related to a
+	 * given entry, in the stats file or optionally in a different file.
+	 * Optional.
+	 *
+	 * to_serialized_data: write extra data for an entry.
+	 *
+	 * from_serialized_data: read extra data for an entry.  Returns true on
+	 * success, false on read error.
+	 *
+	 * "statfile" is a pointer to the on-disk stats file, named
+	 * PGSTAT_STAT_PERMANENT_FILENAME.  "key" is the hash key of the entry
+	 * just written or read.  "header" is a pointer to the stats data.
+	 */
+	void		(*to_serialized_data) (const PgStat_HashKey *key,
+									   const PgStatShared_Common *header,
+									   FILE *statfile);
+	bool		(*from_serialized_data) (const PgStat_HashKey *key,
+										 const PgStatShared_Common *header,
+										 FILE *statfile);
+
+	/*
+	 * For fixed-numbered or variable-numbered statistics.
+	 *
+	 * Perform custom actions when done processing the on-disk stats file
+	 * after all the stats entries have been processed.  Optional.
+	 *
+	 * "status" tracks the operation done for the on-disk stats file (read,
+	 * write, discard).
+	 */
+	void		(*finish) (PgStat_StatsFileOp status);
+
 	/*
 	 * For fixed-numbered statistics: Initialize shared memory state.
 	 *
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index 1cc126772f7c..67bdd3d93d05 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -307,9 +307,8 @@ pq_endmessage(StringInfo buf)
  *
  * The data buffer is *not* freed, allowing to reuse the buffer with
  * pq_beginmessage_reuse.
- --------------------------------
+ * --------------------------------
  */
-
 void
 pq_endmessage_reuse(StringInfo buf)
 {
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 688dcd09ca6e..0852322cc588 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -2620,7 +2620,7 @@ view_col_is_auto_updatable(RangeTblRef *rtr, TargetEntry *tle)
  * view_query_is_auto_updatable - test whether the specified view definition
  * represents an auto-updatable view. Returns NULL (if the view can be updated)
  * or a message string giving the reason that it cannot be.
-
+ *
  * The returned string has not been translated; if it is shown as an error
  * message, the caller should apply _() to translate it.
  *
diff --git a/src/backend/storage/file/fileset.c b/src/backend/storage/file/fileset.c
index 4d5ee353fd7a..2061aa44e773 100644
--- a/src/backend/storage/file/fileset.c
+++ b/src/backend/storage/file/fileset.c
@@ -114,7 +114,8 @@ FileSetCreate(FileSet *fileset, const char *name)
 }
 
 /*
- * Open a file that was created with FileSetCreate() */
+ * Open a file that was created with FileSetCreate()
+ */
 File
 FileSetOpen(FileSet *fileset, const char *name, int mode)
 {
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 8713c7a04834..647fe9b111a4 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -523,6 +523,7 @@ pgstat_discard_stats(void)
 
 	/* NB: this needs to be done even in single user mode */
 
+	/* First, cleanup the main stats file, PGSTAT_STAT_PERMANENT_FILENAME */
 	ret = unlink(PGSTAT_STAT_PERMANENT_FILENAME);
 	if (ret != 0)
 	{
@@ -544,6 +545,15 @@ pgstat_discard_stats(void)
 								 PGSTAT_STAT_PERMANENT_FILENAME)));
 	}
 
+	/* Finish callbacks, if required */
+	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 && kind_info->finish)
+			kind_info->finish(STATS_DISCARD);
+	}
+
 	/*
 	 * Reset stats contents. This will set reset timestamps of fixed-numbered
 	 * stats to the current time (no variable stats exist).
@@ -1702,6 +1712,10 @@ pgstat_write_statsfile(void)
 		pgstat_write_chunk(fpout,
 						   pgstat_get_entry_data(ps->key.kind, shstats),
 						   pgstat_get_entry_len(ps->key.kind));
+
+		/* Write more data for the entry, if required */
+		if (kind_info->to_serialized_data)
+			kind_info->to_serialized_data(&ps->key, shstats, fpout);
 	}
 	dshash_seq_term(&hstat);
 
@@ -1734,6 +1748,15 @@ pgstat_write_statsfile(void)
 		/* durable_rename already emitted log message */
 		unlink(tmpfile);
 	}
+
+	/* Now, allow stats kinds to finalize the data writes */
+	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 && kind_info->finish)
+			kind_info->finish(STATS_WRITE);
+	}
 }
 
 /* helper for pgstat_read_statsfile() */
@@ -1871,6 +1894,7 @@ pgstat_read_statsfile(void)
 					PgStat_HashKey key;
 					PgStatShared_HashEntry *p;
 					PgStatShared_Common *header;
+					const PgStat_KindInfo *kind_info = NULL;
 
 					CHECK_FOR_INTERRUPTS();
 
@@ -1891,7 +1915,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,
@@ -1902,7 +1927,6 @@ 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;
 
@@ -1996,6 +2020,18 @@ pgstat_read_statsfile(void)
 						goto error;
 					}
 
+					/* read more data for the entry, if required */
+					if (kind_info->from_serialized_data)
+					{
+						if (!kind_info->from_serialized_data(&key, header, fpin))
+						{
+							elog(WARNING, "could not read extra stats for entry %u/%u/%" PRIu64 " of type %c",
+								 key.kind, key.dboid,
+								 key.objid, t);
+							goto error;
+						}
+					}
+
 					break;
 				}
 			case PGSTAT_FILE_ENTRY_END:
@@ -2019,11 +2055,21 @@ pgstat_read_statsfile(void)
 	}
 
 done:
+	/* First, cleanup the main stats file, PGSTAT_STAT_PERMANENT_FILENAME */
 	FreeFile(fpin);

+	/* Let each stats kind run its finish callback, if required */
+	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 && kind_info->finish)
+			kind_info->finish(STATS_READ);
+	}
+
 	return;
 
 error:
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index 3bf30774a0c9..254c5cf82e4b 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -113,7 +113,7 @@ static const int8 hexlookup[128] = {
  * pg_strtoint16() will throw ereport() upon bad input format or overflow;
  * while pg_strtoint16_safe() instead returns such complaints in *escontext,
  * if it's an ErrorSaveContext.
-*
+ *
  * NB: Accumulate input as an unsigned number, to deal with two's complement
  * representation of the most negative number, which can't be represented as a
  * signed positive number.
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index a4dc1cbe5aec..2d0cb7bcfd4a 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5643,7 +5643,7 @@ RelationGetIdentityKeyBitmap(Relation relation)
  * This should be called only for an index that is known to have an associated
  * exclusion constraint or primary key/unique constraint using WITHOUT
  * OVERLAPS.
-
+ *
  * It returns arrays (palloc'd in caller's context) of the exclusion operator
  * OIDs, their underlying functions' OIDs, and their strategy numbers in the
  * index's opclasses.  We cache all this information since it requires a fair
diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index 4a312ab429b6..d53e885b067e 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -484,7 +484,7 @@ pg_mb_radix_conv(const pg_mb_radix_tree *rt,
  * utf: input string in UTF8 encoding (need not be null-terminated)
  * len: length of input string (in bytes)
  * iso: pointer to the output area (must be large enough!)
-		  (output string will be null-terminated)
+ *		  (output string will be null-terminated)
  * map: conversion map for single characters
  * cmap: conversion map for combined characters
  *		  (optional, pass NULL if none)
@@ -694,7 +694,7 @@ UtfToLocal(const unsigned char *utf, int len,
  * iso: input string in local encoding (need not be null-terminated)
  * len: length of input string (in bytes)
  * utf: pointer to the output area (must be large enough!)
-		  (output string will be null-terminated)
+ *		  (output string will be null-terminated)
  * map: conversion map for single characters
  * cmap: conversion map for combined characters
  *		  (optional, pass NULL if none)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index dbce0e61812b..d1701d69b16e 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -497,7 +497,8 @@ pg_do_encoding_conversion_buf(Oid proc,
  * Convert string to encoding encoding_name. The source
  * encoding is the DB encoding.
  *
- * BYTEA convert_to(TEXT string, NAME encoding_name) */
+ * BYTEA convert_to(TEXT string, NAME encoding_name)
+ */
 Datum
 pg_convert_to(PG_FUNCTION_ARGS)
 {
@@ -522,7 +523,8 @@ pg_convert_to(PG_FUNCTION_ARGS)
  * Convert string from encoding encoding_name. The destination
  * encoding is the DB encoding.
  *
- * TEXT convert_from(BYTEA string, NAME encoding_name) */
+ * TEXT convert_from(BYTEA string, NAME encoding_name)
+ */
 Datum
 pg_convert_from(PG_FUNCTION_ARGS)
 {
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f7d63e04c046..935c235e1b39 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5449,7 +5449,7 @@ ShowGUCOption(const struct config_generic *record, bool use_units)
  *		variable sourceline, integer
  *		variable source, integer
  *		variable scontext, integer
-*		variable srole, OID
+ *		variable srole, OID
  */
 static void
 write_one_nondefault_variable(FILE *fp, struct config_generic *gconf)
diff --git a/src/backend/utils/mmgr/alignedalloc.c b/src/backend/utils/mmgr/alignedalloc.c
index b1be74269149..daee3fc80a1c 100644
--- a/src/backend/utils/mmgr/alignedalloc.c
+++ b/src/backend/utils/mmgr/alignedalloc.c
@@ -23,8 +23,8 @@
 
 /*
  * AlignedAllocFree
-*		Frees allocated memory; memory is removed from its owning context.
-*/
+ *		Frees allocated memory; memory is removed from its owning context.
+ */
 void
 AlignedAllocFree(void *pointer)
 {
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 943da087c9f5..1f2a423f38a6 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -853,7 +853,8 @@ AtAbort_Portals(void)
 /*
  * Post-abort cleanup for portals.
  *
- * Delete all portals not held over from prior transactions.  */
+ * Delete all portals not held over from prior transactions.
+ */
 void
 AtCleanup_Portals(void)
 {
diff --git a/src/backend/utils/sort/tuplesortvariants.c b/src/backend/utils/sort/tuplesortvariants.c
index 079a51c474d0..a1f5c19ee976 100644
--- a/src/backend/utils/sort/tuplesortvariants.c
+++ b/src/backend/utils/sort/tuplesortvariants.c
@@ -1132,7 +1132,6 @@ tuplesort_getgintuple(Tuplesortstate *state, Size *len, bool forward)
  * efficient, but only safe for callers that are prepared to have any
  * subsequent manipulation of the tuplesort's state invalidate slot contents.
  * For byval Datums, the value of the 'copy' parameter has no effect.
-
  */
 bool
 tuplesort_getdatum(Tuplesortstate *state, bool forward, bool copy,
diff --git a/src/test/modules/test_custom_stats/t/001_custom_stats.pl b/src/test/modules/test_custom_stats/t/001_custom_stats.pl
index e528595cfb0c..378ec22bbdf7 100644
--- a/src/test/modules/test_custom_stats/t/001_custom_stats.pl
+++ b/src/test/modules/test_custom_stats/t/001_custom_stats.pl
@@ -29,13 +29,13 @@ $node->safe_psql('postgres', q(CREATE EXTENSION test_custom_fixed_stats));
 
 # Create entries for variable-sized stats.
 $node->safe_psql('postgres',
-	q(select test_custom_stats_var_create('entry1')));
+	q(select test_custom_stats_var_create('entry1', 'Test entry 1')));
 $node->safe_psql('postgres',
-	q(select test_custom_stats_var_create('entry2')));
+	q(select test_custom_stats_var_create('entry2', 'Test entry 2')));
 $node->safe_psql('postgres',
-	q(select test_custom_stats_var_create('entry3')));
+	q(select test_custom_stats_var_create('entry3', 'Test entry 3')));
 $node->safe_psql('postgres',
-	q(select test_custom_stats_var_create('entry4')));
+	q(select test_custom_stats_var_create('entry4', 'Test entry 4')));
 
 # Update counters: entry1=2, entry2=3, entry3=2, entry4=3, fixed=3
 $node->safe_psql('postgres',
@@ -65,16 +65,28 @@ $node->safe_psql('postgres', q(select test_custom_stats_fixed_update()));
 # Test data reports.
 my $result = $node->safe_psql('postgres',
 	q(select * from test_custom_stats_var_report('entry1')));
-is($result, "entry1|2", "report for variable-sized data of entry1");
+is( $result,
+	"entry1|2|Test entry 1",
+	"report for variable-sized data of entry1");
+
 $result = $node->safe_psql('postgres',
 	q(select * from test_custom_stats_var_report('entry2')));
-is($result, "entry2|3", "report for variable-sized data of entry2");
+is( $result,
+	"entry2|3|Test entry 2",
+	"report for variable-sized data of entry2");
+
 $result = $node->safe_psql('postgres',
 	q(select * from test_custom_stats_var_report('entry3')));
-is($result, "entry3|2", "report for variable-sized data of entry3");
+is( $result,
+	"entry3|2|Test entry 3",
+	"report for variable-sized data of entry3");
+
 $result = $node->safe_psql('postgres',
 	q(select * from test_custom_stats_var_report('entry4')));
-is($result, "entry4|3", "report for variable-sized data of entry4");
+is( $result,
+	"entry4|3|Test entry 4",
+	"report for variable-sized data of entry4");
+
 $result = $node->safe_psql('postgres',
 	q(select * from test_custom_stats_fixed_report()));
 is($result, "3|", "report for fixed-sized stats");
@@ -97,7 +109,16 @@ $node->start();
 
 $result = $node->safe_psql('postgres',
 	q(select * from test_custom_stats_var_report('entry1')));
-is($result, "entry1|2", "variable-sized stats persist after clean restart");
+is( $result,
+	"entry1|2|Test entry 1",
+	"variable-sized stats persist after clean restart");
+
+$result = $node->safe_psql('postgres',
+	q(select * from test_custom_stats_var_report('entry2')));
+is( $result,
+	"entry2|3|Test entry 2",
+	"variable-sized stats persist after clean restart");
+
 $result = $node->safe_psql('postgres',
 	q(select * from test_custom_stats_fixed_report()));
 is($result, "3|", "fixed-sized stats persist after clean restart");
diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats--1.0.sql b/src/test/modules/test_custom_stats/test_custom_var_stats--1.0.sql
index d5f82b5d546e..5ed8cfc2dcf1 100644
--- a/src/test/modules/test_custom_stats/test_custom_var_stats--1.0.sql
+++ b/src/test/modules/test_custom_stats/test_custom_var_stats--1.0.sql
@@ -3,7 +3,7 @@
 -- complain if script is sourced in psql, rather than via CREATE EXTENSION
 \echo Use "CREATE EXTENSION test_custom_var_stats" to load this file. \quit
 
-CREATE FUNCTION test_custom_stats_var_create(IN name TEXT)
+CREATE FUNCTION test_custom_stats_var_create(IN name TEXT, in description TEXT)
 RETURNS void
 AS 'MODULE_PATHNAME', 'test_custom_stats_var_create'
 LANGUAGE C STRICT PARALLEL UNSAFE;
@@ -18,8 +18,9 @@ RETURNS void
 AS 'MODULE_PATHNAME', 'test_custom_stats_var_drop'
 LANGUAGE C STRICT PARALLEL UNSAFE;
 
-
-CREATE FUNCTION test_custom_stats_var_report(INOUT name TEXT, OUT calls BIGINT)
+CREATE FUNCTION test_custom_stats_var_report(INOUT name TEXT,
+                                             OUT calls BIGINT,
+                                             OUT description TEXT)
 RETURNS SETOF record
 AS 'MODULE_PATHNAME', 'test_custom_stats_var_report'
 LANGUAGE C STRICT PARALLEL UNSAFE;
diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats.c b/src/test/modules/test_custom_stats/test_custom_var_stats.c
index d4905ab4ee99..9cc9ff8cd9f9 100644
--- a/src/test/modules/test_custom_stats/test_custom_var_stats.c
+++ b/src/test/modules/test_custom_stats/test_custom_var_stats.c
@@ -14,6 +14,7 @@
 
 #include "common/hashfn.h"
 #include "funcapi.h"
+#include "storage/dsm_registry.h"
 #include "utils/builtins.h"
 #include "utils/pgstat_internal.h"
 
@@ -22,6 +23,8 @@ PG_MODULE_MAGIC_EXT(
 					.version = PG_VERSION
 );
 
+#define TEST_CUSTOM_VAR_MAGIC_NUMBER (0xBEEFBEEF)
+
 /*--------------------------------------------------------------------------
  * Macros and constants
  *--------------------------------------------------------------------------
@@ -32,6 +35,9 @@ PG_MODULE_MAGIC_EXT(
  */
 #define PGSTAT_KIND_TEST_CUSTOM_VAR_STATS 25
 
+/* File paths for extra statistics data serialization */
+#define TEST_CUSTOM_EXTRA_DATA_DESC "pg_stat/test_custom_var_stats_desc.stats"
+
 /*
  * Hash statistic name to generate entry index for pgstat lookup.
  */
@@ -53,8 +59,23 @@ typedef struct PgStatShared_CustomVarEntry
 {
 	PgStatShared_Common header; /* standard pgstat entry header */
 	PgStat_StatCustomVarEntry stats;	/* custom statistics data */
+	dsa_pointer description;	/* extra statistics data */
 } PgStatShared_CustomVarEntry;
 
+/*--------------------------------------------------------------------------
+ * Global Variables
+ *--------------------------------------------------------------------------
+ */
+
+/* File handle for extra statistics data serialization */
+static FILE *fd_description = NULL;
+
+/* Current write offset in fd_description file */
+static long fd_description_offset = 0;
+
+/* DSA area for storing variable-length description strings */
+static dsa_area *custom_stats_description_dsa = NULL;
+
 /*--------------------------------------------------------------------------
  * Function prototypes
  *--------------------------------------------------------------------------
@@ -64,6 +85,19 @@ typedef struct PgStatShared_CustomVarEntry
 static bool test_custom_stats_var_flush_pending_cb(PgStat_EntryRef *entry_ref,
 												   bool nowait);
 
+/* Serialization callback: serialize extra statistics data */
+static void test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key,
+													 const PgStatShared_Common *header,
+													 FILE *statfile);
+
+/* Deserialization callback: deserialize extra statistics data */
+static bool test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key,
+													   const PgStatShared_Common *header,
+													   FILE *statfile);
+
+/* Cleanup callback: end of statistics file operations */
+static void test_custom_stats_var_finish(PgStat_StatsFileOp status);
+
 /*--------------------------------------------------------------------------
  * Custom kind configuration
  *--------------------------------------------------------------------------
@@ -80,6 +114,9 @@ static const PgStat_KindInfo custom_stats = {
 	.shared_data_len = sizeof(((PgStatShared_CustomVarEntry *) 0)->stats),
 	.pending_size = sizeof(PgStat_StatCustomVarEntry),
 	.flush_pending_cb = test_custom_stats_var_flush_pending_cb,
+	.to_serialized_data = test_custom_stats_var_to_serialized_data,
+	.from_serialized_data = test_custom_stats_var_from_serialized_data,
+	.finish = test_custom_stats_var_finish,
 };
 
 /*--------------------------------------------------------------------------
@@ -132,6 +169,309 @@ test_custom_stats_var_flush_pending_cb(PgStat_EntryRef *entry_ref, bool nowait)
 	return true;
 }
 
+/*
+ * test_custom_stats_var_to_serialized_data() -
+ *
+ * Serialize extra data (descriptions) for custom statistics entries to
+ * the statistics file.  Called during statistics file writing to preserve
+ * description strings across restarts.
+ *
+ * This callback writes a mix of data within the main pgstats file and a
+ * secondary file.  The following data is written to the main file for
+ * each entry:
+ * - An arbitrary magic number.
+ * - An offset.  This is used to know the location we need to look at
+ * to retrieve the information from the second file.
+ *
+ * The following data is written to the secondary file:
+ * - The entry key, cross-checked with the data from the main file
+ * when reloaded.
+ * - The length of the description.
+ * - The description data itself.
+ */
+static void
+test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key,
+										 const PgStatShared_Common *header,
+										 FILE *statfile)
+{
+	char	   *description;
+	size_t		len;
+	PgStatShared_CustomVarEntry *entry = (PgStatShared_CustomVarEntry *) header;
+	bool		found;
+	uint32		magic_number = TEST_CUSTOM_VAR_MAGIC_NUMBER;
+
+	/*
+	 * First mark the main file with a magic number, keeping a trace that some
+	 * extra data will exist in the secondary file.
+	 */
+	pgstat_write_chunk_s(statfile, &magic_number);
+
+	/* Open statistics file for writing. */
+	if (!fd_description)
+	{
+		fd_description = AllocateFile(TEST_CUSTOM_EXTRA_DATA_DESC, PG_BINARY_W);
+		if (fd_description == NULL)
+		{
+			ereport(LOG,
+					(errcode_for_file_access(),
+					 errmsg("could not open statistics file \"%s\" for writing: %m",
+							TEST_CUSTOM_EXTRA_DATA_DESC)));
+			return;
+		}
+
+		/* Initialize offset for secondary file. */
+		fd_description_offset = 0;
+	}
+
+	/* Write offset to the main data file */
+	pgstat_write_chunk_s(statfile, &fd_description_offset);
+
+	/*
+	 * First write the entry key to the secondary file.  This will be
+	 * cross-checked with the key read from main stats file at loading time.
+	 */
+	pgstat_write_chunk_s(fd_description, (PgStat_HashKey *) key);
+	fd_description_offset += sizeof(PgStat_HashKey);
+
+	if (!custom_stats_description_dsa)
+		custom_stats_description_dsa = GetNamedDSA("test_custom_stat_dsa", &found);
+
+	/* Handle entries without descriptions */
+	if (!DsaPointerIsValid(entry->description) || !custom_stats_description_dsa)
+	{
+		/* length to description file */
+		len = 0;
+		pgstat_write_chunk_s(fd_description, &len);
+		fd_description_offset += sizeof(size_t);
+		return;
+	}
+
+	/*
+	 * Retrieve description from DSA, then write the length followed by the
+	 * description.
+	 */
+	description = dsa_get_address(custom_stats_description_dsa,
+								  entry->description);
+	len = strlen(description) + 1;
+	pgstat_write_chunk_s(fd_description, &len);
+	pgstat_write_chunk(fd_description, description, len);
+
+	/*
+	 * Update offset for next entry, counting for the length (size_t) of the
+	 * description and the description contents.
+	 */
+	fd_description_offset += len + sizeof(size_t);
+}
+
+/*
+ * test_custom_stats_var_from_serialized_data() -
+ *
+ * Read extra data (descriptions) for custom statistics entries from
+ * the statistics file.  This is called while loading the statistics
+ * at startup.
+ *
+ * See the top of test_custom_stats_var_to_serialized_data() for a
+ * detailed description of the data layout read here.
+ */
+static bool
+test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key,
+										   const PgStatShared_Common *header,
+										   FILE *statfile)
+{
+	PgStatShared_CustomVarEntry *entry;
+	dsa_pointer dp;
+	size_t		len;
+	long		offset;
+	char	   *buffer;
+	bool		found;
+	uint32		magic_number = 0;
+	PgStat_HashKey file_key;
+
+	/* Check the magic number first, in the main file. */
+	if (!pgstat_read_chunk_s(statfile, &magic_number))
+	{
+		elog(WARNING, "failed to read magic number from statistics file");
+		return false;
+	}
+
+	if (magic_number != TEST_CUSTOM_VAR_MAGIC_NUMBER)
+	{
+		elog(WARNING, "found magic number %u from statistics file, should be %u",
+			 magic_number, TEST_CUSTOM_VAR_MAGIC_NUMBER);
+		return false;
+	}
+
+	/*
+	 * Read the offset from the main stats file, to be able to read the extra
+	 * data from the secondary file.
+	 */
+	if (!pgstat_read_chunk_s(statfile, &offset))
+	{
+		elog(WARNING, "failed to read metadata offset from statistics file");
+		return false;
+	}
+
+	/* Open statistics file for reading if not already open */
+	if (!fd_description)
+	{
+		fd_description = AllocateFile(TEST_CUSTOM_EXTRA_DATA_DESC, PG_BINARY_R);
+		if (fd_description == NULL)
+		{
+			if (errno != ENOENT)
+				ereport(LOG,
+						(errcode_for_file_access(),
+						 errmsg("could not open statistics file \"%s\" for reading: %m",
+								TEST_CUSTOM_EXTRA_DATA_DESC)));
+			pgstat_reset_of_kind(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS);
+			return false;
+		}
+	}
+
+	/* Read data from the secondary file, at the specified offset */
+	if (fseek(fd_description, offset, SEEK_SET) != 0)
+	{
+		elog(WARNING, "failed to seek to offset %ld in description file", offset);
+		return false;
+	}
+
+	/* Read the hash key from the secondary file */
+	if (!pgstat_read_chunk_s(fd_description, &file_key))
+	{
+		elog(WARNING, "failed to read hash key from file");
+		return false;
+	}
+
+	/* Check key consistency */
+	if (file_key.kind != key->kind ||
+		file_key.dboid != key->dboid ||
+		file_key.objid != key->objid)
+	{
+		elog(WARNING, "found entry key %u/%u/%" PRIu64 " not matching with %u/%u/%" PRIu64,
+			 file_key.kind, file_key.dboid, file_key.objid,
+			 key->kind, key->dboid, key->objid);
+		return false;
+	}
+
+	entry = (PgStatShared_CustomVarEntry *) header;
+
+	/* Read the description length and its data */
+	if (!pgstat_read_chunk_s(fd_description, &len))
+	{
+		elog(WARNING, "failed to read metadata length from statistics file");
+		return false;
+	}
+
+	/* Handle empty descriptions */
+	if (len == 0)
+	{
+		entry->description = InvalidDsaPointer;
+		return true;
+	}
+
+	/* Initialize DSA if needed */
+	if (!custom_stats_description_dsa)
+		custom_stats_description_dsa = GetNamedDSA("test_custom_stat_dsa", &found);
+
+	if (!custom_stats_description_dsa)
+	{
+		elog(WARNING, "could not access DSA for custom statistics descriptions");
+		return false;
+	}
+
+	buffer = palloc(len);
+	if (!pgstat_read_chunk(fd_description, buffer, len))
+	{
+		pfree(buffer);
+		elog(WARNING, "failed to read description from file");
+		return false;
+	}
+
+	/* Allocate space in DSA and copy the description */
+	dp = dsa_allocate(custom_stats_description_dsa, len);
+	memcpy(dsa_get_address(custom_stats_description_dsa, dp), buffer, len);
+	entry->description = dp;
+	pfree(buffer);
+
+	return true;
+}
+
+/*
+ * test_custom_stats_var_finish() -
+ *
+ * Cleanup function called at the end of statistics file operations.
+ * Handles closing files and cleanup based on the operation type.
+ */
+static void
+test_custom_stats_var_finish(PgStat_StatsFileOp status)
+{
+	switch (status)
+	{
+		case STATS_WRITE:
+			if (!fd_description)
+				return;
+
+			fd_description_offset = 0;
+
+			/* Check for write errors and cleanup if necessary */
+			if (ferror(fd_description))
+			{
+				ereport(LOG,
+						(errcode_for_file_access(),
+						 errmsg("could not write statistics file \"%s\": %m",
+								TEST_CUSTOM_EXTRA_DATA_DESC)));
+				FreeFile(fd_description);
+				unlink(TEST_CUSTOM_EXTRA_DATA_DESC);
+			}
+			else if (FreeFile(fd_description) < 0)
+			{
+				ereport(LOG,
+						(errcode_for_file_access(),
+						 errmsg("could not close statistics file \"%s\": %m",
+								TEST_CUSTOM_EXTRA_DATA_DESC)));
+				unlink(TEST_CUSTOM_EXTRA_DATA_DESC);
+			}
+			break;
+
+		case STATS_READ:
+			if (fd_description)
+				FreeFile(fd_description);
+
+			/* Remove the temporary statistics file after reading */
+			elog(DEBUG2, "removing statistics file \"%s\"", TEST_CUSTOM_EXTRA_DATA_DESC);
+			unlink(TEST_CUSTOM_EXTRA_DATA_DESC);
+			break;
+
+		case STATS_DISCARD:
+			{
+				int			ret;
+
+				/* Attempt to remove the statistics file */
+				ret = unlink(TEST_CUSTOM_EXTRA_DATA_DESC);
+				if (ret != 0)
+				{
+					if (errno == ENOENT)
+						elog(LOG,
+							 "didn't need to unlink permanent stats file \"%s\" - didn't exist",
+							 TEST_CUSTOM_EXTRA_DATA_DESC);
+					else
+						ereport(LOG,
+								(errcode_for_file_access(),
+								 errmsg("could not unlink permanent statistics file \"%s\": %m",
+										TEST_CUSTOM_EXTRA_DATA_DESC)));
+				}
+				else
+				{
+					ereport(LOG,
+							(errmsg_internal("unlinked permanent statistics file \"%s\"",
+											 TEST_CUSTOM_EXTRA_DATA_DESC)));
+				}
+			}
+			break;
+	}
+
+	fd_description = NULL;
+}
+
 /*--------------------------------------------------------------------------
  * Helper functions
  *--------------------------------------------------------------------------
@@ -162,8 +502,7 @@ test_custom_stats_var_fetch_entry(const char *stat_name)
  * test_custom_stats_var_create
  *		Create new custom statistic entry
  *
- * Initializes a zero-valued statistics entry in shared memory.
- * Validates name length against NAMEDATALEN limit.
+ * Initializes a statistics entry with the given name and description.
  */
 PG_FUNCTION_INFO_V1(test_custom_stats_var_create);
 Datum
@@ -172,6 +511,9 @@ test_custom_stats_var_create(PG_FUNCTION_ARGS)
 	PgStat_EntryRef *entry_ref;
 	PgStatShared_CustomVarEntry *shared_entry;
 	char	   *stat_name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+	char	   *description = text_to_cstring(PG_GETARG_TEXT_PP(1));
+	dsa_pointer dp = InvalidDsaPointer;
+	bool		found;
 
 	/* Validate name length first */
 	if (strlen(stat_name) >= NAMEDATALEN)
@@ -180,6 +522,20 @@ test_custom_stats_var_create(PG_FUNCTION_ARGS)
 				 errmsg("custom statistic name \"%s\" is too long", stat_name),
 				 errdetail("Name must be less than %d characters.", NAMEDATALEN)));
 
+	/* Initialize DSA and description provided */
+	if (!custom_stats_description_dsa)
+		custom_stats_description_dsa = GetNamedDSA("test_custom_stat_dsa", &found);
+
+	if (!custom_stats_description_dsa)
+		ereport(ERROR,
+				(errmsg("could not access DSA for custom statistics descriptions")));
+
+	/* Allocate space in DSA and copy description */
+	dp = dsa_allocate(custom_stats_description_dsa, strlen(description) + 1);
+	memcpy(dsa_get_address(custom_stats_description_dsa, dp),
+		   description,
+		   strlen(description) + 1);
+
 	/* Create or get existing entry */
 	entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS, InvalidOid,
 											PGSTAT_CUSTOM_VAR_STATS_IDX(stat_name), true);
@@ -192,6 +548,9 @@ test_custom_stats_var_create(PG_FUNCTION_ARGS)
 	/* Zero-initialize statistics */
 	memset(&shared_entry->stats, 0, sizeof(shared_entry->stats));
 
+	/* Store description pointer */
+	shared_entry->description = dp;
+
 	pgstat_unlock_entry(entry_ref);
 
 	PG_RETURN_VOID();
@@ -226,8 +585,7 @@ test_custom_stats_var_update(PG_FUNCTION_ARGS)
  * test_custom_stats_var_drop
  *		Remove custom statistic entry
  *
- * Drops the named statistic from shared memory and requests
- * garbage collection if needed.
+ * Drops the named statistic from shared memory.
  */
 PG_FUNCTION_INFO_V1(test_custom_stats_var_drop);
 Datum
@@ -247,7 +605,7 @@ test_custom_stats_var_drop(PG_FUNCTION_ARGS)
  * test_custom_stats_var_report
  *		Retrieve custom statistic values
  *
- * Returns single row with statistic name and call count if the
+ * Returns single row with statistic name, call count, and description if the
  * statistic exists, otherwise returns no rows.
  */
 PG_FUNCTION_INFO_V1(test_custom_stats_var_report);
@@ -281,9 +639,13 @@ test_custom_stats_var_report(PG_FUNCTION_ARGS)
 
 	if (funcctx->call_cntr < funcctx->max_calls)
 	{
-		Datum		values[2];
-		bool		nulls[2] = {false, false};
+		Datum		values[3];
+		bool		nulls[3] = {false, false, false};
 		HeapTuple	tuple;
+		PgStat_EntryRef *entry_ref;
+		PgStatShared_CustomVarEntry *shared_entry;
+		char	   *description = NULL;
+		bool		found;
 
 		stat_name = text_to_cstring(PG_GETARG_TEXT_PP(0));
 		stat_entry = test_custom_stats_var_fetch_entry(stat_name);
@@ -291,9 +653,33 @@ test_custom_stats_var_report(PG_FUNCTION_ARGS)
 		/* Return row only if entry exists */
 		if (stat_entry)
 		{
+			/* Get entry ref to access shared entry */
+			entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS, InvalidOid,
+											 PGSTAT_CUSTOM_VAR_STATS_IDX(stat_name), false, NULL);
+
+			if (entry_ref)
+			{
+				shared_entry = (PgStatShared_CustomVarEntry *) entry_ref->shared_stats;
+
+				/* Get description from DSA if available */
+				if (DsaPointerIsValid(shared_entry->description))
+				{
+					if (!custom_stats_description_dsa)
+						custom_stats_description_dsa = GetNamedDSA("test_custom_stat_dsa", &found);
+
+					if (custom_stats_description_dsa)
+						description = dsa_get_address(custom_stats_description_dsa, shared_entry->description);
+				}
+			}
+
 			values[0] = PointerGetDatum(cstring_to_text(stat_name));
 			values[1] = Int64GetDatum(stat_entry->numcalls);
 
+			if (description)
+				values[2] = PointerGetDatum(cstring_to_text(description));
+			else
+				nulls[2] = true;
+
 			tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls);
 			SRF_RETURN_NEXT(funcctx, HeapTupleGetDatum(tuple));
 		}
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9dd65b102544..14f7f8217491 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2269,6 +2269,7 @@ PgStat_StatFuncEntry
 PgStat_StatReplSlotEntry
 PgStat_StatSubEntry
 PgStat_StatTabEntry
+PgStat_StatsFileOp
 PgStat_SubXactStatus
 PgStat_TableCounts
 PgStat_TableStatus
-- 
2.51.0

---

From: Sami Imseih <[email-masked]>
Date: Fri, 12 Dec 2025 18:41:20 -0600

Thanks for the updates!

> are exposing these APIs, let's use them.

oops. That totally slipped my mind :( sorry about that.

> end-of-operation actions and to/from_serialized_data.

At first I wasn’t a fan of the name “finish” for the callback.
I was thinking of calling it “finish_auxiliary”. But, we’re not
forcing callbacks to be used together, and there could perhaps
be cases where “finish" can be used on its own, so this is fine by me.

I made some changes as well, in v8:

1/ looks like b4cbc106a6ce snuck into v7. I fixed that.

2/ After looking this over, I realized that “extra” and “auxiliary”
were being used interchangeably. To avoid confusion, I replaced all
instances of “extra” with “auxiliary" in both the comments and
macros, i.e. TEST_CUSTOM_AUX_DATA_DESC

---

From: Michael Paquier <[email-masked]>
Date: Sat, 13 Dec 2025 11:00:30 +0900

> 1/ looks like b4cbc106a6ce snuck into v7. I fixed that.

Oops, sorry about that.  I went one reset too deep..  I can see that
my local branch was also wrong, an a rebase fixed it immediately.

> macros, i.e. TEST_CUSTOM_AUX_DATA_DESC

I can see what you have changed in v8 compared to v7, in terms of the
elog(), the comments and TEST_CUSTOM_AUX_DATA_DESC.  That works for
me.  If somebody has a better idea for a name, these can always be
tweaked at will.
--
Michael

---

From: Sami Imseih <[email-masked]>
Date: Sat, 13 Dec 2025 18:33:41 -0600

I just remembered that we should document the new callbacks in [0] with a
brief explanation of their purpose and a reference to test_custom_stats
as an example of usage. What do you think?

[0]
https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-ADDIN-CUSTOM-CUMULATIVE-STATISTICS

---

From: Sami Imseih <[email-masked]>
Date: Sat, 13 Dec 2025 19:55:04 -0600

> as an example of usage. What do you think?

oh, and I also realized that the documentation was updated incorrectly when
test_custom_stats was originally committed. Thought it was better to fix this
in a separate thread [0].

[0] https://www.postgresql.org/message-id/[email-masked]

---

From: Michael Paquier <[email-masked]>
Date: Mon, 15 Dec 2025 09:55:43 +0900

> as an example of usage. What do you think?

I'd rather keep the documentation simpler, pointing only to the
code templates we have and pgstat_internal.h.  One reason is that code
in the documentation tends to rot very easily, particularly when
applied to plugin APIs.  If you think that some of the callbacks of
pgstat_internal.h deserve more documentation or explanation, let's do
that directly in the header.

Saying that, I have tweaked a bit more the patch this morning and
applied the result after splitting things in two: one for the core
backend changes and one for the tests of the new APIs.  Some comments
and error strings have been simplified and I have noticed some more
inconsistencies after a follow-up read.

Another thing that I did not like is the use of "long" for the offset,
which is not portable.  We have a drop-in portable replacement for
seeks and offsets: fseeko() and pgoff_t.  That was in the test code,
but still let's keep things more portable in the long run without a
4-byte limitation on WIN32.

I guess that we are done for this thread then.
--
Michael

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 17 Dec 2025 08:03:36 +0100

> macros, i.e. TEST_CUSTOM_AUX_DATA_DESC

The function test_custom_stats_var_from_serialized_data() takes an 
argument of type

     const PgStatShared_Common *header

which is then later cast

     entry = (PgStatShared_CustomVarEntry *) header;

where entry is defined as

     PgStatShared_CustomVarEntry *entry;

So you are losing the const qualification here.

But fixing that by adding the const qualification to entry would not 
work because what entry points to is later modified:

     entry->description = InvalidDsaPointer;

So the header argument of the function should not be const qualified.

But the signature of that function is apparently determined by this new 
callbacks API, so it cannot be changed in isolation.

So it seems to me that either the callbacks API needs some adjustments, 
or this particular implementation of the callback function is incorrect.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 17 Dec 2025 17:11:19 +0900

On Wed, Dec 17, 2025 at 08:03:36AM +0100, Peter Eisentraut wrote:
> So it seems to me that either the callbacks API needs some adjustments, or
> this particular implementation of the callback function is incorrect.

Hmm, you are right that this is not aligned.  This can be improved
with one change for each callback:
- It is OK with from_serialized_data() to manipulate the header data,
because we want to fill a portion of the shmem data with extra data
read from disk (the module wants to add a reference to a DSA stored in
the shmem entry, read from the second file).  So we should discard the
const marker from the callback definition.
- The const usage is OK for to_serialized_data(): it is better to
encourage a policy where the header data cannot be manipulated.  So
the const needs to be kept in the definition, but I also think that we
should change the module implementation so as the cast to
PgStatShared_CustomVarEntry is a const.

These changes result in the attached.  Sami, what do you think?
--
Michael
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 5c1ce4d3d6af..01db3b701bdf 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -335,7 +335,7 @@ typedef struct PgStat_KindInfo
 									   const PgStatShared_Common *header,
 									   FILE *statfile);
 	bool		(*from_serialized_data) (const PgStat_HashKey *key,
-										 const PgStatShared_Common *header,
+										 PgStatShared_Common *header,
 										 FILE *statfile);
 
 	/*
diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats.c b/src/test/modules/test_custom_stats/test_custom_var_stats.c
index c71922dc4a8f..3dea38e3fe83 100644
--- a/src/test/modules/test_custom_stats/test_custom_var_stats.c
+++ b/src/test/modules/test_custom_stats/test_custom_var_stats.c
@@ -92,7 +92,7 @@ static void test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key,
 
 /* Deserialization callback: read auxiliary entry data */
 static bool test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key,
-													   const PgStatShared_Common *header,
+													   PgStatShared_Common *header,
 													   FILE *statfile);
 
 /* Finish callback: end of statistics file operations */
@@ -196,9 +196,10 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key,
 {
 	char	   *description;
 	size_t		len;
-	PgStatShared_CustomVarEntry *entry = (PgStatShared_CustomVarEntry *) header;
 	bool		found;
 	uint32		magic_number = TEST_CUSTOM_VAR_MAGIC_NUMBER;
+	const PgStatShared_CustomVarEntry *entry =
+		(const PgStatShared_CustomVarEntry *) header;
 
 	/*
 	 * First mark the main file with a magic number, keeping a trace that some
@@ -276,7 +277,7 @@ test_custom_stats_var_to_serialized_data(const PgStat_HashKey *key,
  */
 static bool
 test_custom_stats_var_from_serialized_data(const PgStat_HashKey *key,
-										   const PgStatShared_Common *header,
+										   PgStatShared_Common *header,
 										   FILE *statfile)
 {
 	PgStatShared_CustomVarEntry *entry;

---

From: Sami Imseih <[email-masked]>
Date: Wed, 17 Dec 2025 11:01:01 -0600

> These changes result in the attached.  Sami, what do you think?

I agree. This was a miss during the review. Thanks for raising this.

The fix looks correct to me in which the from_serialized_data callback
is expected to modify the header, to reconstruct the entry and the
to_serialized_data is never expected to modify the header, since we
are only reading what is currently in stats. I can't think of a reason to
ever have to modify the entry while writing out to disk.

I got the attached patch ready with some additional comments in
the callback definitions to clarify the API contract. We only need
to call out the "header' nuance since it's a const in one callback
and not the other. "key" is self documenting being a const in both
cases.

---

From: Michael Paquier <[email-masked]>
Date: Thu, 18 Dec 2025 07:42:00 +0900

> and not the other. "key" is self documenting being a const in both

The comment sounds like a good idea, so included it and applied the
result.  Thanks for looking!
--
Michael

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 9 Jul 2026 11:10:18 +0200

> blast, and applied that as it can also be useful on its own.

I came across this now.  I'm concerned that these write/read routines 
for stats files are a bit weirdly defined.

They were originally internal to pgstat.c where they were specifically 
tailored to the control flow in that file.  For example, 
pgstat_write_chunk() contains an internal comment "We check for errors 
with ferror() when done writing the stats.", which is fine as a 
description of what happens in that file, but it's pretty poor as an API 
documentation.  On the other hand, pgstat_read_chunk() does do some 
error checking, but completely undocumented and weirdly inconsistent 
with the write counterpart.  Again, this might be fine for an internal 
helper but not for an external API.

I also don't think these functions are well-named.  They don't do 
anything about "pgstat", they are just convenience wrappers around 
fread() and fwrite() with some sizeof integration (which is also kind of 
fragile, because there is no type checking about what you are passing). 
I have not found any documentation about this new feature for pluggable 
statistics, so it's not clear whether these functions are meant to be 
part of the official API for that, or they just happened to be available 
for implementing that test module.

I suggest that unless we can come up with a well-defined, robust, and 
well-documented API for these, they should be moved back to be internal 
to pgstat.c, and test_custom_var_stats.c can define its own helper 
functions.  Maybe in the future, when we have more of these, some good 
API will emerge.