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
