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
signature.asc
Description: PGP signature
