Re: Extended Statistics set/restore/clear functions. - Mailing list pgsql-hackers

> On Jan 15, 2026, at 16:09, Michael Paquier <michael@paquier.xyz> 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
bunchof 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
casefor inherited = true.  

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/