Feedback-ID: ie0a040ee:Fastmail
Message-ID: <a4a8e9af-3eaf-4bbf-9b21-21620f3fc434@eisentraut.org>
Date: Thu, 9 Jul 2026 11:10:18 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Proposal] Adding callback support for custom statistics kinds
To: Michael Paquier <michael@paquier.xyz>, Sami Imseih <samimseih@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <aSUOaqT53UBIYTKh@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/a4a8e9af-3eaf-4bbf-9b21-21620f3fc434%40eisentraut.org>
Precedence: bulk

On 25.11.25 03:03, Michael Paquier wrote:
> 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.

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.