Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: [Proposal] Adding callback support for custom statistics kinds
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAA5RZ0ufds2r3H7QUro_yJDrnUmsQ4ocufqgcsRGoZvxQKeMrA@mail.gmail.com>
Date: Wed, 3 Dec 2025 13:41:44 +0800
Cc: Michael Paquier <michael@paquier.xyz>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <4FEEC2B8-D9CF-4BD0-AA7E-44CE3E27ADD8@gmail.com>
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>
 <CAA5RZ0thcqk_r_SCB3TdXP9hVV5mG_29yc9GaGr4ujPQmcLDEQ@mail.gmail.com>
 <aSUOaqT53UBIYTKh@paquier.xyz>
 <CAA5RZ0ufds2r3H7QUro_yJDrnUmsQ4ocufqgcsRGoZvxQKeMrA@mail.gmail.com>
To: Sami Imseih <samimseih@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/4FEEC2B8-D9CF-4BD0-AA7E-44CE3E27ADD8%40gmail.com>
Precedence: bulk


> On Dec 3, 2025, at 04:58, Sami Imseih <samimseih@gmail.com> wrote:
>=20
>>> 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.
>>=20
>> 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?
>=20
> By "template" code, do you mean Something like?
>=20
> include/utils/custom_statkinds.h
> backend/utils/misc/custom_statkinds.c
>=20
> 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.
>=20
> 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.
>=20
> v2 attached is something that may be closer to what we've been =
discussing
>=20
> 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.
>=20
> v2-0002 implements a new test module that tests mainly that the =
recovery,
> clean and crash, are working as expected.
>=20
> 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?
>=20
> [0] =
https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-ADDIN-CUSTOM-CU=
MULATIVE-STATISTICS
>=20
>=20
> --
> Sami Imseih
> Amazon Web Services (AWS)
> =
<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=E2=80=
=99s 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/