Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 21 Oct 2025 14:48:37 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Corey Huinker <corey.huinker@gmail.com>
Cc: jian he <jian.universality@gmail.com>, Tomas Vondra <tomas@vondra.me>,
	pgsql-hackers@lists.postgresql.org, tgl@sss.pgh.pa.us
Subject: Re: Extended Statistics set/restore/clear functions.
Message-ID: <aPcetUfI2NJDqYxZ@paquier.xyz>
References: 
 <CACJufxF0hnpjdsjrx4zuktVMaFOqM=mx3xQXctn2262vmwCapg@mail.gmail.com>
 <CADkLM=e=ZhNbHQ-QfLRxpSN1d9kFQ3vn4yBZhw0uWr6usPA96A@mail.gmail.com>
 <CADkLM=ekXTzxG7_-Rd7qR0Gwrq8o3kG7oSkAJRTUNSqXi0KTnA@mail.gmail.com>
 <CADkLM=dnFKZMAo7MwqD2X6JjiiLCoVXHmszqtZp8sycYmoCcMQ@mail.gmail.com>
 <CADkLM=cuVMUNsv+C7dGUrc8zF2c4C_LXBcPCtb_zHR0z+fmb1A@mail.gmail.com>
 <CADkLM=eM0VA-muiu+JMM4-B8eL5ZscPeGtTxh7UtmM+iVO7C=A@mail.gmail.com>
 <CADkLM=daHJhUf-sO1iD=UTaymbBVEgEW4Sxm8PtHWWDEGWW9jQ@mail.gmail.com>
 <CADkLM=d_=STa4RHWHk7AGZ1RAZYKMtJxGMsw0Mrdb8s6CHdvbw@mail.gmail.com>
 <CADkLM=f2B8cDpx3V2bZW07GN5JHaP7kFe7G0cmJMSPVUQ4eHhQ@mail.gmail.com>
 <CADkLM=cWJTOnhC4jbKUvFgjMd=Hc252FXbampsjuYekBvmMVAg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="rZpy6TawEcvbnz53"
Content-Disposition: inline
In-Reply-To: 
 <CADkLM=cWJTOnhC4jbKUvFgjMd=Hc252FXbampsjuYekBvmMVAg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aPcetUfI2NJDqYxZ%40paquier.xyz>
Precedence: bulk


--rZpy6TawEcvbnz53
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Oct 18, 2025 at 08:27:58PM -0400, Corey Huinker wrote:
> And rebased again to conform to  688dc6299 and 4bd919129.

The format redesign for extended stats is pretty nice, as done in
0001.  I think that this patch should be split in two, actually, as it
tackles two issues:
- One patch for the format change.
- Second patch for the introduction of the input function, useful on
its own to allow more regression tests paths for the format generated.

Similar split comment for 0002 regarding pg_dependencies.  The format
change is one thing.  The input function is cool to have for input
validation, still not absolutely mandatory for the core part of the
format change.

The functions exposed in 0003 should be renamed to match more with the
style of the rest, aka it is a bit hard to figure out what they do at
first sight.  Presumably, these should be prefixed with some
"statext_", except text_to_stavalues() which could still be named the
same.

Do you have some numbers regarding the increase in size this generates
for the catalogs?

0004 has been designed following the same model as the relation and
attribute stats.  That sounds OK here.

+enum extended_stats_argnum
[...]
+enum extended_stats_exprs_element

It would be nice to document why such things are around.  That would
be less guessing for somebody reading the code.

Reusing this small sequence from your pg_dump patch, executed on a v14
backend:
create schema dump_test;
CREATE TABLE dump_test.has_ext_stats
  AS SELECT g.g AS x, g.g / 2 AS y FROM generate_series(1,100) AS g(g);
CREATE STATISTICS dump_test.es1 ON x, (y % 2) FROM dump_test.has_ext_stats;
ANALYZE dump_test.has_ext_stats;

Then pg_dump fails:
pg_dump: error: query failed: ERROR:  column e.inherited does not exist
LINE 2: ...hemaname =3D $1 AND e.statistics_name =3D $2 ORDER BY e.inherite=
=2E..


+        * TODO: Until v18 is released the master branch has a
+        * server_version_num of 180000. We will update this to 190000
as soon
+        * as the master branch updates.

This part has not been updated.

+       Assert(item.nattributes > 0);   /* TODO: elog? */
[...]
+       Assert(dependency->nattributes > 1);    /* TODO: elog? */
Yes and yes.  It seems like it should be possible to craft some input
that triggers these..

+void
+free_pg_dependencies(MVDependencies *dependencies);

Double declaration of this routine in dependencies.c.

Perhaps some of the regression tests could use some jsonb_pretty() in
the outputs generated.  Some of the results generated are very hard to
parse, something that would become harder in the buildfarm.  This
comment starts with 0001 for stxdndistinct.

I have mixed feelings about 0005, FWIW.  I am wondering if we should
not lift the needle a bit here and only support the dump of extended
statistics when dealing with a backend of at least v19.  This would
mean that we would only get the full benefit of this feature once
people upgrade to v20 or dump from a pg_dump with --statistics from at
least v19, but with the long-term picture in mind this would also make
the dump/restore picture of the patch dead simple (spoiler: I like
simple).=20

Tomas, what is your take about the format changes and my argument
about the backward requirements of pg_dump (about not dumping these
stats if connecting to a server older than v18, included)?
--
Michael

--rZpy6TawEcvbnz53
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmj3HrUACgkQnvQgOdby
QH2QwQ//eKZE2oy3lc8vmOwvwXwaJC27hoiZeClFleHSQpsbS7228GLyQN1ejnAC
P4YocG3+nA8WhQPVqxYHdwWbFv8y6isBkCM54CRZUh2kUoFfsVTKPtGeAOXgQEVB
Lkj+TmvCWC3PI/qGNY1jBvHxhKfLlKg5N0hUHtHF4H6sErsKEwC3ZGv4QnWJFnQC
ecMGGwMNtN8VQ/ltWxPmlwxgK1TYLXbBG0UteXKydVge/4AxDAsrQNN1CcLKpbP5
3EroLScLoKVa2Nf1993rGhITJQQuxVMVMfolLevgjOwhbSRW+DXRnAYAQsCuAUv1
93+raQNIJJvq3d5ix4QzEbI0HFjxIUkXweOWv7rFIN3zWd/vWfYQYiu8iyStXf3d
ArIJ/tB0y45KU0lHhLe+8ckHXCoDF1OMlJgMA4JZVPMLfrQjoxwpPbHk2jLaOn9o
1I9mpuyQwDFtCsb2HCrU0UmhWejAorm2IbBI0cO0HvRLbCao5YTyCdlBhTlvpP/p
Kg22TseBEsngJX80PmR8NsJT0JNPrJqSrTVAWnSKhAH/z3tSZ/cItcUHwo0M1FvU
3hRuuUNiWxM1/ZbFe+/tjgi2YOxfU8NLn4u3ySXjUNPgzWjMycoqoRBtb+8VS4Ca
+izDu2EQcr4f2DaGdLZGiHQoRbulGzUN4fnIyjoYW729pTF+qmw=
=ux33
-----END PGP SIGNATURE-----

--rZpy6TawEcvbnz53--