> On Jan 15, 2026, at 16:09, Michael Paquier <[email protected]> wrote:
>
> On Wed, Jan 14, 2026 at 12:57:45PM -0500, Corey Huinker wrote:
>> Some of the header cleanup work required adding utils/typcache.h to
>> extended_stats.c.
>
> Thanks for sending a rebased set. It is a large patch, but most of
> the changes are super mechanical, making it sort of "simpler" to think
> about the whole.
>
> Anyway, I have begun a review of the backend changes, and found one
> independent piece that was useful, leading to 32e27bd32082 that I have
> just applied after some renames and a couple of fixes.
>
> Then I have spent a good portion of today looking at the backend
> changes, with first a focus to extract the "clear" function as it is
> useful on its own, also because its relation lookup logic is the same
> as the restore function.
>
> And then, this block of code has been giving me a long pause, because
> it was fishy, and clearly wrong:
> + /* no need to fetch reloid, we already have it */
> + RangeVarGetRelidExtended(makeRangeVar(nspname,
> + RelationGetRelationName(rel), -1),
> + ShareUpdateExclusiveLock, 0,
> + RangeVarCallbackForStats, &locked_table);
>
> A shocking thing here is that we build a RangeVar for the relation of
> the extended stats object based on the *extstat namespace*, not the
> *relation namespace*, and that we do so *after* opening the extstat
> catalog. Anyway, I think that we need to redesign that, and for
> consistency's sake do a RangeVar lookup *before* even trying to touch
> the stats data or open its catalogs. That would be beneficial on
> consistency ground, and one piece where I think that patch is designed
> wrongly is that we should give in input of the new clear and restore
> functions the *schema* and *relation* names of the table we expect to
> find, so as we can enforce first a clean RangeVar lookup, then
> manipulate the stats data. This relates to 688dc6299a5b, as well,
> except that we would be stuck with an incorrect design after release
> if we are not careful because the function schema would be stuck in
> time. This has to be right from the start.
>
> All these changes have given me the attached patch for the clear()
> function. Another piece is that we did not really check that a role
> has the MAINTAIN rights of the relation where we want to manipulate
> the extended stats.
>
> A final thing is the location of the new SQL function code, let's put
> that into a new file, named extended_stats_funcs.c in the patch.
> There is nothing in the new restore and clear functions that require
> extended_stats.c.
>
> Finally, for the clear() part, the attached version addresses
> everything I have found during my review. We will need to make the
> restore() part follow the same design model with the Rangevar lookups
> of the parent relation, or we'll have trouble waiting ahead.
> --
> Michael
> <v24-0001-Add-pg_clear_extended_stats.patch>
Hi Michael,
Here are some comments for v24:
1.
```
+ inherited = PG_GETARG_NAME(INHERITED_ARG);
```
Should this use PG_GETARG_BOOL()?
2
```
proparallel => 'u', prorettype => 'void', proargtypes => 'text text text text
bool’,
```
Should we define proargtypes as “name name name name bool”? As the doc change
mentions type “name” for the first 4 parameters. Maybe you want to avoid that
because Name is a structure. Anyway, I don’t have a strong opinion here.
3 - extended_stats_funcs.c
```
* Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
```
This is a brand new file, so the copyright year should be just 2026. If you
search for “2025-2026”, you will get a bunch of files, they should be created
in 2025.
4 - Given comment 1, no test fails. I think that’s because all test cases use
inherited = false. Maybe we need a test case for inherited = true.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
