MIME-Version: 1.0
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>
 <aPcetUfI2NJDqYxZ@paquier.xyz>
In-Reply-To: <aPcetUfI2NJDqYxZ@paquier.xyz>
From: Corey Huinker <corey.huinker@gmail.com>
Date: Wed, 22 Oct 2025 14:55:31 +0300
Message-ID: 
 <CADkLM=efY2h8Bio=bE8O2Tnd=WUdyKDL1Zrtz9h+at16E4HVUA@mail.gmail.com>
Subject: Re: Extended Statistics set/restore/clear functions.
To: Michael Paquier <michael@paquier.xyz>
Cc: jian he <jian.universality@gmail.com>, Tomas Vondra <tomas@vondra.me>,
	pgsql-hackers@lists.postgresql.org, tgl@sss.pgh.pa.us
Content-Type: multipart/alternative; boundary="0000000000000f9b210641bdff09"
Archived-At: 
 <https://www.postgresql.org/message-id/CADkLM%3DefY2h8Bio%3DbE8O2Tnd%3DWUdyKDL1Zrtz9h%2Bat16E4HVUA%40mail.gmail.com>
Precedence: bulk

--0000000000000f9b210641bdff09
Content-Type: text/plain; charset="UTF-8"

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

That prefix would probably be statatt_ or statattr_.


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

Sorry, I don't understand. There shouldn't be any increase inside the
catalogs as the internal storage of the datatypes hasn't changed, so I can
only conclude that you're referring to something else.


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

The equivalent structures in attribute_stats.c will need documenting too.


>
> 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 = $1 AND e.statistics_name = $2 ORDER BY e.inherite...
>

Noted.


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

+1


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

Can do.


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

I also like simple.

Right now we have a situation where the vast majority of databases can
carry forward all of their stats via pg_upgrade, except for those databases
that have extended stats. The trouble is, most customers don't know if
their database uses extended statistics or not, and those that do are in
for some bad query plans if they haven't run vacuumdb --missing-stats-only.
Explaining that to customers is complicated, especially when most of them
do not know what extended stats are, let alone whether they have them. It
would be a lot simpler to just say "all stats are carried over on upgrade",
and vacuumdb becomes unnecessary, making upgrades one step simpler as well.

Given that, I think that the admittedly ugly transformation is worth it,
and sequestering it inside pg_dump is the smallest footprint it can have.
Earlier in this thread I posted some functions that did the translation
from the existing formats to the proposed new formats. We could include
those as new system functions, and that would make the dump code very
simple. Having said that, I don't know that there would be use for those
functions except inside pg_dump, hence the decision to do the transforms
right in the dump query.

If the format translation is a barrier to fetching existing extended stats,
then I'd be more inclined to keep the existing pg_ndistinct and
pg_dependencies data formats as they are now.

--0000000000000f9b210641bdff09
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div class=3D"gmail_quote gmail_quote_container"><blockquo=
te class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px =
solid rgb(204,204,204);padding-left:1ex">The functions exposed in 0003 shou=
ld be renamed to match more with the<br>
style of the rest, aka it is a bit hard to figure out what they do at<br>
first sight.=C2=A0 Presumably, these should be prefixed with some<br>
&quot;statext_&quot;, except text_to_stavalues() which could still be named=
 the<br>
same.<br></blockquote><div><br></div>That prefix would probably be statatt_=
 or statattr_.=C2=A0<div>=C2=A0</div><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin=
g-left:1ex">
<br>
Do you have some numbers regarding the increase in size this generates<br>
for the catalogs?<br></blockquote><div><br></div>Sorry, I don&#39;t underst=
and. There shouldn&#39;t be any increase inside the catalogs as the interna=
l storage of the datatypes=C2=A0hasn&#39;t changed, so I can only conclude =
that you&#39;re referring to something else.<div>=C2=A0</div><blockquote cl=
ass=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid=
 rgb(204,204,204);padding-left:1ex">
<br>
0004 has been designed following the same model as the relation and<br>
attribute stats.=C2=A0 That sounds OK here.<br>
<br>
+enum extended_stats_argnum<br>
[...]<br>
+enum extended_stats_exprs_element<br>
<br>
It would be nice to document why such things are around.=C2=A0 That would<b=
r>
be less guessing for somebody reading the code.<br></blockquote><div><br></=
div>The equivalent structures in attribute_stats.c will need documenting to=
o.</div><div class=3D"gmail_quote gmail_quote_container"><div><br></div><di=
v><br></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"mar=
gin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1=
ex">
<br>
Reusing this small sequence from your pg_dump patch, executed on a v14<br>
backend:<br>
create schema dump_test;<br>
CREATE TABLE dump_test.has_ext_stats<br>
=C2=A0 AS SELECT g.g AS x, g.g / 2 AS y FROM generate_series(1,100) AS g(g)=
;<br>
CREATE STATISTICS dump_test.es1 ON x, (y % 2) FROM dump_test.has_ext_stats;=
<br>
ANALYZE dump_test.has_ext_stats;<br>
<br>
Then pg_dump fails:<br>
pg_dump: error: query failed: ERROR:=C2=A0 column e.inherited does not exis=
t<br>
LINE 2: ...hemaname =3D $1 AND e.statistics_name =3D $2 ORDER BY e.inherite=
...<br></blockquote><div><br></div><div>Noted.</div><div>=C2=A0</div><block=
quote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1=
px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * TODO: Until v18 is released the master branc=
h has a<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * server_version_num of 180000. We will update=
 this to 190000<br>
as soon<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * as the master branch updates.<br>
<br>
This part has not been updated.<br>
<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0Assert(item.nattributes &gt; 0);=C2=A0 =C2=A0/*=
 TODO: elog? */<br>
[...]<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0Assert(dependency-&gt;nattributes &gt; 1);=C2=
=A0 =C2=A0 /* TODO: elog? */<br>
Yes and yes.=C2=A0 It seems like it should be possible to craft some input<=
br>
that triggers these..<br></blockquote><div><br></div><div>+1</div><div>=C2=
=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8e=
x;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
+void<br>
+free_pg_dependencies(MVDependencies *dependencies);<br>
<br>
Double declaration of this routine in dependencies.c.<br>
<br>
Perhaps some of the regression tests could use some jsonb_pretty() in<br>
the outputs generated.=C2=A0 Some of the results generated are very hard to=
<br>
parse, something that would become harder in the buildfarm.=C2=A0 This<br>
comment starts with 0001 for stxdndistinct.<br></blockquote><div><br></div>=
Can do.</div><div class=3D"gmail_quote gmail_quote_container"><div>=C2=A0</=
div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bor=
der-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I have mixed feelings about 0005, FWIW.=C2=A0 I am wondering if we should<b=
r>
not lift the needle a bit here and only support the dump of extended<br>
statistics when dealing with a backend of at least v19.=C2=A0 This would<br=
>
mean that we would only get the full benefit of this feature once<br>
people upgrade to v20 or dump from a pg_dump with --statistics from at<br>
least v19, but with the long-term picture in mind this would also make<br>
the dump/restore picture of the patch dead simple (spoiler: I like<br>
simple). <br></blockquote><div><br></div><div>I also like simple.</div><div=
><br></div><div>Right now we have a situation where the vast majority of da=
tabases can carry forward all of their stats via pg_upgrade, except for tho=
se databases that have extended stats. The trouble is, most customers don&#=
39;t know=C2=A0if their database uses extended statistics or not, and those=
 that do are in for some bad query plans if they haven&#39;t run vacuumdb -=
-missing-stats-only. Explaining that to customers is complicated, especiall=
y when most of them do not know what extended stats are, let alone whether =
they have them. It would be a lot simpler to just say &quot;all stats are c=
arried over on upgrade&quot;, and vacuumdb becomes unnecessary, making upgr=
ades one step simpler as well.</div><div><br></div><div>Given that, I think=
 that the admittedly ugly transformation is worth it, and sequestering it i=
nside pg_dump is the smallest footprint it can have. Earlier in this thread=
 I posted some functions that did the translation from the existing formats=
 to the proposed new formats. We could include those as new system function=
s, and that would make the dump code very simple. Having said that, I don&#=
39;t know that there would be use for those functions except inside pg_dump=
, hence the decision to do the transforms right in the dump query.</div><br=
></div><div class=3D"gmail_quote gmail_quote_container">If the format trans=
lation is a barrier to fetching existing extended stats, then I&#39;d be mo=
re inclined to keep the existing pg_ndistinct and pg_dependencies data form=
ats as they are now.</div></div>

--0000000000000f9b210641bdff09--