MIME-Version: 1.0
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>
 <CADkLM=ei_07zDAyTrabZRc1tbRwzWLBd5yaP1zzqBTXxpvS-SA@mail.gmail.com>
 <aRutno2k5ilGCMIC@paquier.xyz>
In-Reply-To: <aRutno2k5ilGCMIC@paquier.xyz>
From: Corey Huinker <corey.huinker@gmail.com>
Date: Mon, 17 Nov 2025 21:32:37 -0500
Message-ID: 
 <CADkLM=d7fu9k03CD60eHciF3bUbd1-ANSD8VjVxsjMyAL1HVGQ@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="000000000000ac17d00643d547d4"
Archived-At: 
 <https://www.postgresql.org/message-id/CADkLM%3Dd7fu9k03CD60eHciF3bUbd1-ANSD8VjVxsjMyAL1HVGQ%40mail.gmail.com>
Precedence: bulk

--000000000000ac17d00643d547d4
Content-Type: text/plain; charset="UTF-8"

>
>
> This feels like a different pretty still compressed output for json.
> I don't think we should change the output functions to do that, but if
> you want to add a function that filters these contents a bit in the
> tests for the input functions, sure, why not.
>

+1, I'll probably just use the replaces rather than define a function and
give someone the false impression that the function exists elsewhere.


>
> Yes, this one is to reduce the translation work, and because the
> messages are quite the same across the board and deal with the same
> requirements:
> - Single integer expected after a key (attnum or actual value).
> - Array of attribute expected after a key.
> - For the degree key, float value.
>
> > I had a feeling that was going to be requested. My question would be if
> > that we want to stick to modeling the other combinations after the first
> > longest combination, last longest, or if we want to defer those checks
> > altogether until we have to validate against an actual stats object?
>
> I would tend to think that performing one round of validation once the
> whole set of objects has been parsed is going to be cheaper than
> periodic checks.
>
> One other thing would be to force a sort of the elements in the array
> to match with the order these are generated when creating the stats.
> We cannot do that in the input functions because we have no idea about
> the order of the attributes in the statistics object yet.  Applying a
> sort sounds also important to me to make sure that we order the stats
> based on what the group generation functions (aka
> generate_combinations(), etc.) think on the matter, which would
> enforce a stronger binary compatibility after we are sure that we have
> a full set of attributes listed in an array with the input function of
> course.  I have briefly looked at the planner code where extended
> stats are used, like selfuncs.c, and the ordering does not completely
> matter, it seems, but it's cheap enough to enforce a stricter ordering
> based on the K groups of N elements generated in the import function.
>
> >> Except for this argument, the input of pg_ndistinct feels OK in terms
> >> of the guarantees that we'd want to enforce on an import.  The same
> >> argument applies in terms of attribute number guarantees for
> >> pg_dependencies, based on DependencyGenerator_init() & friends in
> >> dependencies.c.  Could you look at that?
> >
> > Yes. I had already looked at it to verify that _all_ combinations were
> > always generated (they are), because I had some vague memory of the
> > generator dropping combinations that were statistically insignificant. In
> > retrospect, I have no idea where I got that idea.
>
> Hmm.  I would need to double-check the code to be sure, but I don't
> think that we drop combinations, because the code prevents duplicates
> to begin with, even for expressions:
> create table aa (a int, b int);
> create statistics stats (ndistinct) ON a, a, b, b from aa;
> ERROR:  42701: duplicate column name in statistics definition
> create statistics stats (ndistinct) ON (a + a), ((a + a)) from aa;
> ERROR:  42701: duplicate expression in statistics definition
>

So I looked at the generator functions, hoping they'd have enough in common
that they could be made generic. And they're just different enough that I
think it's not worth it to try.

But, if we don't care about the order of the combinations, I also don't
think we need to expose the functions at all. We know exactly how many
combinations there should be for any N attributes as each attribute must be
unique. So if we have the right number of unique combinations, and they're
all subsets of the first-longest, then we must have a complete set.
Thoughts on that?

Getting _too_ tight with the ordering and contents makes me concerned for
the day when the format might change. We don't want to _fail_ an upgrade
because some of the combinations were in the wrong order.


> These don't make sense anyway because they have a predictible and
> perfectly matching correlation relationship.
>

They do, for now, but are we willing to lock ourselves into that forever?


>
> > This is fairly simple to do. The dependency attnum is just appended to
> the
> > list of attnums, and the combinations are generated the same as
> ndistinct,
> > though obviously there are no single elements.
>
> Yeah.  That should be not be bad, I hope.
>
> > There's probably some common code between the lists to be shared,
> differing
> > only in how they report missing combinations.
>
> I would like to agree on that, but it did not look that obvious to me
> yesterday.  If you think that something could be refactored, I'd
> suggest a refactoring patch that applies on top of the rest of the
> patch set, with new generic facilities in stat_util.c, or even a
> new separate file, if that leads to a cleaner result (okay, a
> definition of "clean" is up to one's taste).
>

Looking over those functions, they both could have use the same generator,
but the dependencies-side decided that dependency order doesn't matter,
which puts doubt in my head that the order is perfectly the same for both,
so we'd better follow each individually IF we want to enforce order.

--000000000000ac17d00643d547d4
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>
This feels like a different pretty still compressed output for json.<br>
I don&#39;t think we should change the output functions to do that, but if<=
br>
you want to add a function that filters these contents a bit in the<br>
tests for the input functions, sure, why not.<br></blockquote><div><br></di=
v><div>+1, I&#39;ll probably just use the replaces rather than define a fun=
ction and give someone the false impression that the function exists elsewh=
ere.</div><div></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" sty=
le=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddi=
ng-left:1ex">
<br>
Yes, this one is to reduce the translation work, and because the<br>
messages are quite the same across the board and deal with the same<br>
requirements:<br>
- Single integer expected after a key (attnum or actual value).<br>
- Array of attribute expected after a key.<br>
- For the degree key, float value.<br>
<br>
&gt; I had a feeling that was going to be requested. My question would be i=
f<br>
&gt; that we want to stick to modeling the other combinations after the fir=
st<br>
&gt; longest combination, last longest, or if we want to defer those checks=
<br>
&gt; altogether until we have to validate against an actual stats object?<b=
r>
<br>
I would tend to think that performing one round of validation once the<br>
whole set of objects has been parsed is going to be cheaper than<br>
periodic checks.<br>
<br>
One other thing would be to force a sort of the elements in the array<br>
to match with the order these are generated when creating the stats.<br>
We cannot do that in the input functions because we have no idea about<br>
the order of the attributes in the statistics object yet.=C2=A0 Applying a<=
br>
sort sounds also important to me to make sure that we order the stats<br>
based on what the group generation functions (aka<br>
generate_combinations(), etc.) think on the matter, which would<br>
enforce a stronger binary compatibility after we are sure that we have<br>
a full set of attributes listed in an array with the input function of<br>
course.=C2=A0 I have briefly looked at the planner code where extended<br>
stats are used, like selfuncs.c, and the ordering does not completely<br>
matter, it seems, but it&#39;s cheap enough to enforce a stricter ordering<=
br>
based on the K groups of N elements generated in the import function.<br>
<br>
&gt;&gt; Except for this argument, the input of pg_ndistinct feels OK in te=
rms<br>
&gt;&gt; of the guarantees that we&#39;d want to enforce on an import.=C2=
=A0 The same<br>
&gt;&gt; argument applies in terms of attribute number guarantees for<br>
&gt;&gt; pg_dependencies, based on DependencyGenerator_init() &amp; friends=
 in<br>
&gt;&gt; dependencies.c.=C2=A0 Could you look at that?<br>
&gt; <br>
&gt; Yes. I had already looked at it to verify that _all_ combinations were=
<br>
&gt; always generated (they are), because I had some vague memory of the<br=
>
&gt; generator dropping combinations that were statistically insignificant.=
 In<br>
&gt; retrospect, I have no idea where I got that idea.<br>
<br>
Hmm.=C2=A0 I would need to double-check the code to be sure, but I don&#39;=
t<br>
think that we drop combinations, because the code prevents duplicates<br>
to begin with, even for expressions:<br>
create table aa (a int, b int);<br>
create statistics stats (ndistinct) ON a, a, b, b from aa;<br>
ERROR:=C2=A0 42701: duplicate column name in statistics definition<br>
create statistics stats (ndistinct) ON (a + a), ((a + a)) from aa;<br>
ERROR:=C2=A0 42701: duplicate expression in statistics definition<br></bloc=
kquote><div><br></div><div><div><div>So I looked at the generator functions=
, hoping they&#39;d have enough in common
 that they could be made generic. And they&#39;re just different enough tha=
t
 I think it&#39;s not worth it to try.</div><div><br></div>But, if we don&#=
39;t care about the order of the combinations, I also=20
don&#39;t think we need to expose the functions at all. We know exactly how=
=20
many combinations there should be for any N attributes as each attribute
 must be unique. So if we have the right number of unique combinations,=20
and they&#39;re all subsets of the first-longest, then we must have a=20
complete set. Thoughts on that?</div><div><br></div><div>Getting _too_ tigh=
t with the ordering and contents makes me concerned for the day when the fo=
rmat might change. We don&#39;t want to _fail_ an upgrade because some of t=
he combinations were in the wrong order.</div><br></div><div>=C2=A0</div><b=
lockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-le=
ft:1px solid rgb(204,204,204);padding-left:1ex">
These don&#39;t make sense anyway because they have a predictible and<br>
perfectly matching correlation relationship.<br></blockquote><div><br></div=
><div>They do, for now, but are we willing to lock ourselves into that fore=
ver?</div><div></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" sty=
le=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddi=
ng-left:1ex">
<br>
&gt; This is fairly simple to do. The dependency attnum is just appended to=
 the<br>
&gt; list of attnums, and the combinations are generated the same as ndisti=
nct,<br>
&gt; though obviously there are no single elements.<br>
<br>
Yeah.=C2=A0 That should be not be bad, I hope.<br>
<br>
&gt; There&#39;s probably some common code between the lists to be shared, =
differing<br>
&gt; only in how they report missing combinations.<br>
<br>
I would like to agree on that, but it did not look that obvious to me<br>
yesterday.=C2=A0 If you think that something could be refactored, I&#39;d<b=
r>
suggest a refactoring patch that applies on top of the rest of the<br>
patch set, with new generic facilities in stat_util.c, or even a<br>
new separate file, if that leads to a cleaner result (okay, a<br>
definition of &quot;clean&quot; is up to one&#39;s taste).<br></blockquote>=
<div><br></div><div><div>Looking over those functions, they both could have=
 use the same generator, but the dependencies-side decided that dependency =
order doesn&#39;t matter, which puts doubt in my head that the order is per=
fectly the same for both, so we&#39;d better follow each individually IF we=
 want to enforce order.</div><div><br></div></div></div></div>

--000000000000ac17d00643d547d4--