From: Chao Li <li.evan.chao@gmail.com>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_D54E3923-7EE7-465E-A1DF-8039A0F0E44D"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Fix small issues of pg_restore_extended_stats()
Message-Id: <A7C11B83-7534-4A09-9071-FBD09175CFC8@gmail.com>
Date: Mon, 18 May 2026 10:25:16 +0800
Cc: Michael Paquier <michael.paquier@gmail.com>,
 Corey Huinker <corey.huinker@gmail.com>
To: PostgreSQL-development <pgsql-hackers@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/A7C11B83-7534-4A09-9071-FBD09175CFC8%40gmail.com>
Precedence: bulk


--Apple-Mail=_D54E3923-7EE7-465E-A1DF-8039A0F0E44D
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

Hi,

While testing pg_restore_extended_stats(), I noticed several small =
issues.

1. Inconsistent expression key verification behavior

In this example, =E2=80=9Cb=E2=80=9D is an invalid key, so it raises a =
warning and rejects the input:
```
evantest=3D# select pg_restore_extended_stats(
    'schemaname', 'repro=E2=80=99,
    'relname', 't=E2=80=99,
    'statistics_schemaname', 'repro=E2=80=99,
    'statistics_name', 's=E2=80=99,
    'inherited', false,=20
    'exprs', '[{"b": "1"}]'::jsonb);
WARNING:  could not import element in expression -1: invalid key name
 pg_restore_extended_stats
---------------------------
 f
(1 row)
```

However, when I tried =E2=80=9Ca=E2=80=9D, which is also an invalid key, =
it is silently ignored:
```
evantest=3D# select pg_restore_extended_stats(
    'schemaname', 'repro=E2=80=99,
    'relname', 't=E2=80=99,
    'statistics_schemaname', 'repro=E2=80=99,
    'statistics_name', 's=E2=80=99,
    'inherited', false,
    'exprs', '[{"a": "1"}]'::jsonb);
 pg_restore_extended_stats
---------------------------
 t
(1 row)
```

After debugging, I think the problem is here:
```
/*
 * Check if key is found in the list of expression argnames.
 */
static bool
key_in_expr_argnames(JsonbValue *key)
{
	Assert(key->type =3D=3D jbvString);
	for (int i =3D 0; i < NUM_ATTRIBUTE_STATS_ELEMS; i++)
	{
		if (strncmp(extexprargname[i], key->val.string.val, =
key->val.string.len) =3D=3D 0)
			return true;
	}
	return false;
}
```

This function is actually doing a prefix comparison using the input key =
length, so as long as an input key matches a prefix of a valid key name, =
the function returns true.

At runtime, it does not lead to an incorrect value being imported, =
because the invalid key will still be filtered out later. But one bad =
scenario I can imagine is that a user is trying to set =
=E2=80=9Ccorrelation=E2=80=9D, and accidentally omits the last =E2=80=9Cn=E2=
=80=9D. In that case, the key is silently discarded and the user is not =
aware of it, which can lead to a surprising result. For example:
```
evantest=3D# select pg_restore_extended_stats(
    'schemaname', 'repro=E2=80=99,
    'relname', 't=E2=80=99,
    'statistics_schemaname', 'repro=E2=80=99,
    'statistics_name', 's=E2=80=99,
    'inherited', false,
    'exprs', '[{"n_distinct": "5", "correlatio": "-0.6"}]'::jsonb);
 pg_restore_extended_stats
---------------------------
 t
(1 row)

evantest=3D# select n_distinct, correlation from pg_stats_ext_exprs =
where statistics_schemaname =3D 'repro' and statistics_name =3D 's';
 n_distinct | correlation
------------+-------------
          5 |
(1 row)
```

Here, the user may think correlation has been set, but it has not, and =
there is no warning. I think this behavior should be fixed.

The fix is straightforward. If we keep using strncmp() for safety, we =
should also compare the lengths of both strings.

2. Wrong number in a warning message

I have only one expression in this example:
```
evantest=3D# select pg_restore_extended_stats(
evantest(#   'schemaname', 'repro',
evantest(#   'relname', 't',
evantest(#   'statistics_schemaname', 'repro',
evantest(#   'statistics_name', 's',
evantest(#   'inherited', false,
evantest(#   'exprs', '[{"n_distinct": "1"}, {"n_distinct": "2"}, =
{"n_distinct": "3"}]'::jsonb
evantest(# );
WARNING:  could not parse "exprs": incorrect number of elements (3 =
required)
 pg_restore_extended_stats
---------------------------
 f
(1 row)
```

The warning message says =E2=80=9C3 required=E2=80=9D, but it should be =
=E2=80=9C1 required=E2=80=9D.

The root cause is here:
```
	num_root_elements =3D JsonContainerSize(root);
	if (numexprs !=3D num_root_elements)
	{
		ereport(WARNING,
				=
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
				errmsg("could not parse \"%s\": =
incorrect number of elements (%d required)",
					   argname, num_root_elements));
		goto exprs_error;
	}
```

In the ereport, we should use numexprs instead.

3. Inconsistent heap_freetuple()

In pg_clear_extended_stats(), heap_freetuple(tup) is only called before =
the final return. However, in two earlier paths, the function only emits =
warning messages and returns. It seems those paths should also call =
heap_freetuple(tup).

But from my previous experience, I know this kind of issue is usually =
not considered as serious, so I only point it out here without making a =
fix for now.

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


--Apple-Mail=_D54E3923-7EE7-465E-A1DF-8039A0F0E44D
Content-Disposition: attachment;
	filename=v1-0001-Fix-prefix-matching-of-expression-stats-keys.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="v1-0001-Fix-prefix-matching-of-expression-stats-keys.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=204573034d05961da16d9f4deb334bea39d620b5e9=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20"Chao=20Li=20(Evan)"=20<lic@highgo.com>=0A=
Date:=20Mon,=2018=20May=202026=2009:17:55=20+0800=0ASubject:=20[PATCH=20=
v1=201/2]=20Fix=20prefix=20matching=20of=20expression=20stats=20keys=0A=0A=
When=20validating=20keys=20in=20imported=20expression=20statistics,=20=
the=20code=20used=0Astrncmp()=20with=20the=20input=20key=20length.=20=
This=20allowed=20an=20invalid=20key=20that=0Awas=20only=20a=20prefix=20=
of=20a=20valid=20key=20name=20to=20be=20accepted.=0A=0AFix=20this=20by=20=
also=20checking=20that=20the=20key=20lengths=20match=20before=20=
comparing=0Athe=20key=20contents.=20Add=20a=20regression=20test=20to=20=
cover=20the=20prefix-key=20case.=0A=0AAuthor:=20Chao=20Li=20=
<lic@highgo.com>=0A---=0A=20=
src/backend/statistics/extended_stats_funcs.c=20|=20=203=20++-=0A=20=
src/test/regress/expected/stats_import.out=20=20=20=20|=2014=20=
++++++++++++++=0A=20src/test/regress/sql/stats_import.sql=20=20=20=20=20=20=
=20=20=20|=20=209=20+++++++++=0A=203=20files=20changed,=2025=20=
insertions(+),=201=20deletion(-)=0A=0Adiff=20--git=20=
a/src/backend/statistics/extended_stats_funcs.c=20=
b/src/backend/statistics/extended_stats_funcs.c=0Aindex=20=
70393d3a904..15f131724e9=20100644=0A---=20=
a/src/backend/statistics/extended_stats_funcs.c=0A+++=20=
b/src/backend/statistics/extended_stats_funcs.c=0A@@=20-886,7=20+886,8=20=
@@=20key_in_expr_argnames(JsonbValue=20*key)=0A=20=09Assert(key->type=20=
=3D=3D=20jbvString);=0A=20=09for=20(int=20i=20=3D=200;=20i=20<=20=
NUM_ATTRIBUTE_STATS_ELEMS;=20i++)=0A=20=09{=0A-=09=09if=20=
(strncmp(extexprargname[i],=20key->val.string.val,=20=
key->val.string.len)=20=3D=3D=200)=0A+=09=09if=20=
(strlen(extexprargname[i])=20=3D=3D=20key->val.string.len=20&&=0A+=09=09=09=
strncmp(extexprargname[i],=20key->val.string.val,=20key->val.string.len)=20=
=3D=3D=200)=0A=20=09=09=09return=20true;=0A=20=09}=0A=20=09return=20=
false;=0Adiff=20--git=20a/src/test/regress/expected/stats_import.out=20=
b/src/test/regress/expected/stats_import.out=0Aindex=20=
f421e83e232..d73bc96039f=20100644=0A---=20=
a/src/test/regress/expected/stats_import.out=0A+++=20=
b/src/test/regress/expected/stats_import.out=0A@@=20-3256,6=20+3256,20=20=
@@=20most_common_elems=20=20=20=20=20=20|=20{-1,0,1,2,3}=0A=20=
most_common_elem_freqs=20|=20{0.25,0.25,0.5,0.25,0.25,0.25,0.5,0.25}=0A=20=
elem_count_histogram=20=20=20|=20=
{1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1=
,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2=
,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,1.5}=0A=20=0A+--=20=
bad:=20exprs=20param=20which=20is=20a=20prefix=20of=20a=20valid=20key=20=
name=0A+SELECT=20pg_catalog.pg_restore_extended_stats(=0A+=20=20=
'schemaname',=20'stats_import',=0A+=20=20'relname',=20'test',=0A+=20=20=
'statistics_schemaname',=20'stats_import',=0A+=20=20'statistics_name',=20=
'test_stat_mcelem',=0A+=20=20'inherited',=20false,=0A+=20=20'exprs',=20=
'[{=20"n":=20"-1"=20}]'::jsonb);=0A+WARNING:=20=20could=20not=20import=20=
element=20in=20expression=20-1:=20invalid=20key=20name=0A+=20=
pg_restore_extended_stats=20=0A+---------------------------=0A+=20f=0A=
+(1=20row)=0A+=0A=20--=20ok:=20tsvector=20exceptions,=20test=20just=20=
the=20collation=20exceptions=0A=20CREATE=20STATISTICS=20=
stats_import.test_stat_tsvec=20ON=20(length(name)),=20=
(to_tsvector(name))=20FROM=20stats_import.test;=0A=20SELECT=20=
pg_catalog.pg_restore_extended_stats(=0Adiff=20--git=20=
a/src/test/regress/sql/stats_import.sql=20=
b/src/test/regress/sql/stats_import.sql=0Aindex=20=
c1bf55690a6..b74f6395a5e=20100644=0A---=20=
a/src/test/regress/sql/stats_import.sql=0A+++=20=
b/src/test/regress/sql/stats_import.sql=0A@@=20-2244,6=20+2244,15=20@@=20=
WHERE=20e.statistics_schemaname=20=3D=20'stats_import'=20AND=0A=20=20=20=20=
=20e.inherited=20=3D=20false=0A=20\gx=0A=20=0A+--=20bad:=20exprs=20param=20=
which=20is=20a=20prefix=20of=20a=20valid=20key=20name=0A+SELECT=20=
pg_catalog.pg_restore_extended_stats(=0A+=20=20'schemaname',=20=
'stats_import',=0A+=20=20'relname',=20'test',=0A+=20=20=
'statistics_schemaname',=20'stats_import',=0A+=20=20'statistics_name',=20=
'test_stat_mcelem',=0A+=20=20'inherited',=20false,=0A+=20=20'exprs',=20=
'[{=20"n":=20"-1"=20}]'::jsonb);=0A+=0A=20--=20ok:=20tsvector=20=
exceptions,=20test=20just=20the=20collation=20exceptions=0A=20CREATE=20=
STATISTICS=20stats_import.test_stat_tsvec=20ON=20(length(name)),=20=
(to_tsvector(name))=20FROM=20stats_import.test;=0A=20SELECT=20=
pg_catalog.pg_restore_extended_stats(=0A--=20=0A2.50.1=20(Apple=20=
Git-155)=0A=0A=

--Apple-Mail=_D54E3923-7EE7-465E-A1DF-8039A0F0E44D
Content-Disposition: attachment;
	filename=v1-0002-Fix-required-expression-count-in-stats-import-war.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="v1-0002-Fix-required-expression-count-in-stats-import-war.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=20961bb9aa5139b303a88b195a3994e0cf7b7d5e63=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20"Chao=20Li=20(Evan)"=20<lic@highgo.com>=0A=
Date:=20Mon,=2018=20May=202026=2010:03:24=20+0800=0ASubject:=20[PATCH=20=
v1=202/2]=20Fix=20required=20expression=20count=20in=20stats=20import=20=
warning=0A=0AWhen=20importing=20expression=20statistics,=20the=20warning=20=
for=20an=20incorrect=0Anumber=20of=20expression=20elements=20reported=20=
the=20number=20of=20elements=20found=20in=0Athe=20input=20as=20the=20=
required=20count.=20This=20made=20the=20message=20misleading,=0A=
especially=20when=20the=20input=20contained=20too=20few=20elements.=0A=0A=
Report=20the=20expected=20number=20of=20expressions=20instead.=20Also=20=
extend=20the=0Aregression=20test=20to=20cover=20both=20too-few=20and=20=
too-many=20expression=20elements.=0A=0AAuthor:=20Chao=20Li=20=
<lic@highgo.com>=0A---=0A=20=
src/backend/statistics/extended_stats_funcs.c=20|=20=202=20+-=0A=20=
src/test/regress/expected/stats_import.out=20=20=20=20|=2018=20=
++++++++++++++++--=0A=20src/test/regress/sql/stats_import.sql=20=20=20=20=
=20=20=20=20=20|=2010=20+++++++++-=0A=203=20files=20changed,=2026=20=
insertions(+),=204=20deletions(-)=0A=0Adiff=20--git=20=
a/src/backend/statistics/extended_stats_funcs.c=20=
b/src/backend/statistics/extended_stats_funcs.c=0Aindex=20=
15f131724e9..8fc9c4c6f45=20100644=0A---=20=
a/src/backend/statistics/extended_stats_funcs.c=0A+++=20=
b/src/backend/statistics/extended_stats_funcs.c=0A@@=20-1593,7=20+1593,7=20=
@@=20import_expressions(Relation=20pgsd,=20int=20numexprs,=0A=20=09=09=
ereport(WARNING,=0A=20=09=09=09=09=
errcode(ERRCODE_INVALID_PARAMETER_VALUE),=0A=20=09=09=09=09errmsg("could=20=
not=20parse=20\"%s\":=20incorrect=20number=20of=20elements=20(%d=20=
required)",=0A-=09=09=09=09=09=20=20=20argname,=20num_root_elements));=0A=
+=09=09=09=09=09=20=20=20argname,=20numexprs));=0A=20=09=09goto=20=
exprs_error;=0A=20=09}=0A=20=0Adiff=20--git=20=
a/src/test/regress/expected/stats_import.out=20=
b/src/test/regress/expected/stats_import.out=0Aindex=20=
d73bc96039f..183e68d65c2=20100644=0A---=20=
a/src/test/regress/expected/stats_import.out=0A+++=20=
b/src/test/regress/expected/stats_import.out=0A@@=20-2455,7=20+2455,7=20=
@@=20WARNING:=20=20could=20not=20parse=20"exprs":=20root-level=20array=20=
required=0A=20=20f=0A=20(1=20row)=0A=20=0A---=20wrong=20number=20of=20=
exprs=0A+--=20wrong=20number=20of=20exprs,=20too=20few=0A=20SELECT=20=
pg_catalog.pg_restore_extended_stats(=0A=20=20=20'schemaname',=20=
'stats_import',=0A=20=20=20'relname',=20'test_clone',=0A@@=20-2463,7=20=
+2463,21=20@@=20SELECT=20pg_catalog.pg_restore_extended_stats(=0A=20=20=20=
'statistics_name',=20'test_stat_clone',=0A=20=20=20'inherited',=20false,=0A=
=20=20=20'exprs',=20'[=20{=20"avg_width":=20"4"=20}=20]'::jsonb);=0A=
-WARNING:=20=20could=20not=20parse=20"exprs":=20incorrect=20number=20of=20=
elements=20(1=20required)=0A+WARNING:=20=20could=20not=20parse=20=
"exprs":=20incorrect=20number=20of=20elements=20(2=20required)=0A+=20=
pg_restore_extended_stats=20=0A+---------------------------=0A+=20f=0A=
+(1=20row)=0A+=0A+--=20wrong=20number=20of=20exprs,=20too=20many=0A=
+SELECT=20pg_catalog.pg_restore_extended_stats(=0A+=20=20'schemaname',=20=
'stats_import',=0A+=20=20'relname',=20'test_clone',=0A+=20=20=
'statistics_schemaname',=20'stats_import',=0A+=20=20'statistics_name',=20=
'test_stat_clone',=0A+=20=20'inherited',=20false,=0A+=20=20'exprs',=20'[=20=
{=20"avg_width":=20"4"=20},=20{=20"avg_width":=20"4"=20},=20{=20=
"avg_width":=20"4"=20}=20]'::jsonb);=0A+WARNING:=20=20could=20not=20=
parse=20"exprs":=20incorrect=20number=20of=20elements=20(2=20required)=0A=
=20=20pg_restore_extended_stats=20=0A=20---------------------------=0A=20=
=20f=0Adiff=20--git=20a/src/test/regress/sql/stats_import.sql=20=
b/src/test/regress/sql/stats_import.sql=0Aindex=20=
b74f6395a5e..6064b7722da=20100644=0A---=20=
a/src/test/regress/sql/stats_import.sql=0A+++=20=
b/src/test/regress/sql/stats_import.sql=0A@@=20-1750,7=20+1750,7=20@@=20=
SELECT=20pg_catalog.pg_restore_extended_stats(=0A=20=20=20=
'statistics_name',=20'test_stat_clone',=0A=20=20=20'inherited',=20false,=0A=
=20=20=20'exprs',=20'{=20"avg_width":=20"4",=20"null_frac":=20"0"=20=
}'::jsonb);=0A---=20wrong=20number=20of=20exprs=0A+--=20wrong=20number=20=
of=20exprs,=20too=20few=0A=20SELECT=20=
pg_catalog.pg_restore_extended_stats(=0A=20=20=20'schemaname',=20=
'stats_import',=0A=20=20=20'relname',=20'test_clone',=0A@@=20-1758,6=20=
+1758,14=20@@=20SELECT=20pg_catalog.pg_restore_extended_stats(=0A=20=20=20=
'statistics_name',=20'test_stat_clone',=0A=20=20=20'inherited',=20false,=0A=
=20=20=20'exprs',=20'[=20{=20"avg_width":=20"4"=20}=20]'::jsonb);=0A+--=20=
wrong=20number=20of=20exprs,=20too=20many=0A+SELECT=20=
pg_catalog.pg_restore_extended_stats(=0A+=20=20'schemaname',=20=
'stats_import',=0A+=20=20'relname',=20'test_clone',=0A+=20=20=
'statistics_schemaname',=20'stats_import',=0A+=20=20'statistics_name',=20=
'test_stat_clone',=0A+=20=20'inherited',=20false,=0A+=20=20'exprs',=20'[=20=
{=20"avg_width":=20"4"=20},=20{=20"avg_width":=20"4"=20},=20{=20=
"avg_width":=20"4"=20}=20]'::jsonb);=0A=20--=20incorrect=20type=20of=20=
value:=20should=20be=20a=20string=20or=20a=20NULL.=0A=20SELECT=20=
pg_catalog.pg_restore_extended_stats(=0A=20=20=20'schemaname',=20=
'stats_import',=0A--=20=0A2.50.1=20(Apple=20Git-155)=0A=0A=

--Apple-Mail=_D54E3923-7EE7-465E-A1DF-8039A0F0E44D--