Re: Extended Statistics set/restore/clear functions.

> On Jan 17, 2026, at 08:13, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Fri, Jan 16, 2026 at 03:50:50PM +0900, Michael Paquier wrote:
>> Note that the restore function needs tests for the inherited record
>> case, and that we had the same error as the clear function when
>> grabbing the argument value for inherited. I have fixed the bug, did
>> not include any tests yet. Another thing that I find lacking is
>> coverage for the many paths that refer to incorrect input formats. A
>> lot of them are not covered at all, which is not good. I did not look
>> at the pg_dump portion yet.
>
> Here's a v27, with pg_dump/pg_upgrade fixed to handle the new version
> of the restore function with the table name and its schema specified
> in input arguments.
> --
> Michael
> <v27-0001-Add-pg_restore_extended_stats.patch><v27-0002-Include-Extended-Statistics-in-pg_dump.patch>

Comments for v27:

1 - 0001 - mcv.c
```
+ else
+ {
+ char *s = TextDatumGetCString(mcv_elems[index]);
+ ErrorSaveContext escontext = {T_ErrorSaveContext};
+
+ if (!InputFunctionCallSafe(&finfo, s, ioparam, atttypmods[j],
+ (Node *) &escontext, &item->values[j]))
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("MCV elemement \"%s\" does not match expected input type.", s)));
+ return (Datum) 0;
+ }
+
+ pfree(s);
+ }
```

* 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.

* We should also free mcvlist before the return, as well as item->values and item->isnull.

* There is a typo in the error message. elemement -> element.

2 - 0001 - mcv.c
```
+ for (int i = 0; i < numattrs; i++)
+ {
+ pfree(vatuples[i]);
```

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

3 - 0001 - mcv.c
```
+/*
+ * The MCV is an array of records, but this is expected as 4 separate arrays.
+ * It is not possible to have a generic input function for pg_mcv_list
+ * because the most_common_values is a composite type with element types
+ * defined by the specific statistics object.
+ */
+Datum
+import_mcvlist(HeapTuple tup, int elevel, int numattrs, Oid *atttypids,
```

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.

4 - 0001- extended_stats_funcs.c
```
} StakindFlags;
```

Just feel “k” should be captain: StaKindFlags.

5 - 0001 - extended_stats_funcs.c
```
+ nspoid = get_namespace_oid(nspname, true);
+ if (nspoid == InvalidOid)
+ {
+ ereport(WARNING,
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("could not find schema \"%s\"", stxname));
+ PG_RETURN_BOOL(false);
+ }
```

Should “stxname” be “nspname”?

6 - 0001 - extended_stats_funcs.c

extended_statistics_update is defined to return a bool, but there are multiple branches returning Datum: "PG_RETURN_BOOL(false)"; "return (Datum) 0”.

7 - 0001 - extended_stats_funcs.c
```
+static Datum
+warn_type_mismatch(Datum d, const char *argname)
+{
+ char *s = TextDatumGetCString(d);
+
+ ereport(WARNING,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not match expression %s element \"%s\" with input type",
+ argname, s));
+ return (Datum) 0;
+}
```

Why this function needs to return a Datum?

8 - 0001 - extended_stats_funcs.c
```
+static void
+expand_stxkind(HeapTuple tup, StakindFlags *enabled)
```

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.

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

9 - 0001 - extended_stats_funcs.c
```
+ if (!ok)
+ {
+ char *s = TextDatumGetCString(exprs_elems[correlation_idx]);
+
+ ereport(WARNING,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("could not match expression %s of element \"%s\" with input type",
+ extexprarginfo[CORRELATION_ELEM].argname, s));
+ return (Datum) 0;
+ }
```

Do we need to pfree(s) before return?

10 - 0001 - extended_stats_funcs.c
```
+ pgstup = heap_form_tuple(RelationGetDescr(pgsd), values, nulls);
+ pgstdat = heap_copy_tuple_as_datum(pgstup, RelationGetDescr(pgsd));
+ astate = accumArrayResult(astate, pgstdat, false, pgstypoid,
+ CurrentMemoryContext);
```

I think we should free pgstup because this is a loop.

11 - 0001
```
+<programlisting>
+ SELECT pg_restore_extended_stats(
+ 'schemaname', 'tab_schema'::name,
+ 'relname', 'tab_name'::name,
+ 'statistics_schemaname', 'stats_schema'::name,
+ 'statistics_name', 'stats_name'::name,
+ 'inherited', false,
+ 'n_distinct', '{"2, 3": 4, "2, -1": 4, "2, -2": 4, "3, -1": 4, "3, -2": 4}'::pg_ndistinct,
+ 'dependencies', '{"2 => 1": 1.000000, "2 => -1": 1.000000, "2 => -2": 1.000000}'::pg_dependencies
+ 'exprs', '{{0,4,-0.75,"{1}","{0.5}","{-1,0}",-0.6,NULL,NULL,NULL},{0.25,4,-0.5,"{2}","{0.5}",NULL,1,NULL,NULL,NULL}}'::text[]);
+</programlisting>
```

Missing a tailing comma in the “dependencies” line.

12 - 0001
```
+ Additionally, this function accepts argument name
+ <literal>version</literal> of type <type>integer</type>, which
+ specifies the server version from which the statistics originated.
```

I don’t see where “version” is implemented.

13 - 0002
```
+ else
+ appendPQExpBufferStr(pq,
+ "FROM ( "
+ "SELECT s.stxndistinct AS n_distinct, "
+ " s.stxdependencies AS dependencies "
+ "FROM pg_catalog.pg_statistics_ext AS s "
+ "JOIN pg_catalog.pg_namespace AS n "
+ "ON n.oid = s.stxnamespace "
+ "WHERE n.nspname = $1 "
+ "AND e.stxname = $2 "
+ ") AS e ");
```

Looks like e.stxname should be s.stxname.

Also, pg_catalog.pg_statistics_ext should be pg_catalog.pg_statistic_ext.

14 - 0002
```
+ /*
+ * Set up query for constraint-specific details.
+ *
+ * 19+: query pg_stats_ext and pg_stats_ext_exprs as-is 15-18: query
+ * pg_stats_ext translating the ndistinct and dependencies, 14:
+ * inherited is always NULL 12-13: no pg_stats_ext_exprs 10-11: no
+ * pg_stats_ext, join pg_statistic_ext and pg_namespace
+ */
+
+ appendPQExpBufferStr(pq,
+ "PREPARE getExtStatsStats(pg_catalog.name, pg_catalog.name) AS\n"
+ "SELECT “);
```

"constraint-specific details” looks like a copy-paste error.

15 - 0002
```
+ * The ndistinnct and depdendencies formats changed in v19, so
```

Typo: ndistinnct -> ndistinct, depdendencies -> dependencies

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