Re: Extended Statistics set/restore/clear functions.

On Mon, Jan 19, 2026 at 10:01:28AM +0800, Chao Li wrote:
> Comments for v27:
>
> I think we should also pfree(s) before ereport. From
> extended_statistics_update(), this function is called with
> elevel=WARNING, this ereport will not immediately fail out, so
> That s is leaked.

Yeah, it's true that we could make an extra effort for this one. I
was also wondering how much a pfree() actually matters here, and if we
could just remove it, but it may not be a good idea if we begin to
call import_mcvlist() in a different context than the one of this
patch.

Here we should have a statext_mcv_free() to do some cleanup. There is
a bit more in this code path that I am spotting on review. As of one
thing: vatuples is not needed in import_mcv().

> vatuples[I] is returned by SearchSysCacheCopy1(), I believe it
> should be free by heap_freetuple(). See the header comment of
> SearchSysCacheCopy().

Here I believe that the issue is different: vatuples is not needed at
all, and can be removed. We use it nowhere in when importing a MCV
list.

> I just feel the head comment needs some enhancement, because it
> doesn’t explain what this function does, what are input and
> output. The current comment seems only to only explain why this
> function exists.

Yep, it needs much, much more description. I have been trying to
clean up the whole, but this patch needs a lot more work before being
mergeable.

> Just feel “k” should be captain: StaKindFlags.

Does not matter much.

> Should “stxname” be “nspname”?
>
> extended_statistics_update is defined to return a bool, but there
> are multiple branches returning Datum: "PG_RETURN_BOOL(false)";
> "return (Datum) 0”.

Obviously.

> 7 - 0001 - extended_stats_funcs.c
> ```
> +static Datum
> +warn_type_mismatch(Datum d, const char *argname)
>
> Why this function needs to return a Datum?

Does not make sense to me either, this should use a (void) as returned
value.

> 8 - 0001 - extended_stats_funcs.c
> This function only partially assign individual fields of “enabled”
> without zero it out, so it relies on the caller to zero out
> “enabled” before calling this function. Now
> extended_statistics_update() has done a memset(), but I just feel it
> would be more robust if this function does the memset.

My first opinion about this API was that as designed it was a mistake,
but on second opinion I can see why this has been coded as it is, for
consistency with how we treat "has", so at this end I'm OK with it
as-is. Based on the fact that we have only one caller, the memset()
done in the upper caller does not matter, it's just a matter of author
style.

> Also, this function silently discard unknown staKind, should we at
> least Assert(false) against unknown staKind?

Hmm, this is data read from the catalogs, so an assert() is out of
place. An ERROR with a switch/case, though, would be much cleaner.

> 9 - 0001 - extended_stats_funcs.c
>
> Do we need to pfree(s) before return?

Yep.

> 10 - 0001 - extended_stats_funcs.c
>
> I think we should free pgstup because this is a loop.

I don't think that this one matters much in this caller context.

> 12 - 0001
>
> I don’t see where “version” is implemented.

It seems to me that you are missing stats_fill_fcinfo_from_arg_pairs().

> 13 - 0002
>
> Looks like e.stxname should be s.stxname.
>
> Also, pg_catalog.pg_statistics_ext should be
> pg_catalog.pg_statistic_ext.

Yep. That's clearly broken when it comes to querying a v11 server.

> "constraint-specific details” looks like a copy-paste error.
>
> Typo: ndistinnct -> ndistinct, depdendencies -> dependencies

Obviously.

(Still looking at the patch in more details now, putting my hands on
it..)
--
Michael