Feedback-ID: ie0a040ee:Fastmail
Message-ID: <d87a93b0-19c7-4db6-b9c0-d6827e7b2da1@eisentraut.org>
Date: Wed, 17 Dec 2025 08:03:36 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Proposal] Adding callback support for custom statistics kinds
To: Sami Imseih <samimseih@gmail.com>, Michael Paquier <michael@paquier.xyz>
Cc: Chao Li <li.evan.chao@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: 
 <CAA5RZ0sg6gskHRgfymo9njEWuXDyn0Zwe+0bcX=nByyE7W+6bw@mail.gmail.com>
 <aTepXZ97PsXpuywI@paquier.xyz>
 <BF27BA43-D94D-4DC6-BD91-95EEAD42AD0F@gmail.com>
 <9252DA7C-C611-4A93-A484-E6CD42557C15@gmail.com>
 <CAA5RZ0vQvmatp8VAk6zroyeKDCw+QVq4XmoRNAFogQo6bJEbNQ@mail.gmail.com>
 <4E61FDC4-CFBB-4116-980F-75ADD245A1F6@gmail.com>
 <CAA5RZ0swDCK+7M2KQip0tqUsBK3LUHd3F0ytpsKdDCfygHksKg@mail.gmail.com>
 <aTkpgP9Xe5QJ5sDu@paquier.xyz>
 <CAA5RZ0uC4j2QGK85ma6bkF6YOWBrYX9J3dV2CEMg3d0CBLpHSA@mail.gmail.com>
 <aTuE7kyeDCggXqVf@paquier.xyz> <aTvi5ceF-pB1IbDR@paquier.xyz>
 <CAA5RZ0ubfcmkkY7i07CmrkY12H8BsRXDoOutYXjYz0gsq7hm-w@mail.gmail.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: 
 <CAA5RZ0ubfcmkkY7i07CmrkY12H8BsRXDoOutYXjYz0gsq7hm-w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/d87a93b0-19c7-4db6-b9c0-d6827e7b2da1%40eisentraut.org>
Precedence: bulk

On 13.12.25 01:41, Sami Imseih wrote:
> Thanks for the updates!
> 
>> - Less fwrite() and fread(), more read_chunk() and write_chunk().  We
>> are exposing these APIs, let's use them.
> 
> oops. That totally slipped my mind :( sorry about that.
> 
>> - The callbacks are renamed, to be more generic: "finish" for the
>> end-of-operation actions and to/from_serialized_data.
> 
> At first I wasn’t a fan of the name “finish” for the callback.
> I was thinking of calling it “finish_auxiliary”. But, we’re not
> forcing callbacks to be used together, and there could perhaps
> be cases where “finish" can be used on its own, so this is fine by me.
> 
> I made some changes as well, in v8:
> 
> 1/ looks like b4cbc106a6ce snuck into v7. I fixed that.
> 
> 2/ After looking this over, I realized that “extra” and “auxiliary”
> were being used interchangeably. To avoid confusion, I replaced all
> instances of “extra” with “auxiliary" in both the comments and
> macros, i.e. TEST_CUSTOM_AUX_DATA_DESC

The function test_custom_stats_var_from_serialized_data() takes an 
argument of type

     const PgStatShared_Common *header

which is then later cast

     entry = (PgStatShared_CustomVarEntry *) header;

where entry is defined as

     PgStatShared_CustomVarEntry *entry;

So you are losing the const qualification here.

But fixing that by adding the const qualification to entry would not 
work because what entry points to is later modified:

     entry->description = InvalidDsaPointer;

So the header argument of the function should not be const qualified.

But the signature of that function is apparently determined by this new 
callbacks API, so it cannot be changed in isolation.

So it seems to me that either the callbacks API needs some adjustments, 
or this particular implementation of the callback function is incorrect.