MIME-Version: 1.0
References: <aTZxqzPWr0wMVLq2@paquier.xyz>
 <CAA5RZ0tST1=piXSQwRn2y+YB-VwS+Dtr0Wk27Dw7ED+Xp6qSXQ@mail.gmail.com>
 <A5D8954F-2E20-4BF5-BA0E-79FDC06A632F@gmail.com>
 <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>
In-Reply-To: <aTkpgP9Xe5QJ5sDu@paquier.xyz>
From: Sami Imseih <samimseih@gmail.com>
Date: Wed, 10 Dec 2025 12:36:36 -0600
Message-ID: 
 <CAA5RZ0uC4j2QGK85ma6bkF6YOWBrYX9J3dV2CEMg3d0CBLpHSA@mail.gmail.com>
Subject: Re: [Proposal] Adding callback support for custom statistics kinds
To: Michael Paquier <michael@paquier.xyz>
Cc: Chao Li <li.evan.chao@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA5RZ0uC4j2QGK85ma6bkF6YOWBrYX9J3dV2CEMg3d0CBLpHSA%40mail.gmail.com>
Precedence: bulk

> Now, it is kind of true that the STATS_WRITE case feels a bit
> disturbing written this way: we let a module take an action, but we
> don't actually know the state of the main pgstats file when inside the
> callback.  I mean, you can know how things are going on, but it means
> that a module can just rely on a check if
> PGSTAT_STAT_PERMANENT_FILENAME is on disk, but an unlink() could have
> failed as well.  So, yes, I am wondering whether we should do what
> Chao is suggesting, passing an extra state to the callback to let the
> module know if we have actually succeeded or failed the operations
> that have been taken on the main stats file before the callback
> end_extra_stats is called in the three cases.  It does not matter for
> the STATS_READ case, but it may matter for the STATS_DISCARD or
> STATS_WRITE case.

I am having a hard time being convinced that this extra status is needed.
I am not expecting an extension to operate on the main stats file inside
the end_extra_stats callback, and even if some operation failed on the
main stats file, the cleanup callback will need to take the steps to
perform the cleanup on its own resources.

Is there a concrete example?

> FWIW, I still have the same question as the one posted here about the
> business in pgstat_initialize(), still present in v6:
> https://www.postgresql.org/message-id/aTepXZ97PsXpuywI@paquier.xyz
>
> This remains unanswered.

Responding to the questions from the thread above.

> Why does this part need to run each time a backend initializes its
> access to pgstats?

Good point. This is unnecessary. This validation should really be
done inside StatsShmemInit by postmaster.

> By the way, checking that to_serialized_extra_stats and
> kind_info->from_serialized_extra_stats need to be both defined is
> fine as these are coupled together, but I am not following the reason
> why end_extra_stats would need to be included in the set? For
> example, a stats kind could decide to add some data to the main
> pgstats file without creating extra files, hence they may not need to
> define end_extra_stats.

.. and after giving this more thought, I actually don't think we should
do any validation for any of the callbacks. If an extension is writing
to any file ( core or custom ), naturally they will want to read it back.
Now I am not sure what these validations are protecting us against.
Also, maybe the extension wants to just read data from the main stats
file, I could see that use-case, perhaps.

So, I am proposing removing the validation altogether. What do
you think?

--
Sami Imseih
Amazon Web Services (AWS)