MIME-Version: 1.0
References: 
 <CAA5RZ0s9SDOu+Z6veoJCHWk+kDeTktAtC-KY9fQ9Z6BJdDUirQ@mail.gmail.com>
 <aPmh7uoW6_EmxFHd@paquier.xyz>
 <CAA5RZ0ux07=E97mJ3JCy2jhs349NKzj7SPJNbDWZ11m9BDkTQQ@mail.gmail.com>
 <CAA5RZ0tnopAS4MCcoxXdJShYoGxYZgsoDXCjcie0Q-2TBPuJwA@mail.gmail.com>
 <aRJzdl_cSc3XHX85@paquier.xyz>
 <CAA5RZ0skT-uWwgrCg3Hj3AQaBbQdN0h72scD9GBLxH1nEKSNwQ@mail.gmail.com>
 <CAA5RZ0tH6ih3anQ=Lcwttm+8BURJ8VkBoy_9V8Ty73ZA1r384Q@mail.gmail.com>
 <aSOidfDnBT4lbtHp@paquier.xyz>
In-Reply-To: <aSOidfDnBT4lbtHp@paquier.xyz>
From: Sami Imseih <samimseih@gmail.com>
Date: Sun, 23 Nov 2025 22:43:17 -0600
Message-ID: 
 <CAA5RZ0tvDvHYYLnu-T0DRK0xv1qv2_EK1yHjth5uAqDWz_GvfA@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/CAA5RZ0tvDvHYYLnu-T0DRK0xv1qv2_EK1yHjth5uAqDWz_GvfA%40mail.gmail.com>
Precedence: bulk

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

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

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