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>
 <CAA5RZ0tvDvHYYLnu-T0DRK0xv1qv2_EK1yHjth5uAqDWz_GvfA@mail.gmail.com>
In-Reply-To: 
 <CAA5RZ0tvDvHYYLnu-T0DRK0xv1qv2_EK1yHjth5uAqDWz_GvfA@mail.gmail.com>
From: Sami Imseih <samimseih@gmail.com>
Date: Mon, 24 Nov 2025 18:18:26 -0600
Message-ID: 
 <CAA5RZ0thcqk_r_SCB3TdXP9hVV5mG_29yc9GaGr4ujPQmcLDEQ@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/CAA5RZ0thcqk_r_SCB3TdXP9hVV5mG_29yc9GaGr4ujPQmcLDEQ%40mail.gmail.com>
Precedence: bulk

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

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?

--
Sami Imseih
Amazon Web Services (AWS)