Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Extended Statistics set/restore/clear functions.
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CADkLM=eRop0t==hg0TU3KuDH6DgnUxQ6vZxV9th=6vW1JG36MQ@mail.gmail.com>
Date: Fri, 21 Nov 2025 11:36:23 +0800
Cc: jian he <jian.universality@gmail.com>,
 Tomas Vondra <tomas@vondra.me>,
 pgsql-hackers@lists.postgresql.org,
 tgl@sss.pgh.pa.us
Content-Transfer-Encoding: quoted-printable
Message-Id: <2C8A20C7-4E08-41F4-B30E-AD9F8B007AE5@gmail.com>
References: 
 <CACJufxHZzUeu_fyem2oA_FSrjk0ky5vn_h1Dw68=UVLekXPjXA@mail.gmail.com>
 <CACJufxHP+wZ9Th2U9JW27cCq-me222e76pv7vcjqt+oCL03m5Q@mail.gmail.com>
 <CADkLM=c8-U4GLMw5VdeDdfp1ae6BW=PCfEQqAky04iZMbckCFw@mail.gmail.com>
 <CACJufxFo5b0FvmV4CmOb74NFrXm_znZHh7bLiiVkAzVzTnJiVw@mail.gmail.com>
 <CADkLM=fky861BrifcuWc0CbWfo3yMbRB+m1jG4qqXG_3hnqdtA@mail.gmail.com>
 <CACJufxFni1oB-VFuLbVs9VSgVx0czvoZyoW0z_tJ2kq4E3SXYw@mail.gmail.com>
 <CADkLM=d_mmTn3+ChL-z_ZomwOKT-BssQU+=6_a8WDv6A4b7uAA@mail.gmail.com>
 <aRbLV7Tru3ulO5ru@paquier.xyz> <aRrHBj7I4CeV21dj@paquier.xyz>
 <CACJufxGGzkb58BU+YyTa9cBAawhybwk2cPFZ1XupS-8xuAzN9A@mail.gmail.com>
 <aRwGeN7o22HJclg5@paquier.xyz>
 <CADkLM=czQ0UUyBn3AiHZfJJWRwXH5e2xn056yG0mpuXpW09RSw@mail.gmail.com>
 <CACJufxHVKiVEvCsc11oxOM=0GgjqChFwi-ii4RJuw95ZFB_fHQ@mail.gmail.com>
 <CADkLM=eRop0t==hg0TU3KuDH6DgnUxQ6vZxV9th=6vW1JG36MQ@mail.gmail.com>
To: Corey Huinker <corey.huinker@gmail.com>,
 Michael Paquier <michael@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/2C8A20C7-4E08-41F4-B30E-AD9F8B007AE5%40gmail.com>
Precedence: bulk

After 16 rounds of revisions, v16 looks solid and robust. Only got a few =
small comments:

> On Nov 21, 2025, at 07:08, Corey Huinker <corey.huinker@gmail.com> =
wrote:
>=20
> =
<v16-0001-Add-working-input-function-for-pg_ndistinct.patch><v16-0002-Add-=
working-input-function-for-pg_dependencies.patch><v16-0003-Expose-attribut=
e-statistics-functions-for-use-in.patch><v16-0004-Add-extended-statistics-=
support-functions.patch><v16-0005-Include-Extended-Statistics-in-pg_dump.p=
atch>

1 - 0001
```
+	/*
+	 * We need at least two attribute numbers for a ndistinct item, =
anything
+	 * less is malformed.
+	 */
+	natts =3D list_length(parse->attnum_list);
+	if ((natts < 2) || (natts > STATS_MAX_DIMENSIONS))
+	{
+		errsave(parse->escontext,
+				=
errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+				errmsg("malformed pg_ndistinct: \"%s\"", =
parse->str),
+				errdetail("The \"%s\" key must contain =
an array of at least %d "
+						  "and no more than %d =
attributes.",
+						  =
PG_NDISTINCT_KEY_ATTRIBUTES, 2, STATS_MAX_DIMENSIONS));
+		return JSON_SEM_ACTION_FAILED;
+	}
```

Do we also need to reset item state vars similar to the success case?

2 - 0001
```
*
+ * Arrays can never be empty.
+ */
+static JsonParseErrorType
+ndistinct_array_end(void *state)
+{
```

This function comment is too simple, and doesn=E2=80=99t match to the =
function name. Reading the function body, the logic is clear, so I would =
suggest either just remove the comment or enhance it.

3  - 0001
```
static JsonParseErrorType
+ndistinct_array_element_start(void *state, bool isnull)
+{
+	NDistinctParseState *parse =3D state;
```

Nit: NDistinctParseState * can be const.

4 - 0001
```
+static bool
+valid_subsequent_attnum(const AttrNumber prev, const AttrNumber cur)
```

AttrNumber is int16, thus =E2=80=9Cconst=E2=80=9D here is unnecessary.

5 - 0001
```
+			return JSON_SUCCESS;
+			break;
```

=E2=80=9CBreak=E2=80=9D after =E2=80=9Creturn=E2=80=9D is unnecessary. I =
tried to delete the =E2=80=9Cbreak=E2=80=9D and didn=E2=80=99t see a =
compile warning, it should be safe to delete the =E2=80=9Cbreak=E2=80=9D.

6 - 0001
```
+/*
+ * Compare the attribute arrays of two MVNDistinctItem values,
+ * looking for duplicate sets.

+ */
+static bool
+has_duplicate_attributes(const MVNDistinctItem *a,
+						 const MVNDistinctItem =
*b)
```

The function comment says =E2=80=9Clooking for duplicate sets=E2=80=9D, =
seems to imply the function would return the set, but the function =
returns a bool, which is a little confusing.

7 - 0002
```
+	for (int i =3D 0; i < natts; i++)
+	{
+		dep->attributes[i] =3D (AttrNumber) =
list_nth_int(parse->attnum_list, i);
+		if (dep->attributes[i] =3D=3D parse->dependency)
+		{
+			errsave(parse->escontext,
+					=
errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+					errmsg("malformed =
pg_dependencies: \"%s\"", parse->str),
+					errdetail("Item \"%s\" value %d =
found in the \"%s\" list.",
+							  =
PG_DEPENDENCIES_KEY_DEPENDENCY, parse->dependency,
+			   				  =
PG_DEPENDENCIES_KEY_ATTRIBUTES));
+			return JSON_SEM_ACTION_FAILED;
+		}
```

Similar to comment 1, do we need to reset state vars upon failure?

8 - 0002
```
+				errdetail("The \"%s\" key must contain =
an array of at least %d "
+						  " and no than %d =
elements.",
```

I believe =E2=80=9Cmore=E2=80=9D is missed between =E2=80=9Cno than=E2=80=9D=
.

9 - 0002
```
+static JsonParseErrorType
+dependencies_array_start(void *state)
+{
+	DependenciesParseState *parse =3D state;
```

Similar to commit 3, DependenciesParseState * can be const.

10 - 0002
```
static bool
valid_subsequent_attnum(const AttrNumber prev, const AttrNumber cur)
```

This function is a dup to the one in 0001, can we put it to a common =
place?

11 - 0003
```
+extern void statatt_get_type(Oid reloid, AttrNumber attnum,
+							 Oid *atttypid, =
int32 *atttypmod,
+							 char =
*atttyptype, Oid *atttypcoll,
+							 Oid *eq_opr, =
Oid *lt_opr);
+extern void statatt_init_empty_tuple(Oid reloid, int16 attnum, bool =
inherited,
+									 =
Datum *values, bool *nulls, bool *replaces);
+
+extern void statatt_set_slot(Datum *values, bool *nulls, bool =
*replaces,
+							 int16 stakind, =
Oid staop, Oid stacoll,
+							 Datum =
stanumbers, bool stanumbers_isnull,
+							 Datum =
stavalues, bool stavalues_isnull);
+
+extern Datum text_to_stavalues(const char *staname, FmgrInfo *array_in, =
Datum d,
+							   Oid typid, =
int32 typmod, bool *ok);
+extern bool statatt_get_elem_type(Oid atttypid, char atttyptype,
+								  Oid =
*elemtypid, Oid *elem_eq_opr);
```

Several functions are made external visible, they are all renamed with =
adding a prefix =E2=80=9Cstatatt_=E2=80=9D, why text_to_stavalues is an =
exception?

11 - 0004
```
+bool
+pg_dependencies_validate_deps(MVDependencies *dependencies, int2vector =
*stxkeys, int numexprs, int elevel)
+{
+	int		attnum_expr_lowbound =3D 0 - numexprs;
+
+	for (int i =3D 0; i < dependencies->ndeps; i++)
+	{
+		MVDependency *dep =3D dependencies->deps[i];
```

This MVDependency * can be const.

12 - 0004
```
static void
upsert_pg_statistic_ext_data(Datum *values, bool *nulls, bool *replaces)
{
```

This function pass values, nulls and replaces to heap_modify_tuple() and =
heap_form_tuple(), the both functions take all const pointers as =
parameters. So, here values, nulls and replaces can all be const.

13 - 0004
```
+		if (!exprs_nulls[offset + AVG_WIDTH_ELEM])
+		{
+			ok =3D text_to_int4(exprs_elems[offset + =
AVG_WIDTH_ELEM],
+							  =
&values[Anum_pg_statistic_stawidth - 1]);
+
+			if (!ok)
+			{
+				char	   *s =3D =
TextDatumGetCString(exprs_elems[offset + NULL_FRAC_ELEM]);
+
+				ereport(WARNING,
+						=
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("Expression %s =
element \"%s\" does not match expected input type.",
+								=
extexprarginfo[AVG_WIDTH_ELEM].argname, s)));
+				pfree(s);
+				return (Datum) 0;
+			}
+		}
```

Here: char    *s =3D TextDatumGetCString(exprs_elems[offset + =
NULL_FRAC_ELEM]);
Should NULL_FRAC_ELEM be AVG_WIDTH_ELEM?

14 - 0004
```
+		if (!exprs_nulls[offset + N_DISTINCT_ELEM])
+		{
+			ok =3D text_to_float4(exprs_elems[offset + =
N_DISTINCT_ELEM],
+								=
&values[Anum_pg_statistic_stadistinct - 1]);
+
+			if (!ok)
+			{
+				char	   *s =3D =
TextDatumGetCString(exprs_elems[offset + NULL_FRAC_ELEM]);
+
+				ereport(WARNING,
+						=
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("Expression %s =
element \"%s\" does not match expected input type.",
+								=
extexprarginfo[N_DISTINCT_ELEM].argname, s)));
+				pfree(s);
+				return (Datum) 0;
+			}
+		}
```

Similar to comment 13, should NULL_FRAC_ELEM be N_DISTINCT_ELEM?

15 - 0004
```
+extern Datum import_mcvlist(HeapTuple tup, int elevel, int numattrs,
+							Oid *atttypids, =
int32 *atttypmods, Oid *atttypcolls,
+							int nitems, =
Datum *mcv_elems, bool *mcv_nulls,
+							bool =
*mcv_elem_nulls, float8 *freqs, float8 *base_freqs);
+
+extern Datum import_mcvlist(HeapTuple tup, int elevel, int numattrs,
+							Oid *atttypids, =
int32 *atttypmods, Oid *atttypcolls,
+							int nitems, =
Datum *mcv_elems, bool *mcv_nulls,
+							bool =
*mcv_elem_nulls, float8 *freqs, float8 *base_freqs);
```

import_mcvlist is declared twice, looks like a copy-paste mistake.

16 - 0005
```
+	PREPQUERY_DUMPEXTSTATSSTATS,
```

=E2=80=9Cstatsstats=E2=80=9D looks weird, I=E2=80=99d suggest =
PREPQUERY_DUMPEXTSTATSDATA.

Best regards,
=E2=80=94
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/