MIME-Version: 1.0
References: 
 <CADkLM=d_=STa4RHWHk7AGZ1RAZYKMtJxGMsw0Mrdb8s6CHdvbw@mail.gmail.com>
 <CADkLM=f2B8cDpx3V2bZW07GN5JHaP7kFe7G0cmJMSPVUQ4eHhQ@mail.gmail.com>
 <CADkLM=cWJTOnhC4jbKUvFgjMd=Hc252FXbampsjuYekBvmMVAg@mail.gmail.com>
 <aPcetUfI2NJDqYxZ@paquier.xyz>
 <CADkLM=efY2h8Bio=bE8O2Tnd=WUdyKDL1Zrtz9h+at16E4HVUA@mail.gmail.com>
 <aPls00-bu3fPcuk-@paquier.xyz>
 <439a9947-e6a1-4ea9-84d8-1f843deaf6b1@vondra.me>
 <CADkLM=cTVN3g7OzdSXfnBHEYNVQiGMJ8cdx1v=zJa86w1A9ULA@mail.gmail.com>
 <aQxb0rHuckPSBeLO@paquier.xyz>
 <CADkLM=dMSJ5pHQoRxj9qoU49s+f33kXbhvm_12g=ARaXgysg=g@mail.gmail.com>
 <aQ2k8--a0FfwSwX9@paquier.xyz>
In-Reply-To: <aQ2k8--a0FfwSwX9@paquier.xyz>
From: Corey Huinker <corey.huinker@gmail.com>
Date: Fri, 7 Nov 2025 17:28:50 -0500
Message-ID: 
 <CADkLM=dWQ3r48eAP8NggLqe90_16JKbit9iu9AtuUrZ8+A=qBA@mail.gmail.com>
Subject: Re: Extended Statistics set/restore/clear functions.
To: Michael Paquier <michael@paquier.xyz>
Cc: Tomas Vondra <tomas@vondra.me>, jian he <jian.universality@gmail.com>,
	pgsql-hackers@lists.postgresql.org, tgl@sss.pgh.pa.us
Content-Type: multipart/alternative; boundary="00000000000061e085064308b539"
Archived-At: 
 <https://www.postgresql.org/message-id/CADkLM%3DdWQ3r48eAP8NggLqe90_16JKbit9iu9AtuUrZ8%2BA%3DqBA%40mail.gmail.com>
Precedence: bulk

--00000000000061e085064308b539
Content-Type: text/plain; charset="UTF-8"

>
>
> Patch 0001 for ndistinct was missing a documentation update, we have
> one query in perform.sgml that looks at stxdndistinct.  Patch 0003 is
> looking OK here as well.
>

Well spotted.


> For dependencies, the format switches from a single json object
> with key/vals like that:
> "3 => 4": 1.000000
> To a JSON array made of elements like that:
> {"degree": 1.000000, "attributes": [3],"dependency": 4},
>
> For ndistincts, we move from a JSON blob with key/vals like that:
> "3, 4": 11
> To a JSON array made of the following elements:
> {"ndistinct": 11, "attributes": [3,4]}
>
> Using a keyword within each element would force a stronger validation
> when these get imported back, which is a good thing.  I like that.
>
> Before going in-depth into the input functions to cross-check the
> amount of validation we should do, have folks any comments about the
> proposed format?  That's the key point this patch set depends on, and
> I'd rather not spend more time the whole thing if somebody would like
> a different format.  This is the format that Tomas has mentioned at
> the top of the thread.  Note: as noted upthread, pg_dump would be in
> charge of transferring the data of the old format to the new format at
> the end.
>

I'm open to other formats, but aside from renaming the json keys (maybe
"attnums" or "keys" instead of "attributes"?), I'm not sure what really
could be done and still be JSON. I suppose we could go with a tuple format
like this:

'{({3,4},11),...}' for pg_ndistinct and
'{({3},4,1.00000),...}'  for pg_dependencies.

Those would certainly be more compact, but makes for a hard read by humans,
and while the JSON code is big, it's also proven in other parts of the
codebase, hence less risky.


>
> While looking at 0002 and 0004 (which have a couple of issues
> actually), I have been wondering about moving into a new file the four
> data-type functions (in, out, send and receive) and the new input
> functions that rely on a new JSON lexer and parser logic into for both
> ndistinct and dependencies.  The new set of headers added at the top
> of mvdistinct.c and dependencies.c for the new code points that a
> separation may be better in the long-term, because the new code relies
> on parts of the backend that the existing code does not care about,
> and these files become larger than the relation and attribute stats
> files.  I would be tempted to name these new files pg_dependencies.c
> and pg_ndistinct.c, mapping with their catalog types.  With this
> separation, it looks like the "core" parts in charge of the
> calculations with ndistinct and dependencies can be kept on its own.
> What do you think?
>

A part of me thinks that everything that remains after removing
in/out/send/recv is just taking a table sample data structure and crunching
numbers to come up with the deserialized data structure...that's in/out
with a different starting/ending points.

There's no denying that JSON parsing is a very different code style than
statistical number crunching, and mixing the two is incongruous, so it's
worth a shot, and I'll try that for v9.


>
> A second comment is for 0005.  The routines of attributes.c are
> applied to the new clear and restore functions.  Shouldn't these be in
> stats_utils.c at the end?  That's where the "common" functions used by
> the stats manipulation logic are.
>

I assume you're referring to attribute_stats.c. I think that would cause
stats_utils.c to have to pull in a lot of things from attribute_stats.c,
and that would create the exact sort of include-pollution that you're
trying to avoid in the mvdistinct.c/dependencies.c situation mentioned
above.

The one lone exception to this is text_to_stavalues(), which is a fancy
wrapper around array_in() and could perhaps be turned to even more generic
usage outside of stats in general.

The functions in question are needed because the exprs value is itself an
array of partly-filled-out pg_attribute tuples, so it's common to those two
needs, but specific to stats about attributes. Maybe we need an
attr_stats_utils.h?

--00000000000061e085064308b539
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"><br>
Patch 0001 for ndistinct was missing a documentation update, we have<br>
one query in perform.sgml that looks at stxdndistinct.=C2=A0 Patch 0003 is<=
br>
looking OK here as well.<br></blockquote><div><br></div><div>Well spotted.<=
/div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px=
 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

For dependencies, the format switches from a single json object<br>
with key/vals like that:<br>
&quot;3 =3D&gt; 4&quot;: 1.000000<br>
To a JSON array made of elements like that:<br>
{&quot;degree&quot;: 1.000000, &quot;attributes&quot;: [3],&quot;dependency=
&quot;: 4},<br>
<br>
For ndistincts, we move from a JSON blob with key/vals like that:<br>
&quot;3, 4&quot;: 11<br>
To a JSON array made of the following elements:<br>
{&quot;ndistinct&quot;: 11, &quot;attributes&quot;: [3,4]}<br>
<br>
Using a keyword within each element would force a stronger validation<br>
when these get imported back, which is a good thing.=C2=A0 I like that.<br>
<br>
Before going in-depth into the input functions to cross-check the<br>
amount of validation we should do, have folks any comments about the<br>
proposed format?=C2=A0 That&#39;s the key point this patch set depends on, =
and<br>
I&#39;d rather not spend more time the whole thing if somebody would like<b=
r>
a different format.=C2=A0 This is the format that Tomas has mentioned at<br=
>
the top of the thread.=C2=A0 Note: as noted upthread, pg_dump would be in<b=
r>
charge of transferring the data of the old format to the new format at<br>
the end.<br></blockquote><div><br></div><div>I&#39;m open to other formats,=
 but aside from renaming the json keys (maybe &quot;attnums&quot; or &quot;=
keys&quot; instead of &quot;attributes&quot;?), I&#39;m not sure what reall=
y could be done and still be JSON. I suppose we could go with a tuple forma=
t like this:<br><br></div><div>&#39;{({3,4},11),...}&#39; for pg_ndistinct =
and<br></div><div>&#39;{({3},4,1.00000),...}&#39;=C2=A0 for pg_dependencies=
.</div><div><br></div><div>Those would certainly be more compact, but makes=
 for a hard read by humans, and while the JSON code is big, it&#39;s also p=
roven in other parts of the codebase, hence less risky.</div><div></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>
While looking at 0002 and 0004 (which have a couple of issues<br>
actually), I have been wondering about moving into a new file the four<br>
data-type functions (in, out, send and receive) and the new input<br>
functions that rely on a new JSON lexer and parser logic into for both<br>
ndistinct and dependencies.=C2=A0 The new set of headers added at the top<b=
r>
of mvdistinct.c and dependencies.c for the new code points that a<br>
separation may be better in the long-term, because the new code relies<br>
on parts of the backend that the existing code does not care about,<br>
and these files become larger than the relation and attribute stats<br>
files.=C2=A0 I would be tempted to name these new files pg_dependencies.c<b=
r>
and pg_ndistinct.c, mapping with their catalog types.=C2=A0 With this<br>
separation, it looks like the &quot;core&quot; parts in charge of the<br>
calculations with ndistinct and dependencies can be kept on its own.<br>
What do you think?<br></blockquote><div><br></div><div>A part of me thinks =
that everything that remains after removing in/out/send/recv is just taking=
 a table sample data structure and crunching numbers to come up with the de=
serialized data structure...that&#39;s in/out with a different starting/end=
ing points.</div><div><br></div><div>There&#39;s no denying that JSON parsi=
ng is a very different code style than statistical number crunching, and mi=
xing the two is incongruous, so it&#39;s worth a shot, and I&#39;ll try tha=
t for v9.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"=
margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-lef=
t:1ex">
<br>
A second comment is for 0005.=C2=A0 The routines of attributes.c are<br>
applied to the new clear and restore functions.=C2=A0 Shouldn&#39;t these b=
e in<br>
stats_utils.c at the end?=C2=A0 That&#39;s where the &quot;common&quot; fun=
ctions used by<br>
the stats manipulation logic are.<br></blockquote><div><br></div><div></div=
><div>I assume you&#39;re referring to attribute_stats.c. I think that woul=
d cause stats_utils.c to have to pull in a lot of things from attribute_sta=
ts.c, and that would create the exact sort of include-pollution that you=
9;re trying to avoid in the mvdistinct.c/dependencies.c situation mentioned=
 above.</div><div><br></div><div>The one lone exception to this is text_to_=
stavalues(), which is a fancy wrapper around array_in() and could perhaps b=
e turned to even more generic usage outside of stats in general.</div><div>=
<br></div><div>The functions in question are needed because the exprs=C2=A0=
value is itself an array of partly-filled-out pg_attribute tuples, so it=
9;s common to those two needs, but specific to stats about attributes. Mayb=
e we need an attr_stats_utils.h?</div></div></div>

--00000000000061e085064308b539--