MIME-Version: 1.0
References: 
 <CAA5RZ0s9SDOu+Z6veoJCHWk+kDeTktAtC-KY9fQ9Z6BJdDUirQ@mail.gmail.com>
 <aPmh7uoW6_EmxFHd@paquier.xyz>
In-Reply-To: <aPmh7uoW6_EmxFHd@paquier.xyz>
From: Sami Imseih <samimseih@gmail.com>
Date: Thu, 23 Oct 2025 16:35:58 -0500
Message-ID: 
 <CAA5RZ0ux07=E97mJ3JCy2jhs349NKzj7SPJNbDWZ11m9BDkTQQ@mail.gmail.com>
Subject: Re: [Proposal] Adding callback support for custom statistics kinds
To: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA5RZ0ux07%3DE97mJ3JCy2jhs349NKzj7SPJNbDWZ11m9BDkTQQ%40mail.gmail.com>
Precedence: bulk

Thanks for the feedback!

> > 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.
>
> Hmm.  Is that really what we want here?  This pretty says that one
> 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?

> 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

Sure, we can go that route.

> - 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.

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.

> - 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.

I will provide a patch with the recommendations.

--
Sami