MIME-Version: 1.0
References: 
 <CADkLM=eGwDZX9DQ_GouzqjMG7x_kCedBS6br2SciJ3wjwivfZg@mail.gmail.com>
 <aRLumVDIQHUTKQYG@paquier.xyz>
 <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>
In-Reply-To: <aRrHBj7I4CeV21dj@paquier.xyz>
From: Corey Huinker <corey.huinker@gmail.com>
Date: Mon, 17 Nov 2025 12:18:55 -0500
Message-ID: 
 <CADkLM=ei_07zDAyTrabZRc1tbRwzWLBd5yaP1zzqBTXxpvS-SA@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="0000000000008a2c1e0643cd8bef"
Archived-At: 
 <https://www.postgresql.org/message-id/CADkLM%3Dei_07zDAyTrabZRc1tbRwzWLBd5yaP1zzqBTXxpvS-SA%40mail.gmail.com>
Precedence: bulk

--0000000000008a2c1e0643cd8bef
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Nov 17, 2025 at 1:56=E2=80=AFAM Michael Paquier <michael@paquier.xy=
z> wrote:

> On Fri, Nov 14, 2025 at 03:25:27PM +0900, Michael Paquier wrote:
> > Thanks for the new versions, I'll also look at all these across the
> > next couple of days.  Probably not at 0005~ for now.
>
> 0001 and 0002 from series v13 have been applied to change the output
> functions.
>

Thanks!

Though, I was thinking some more about the output format. Using
jsonb_pretty() makes it readable in one way, and very clumsy in other ways.
Instead, I'm going to try doing the following:

replace (ndist_string_value, '},', E'}\n,')

This will result in the output value being formatted in exactly the way
described in the commit messages.

Of course, we could make the the actual default format by changing
appendStringInfoString(&str, ", ") instead.

One might argue that the output shouldn't get too flowery, but we're
already adding spaces between items and array elements, and we've already
made extensive changes favoring readability over compactness.


Review of changes to input function patches:

I'm curious about the re-parameterization of error messages involving
PG_NDISTINCT_KEY_ATTRIBUTES, PG_NDISTINCT_KEY_NDISTINCT, and similar keys
in dependencies. I like the parameterized version better, and was confused
as to why it was removed. Did you change your mind, or was it done for ease
of translation?

Wording changes all look good. Use of pg_input_error_info() makes sense.


>
> There is an extra thing that bugs me as incorrect for the pg_ndistinct
> input, something I have not tackled myself yet.  Your patch checks
> that subsets of attributes are included in the longest set found, but
> it does not match the guarantees we have in mvndistinct.c: we have to
> check that *all* the combinations generated by generator_init() are
> satisfied based on the longest of attributes detected.  For example,
> this is thought as correct in the input function:
> SELECT '[{"attributes" : [-1,2], "ndistinct" : 1},
>          {"attributes" : [-1,2,3], "ndistinct" : 3}]'::pg_ndistinct;
>
> However it is obviously not correct as we are missing an element for
> the attributes [-1, 3].  The simplest solution would be to export the
> routines that generate the groups now in mvndistinct.c.  Also we
> should make sure that the number of elements in the arrays match with
> the number of groups we expect, not only the elements.  I don't think
> that we need to care much about the values, but we ought to provide
> stronger guarantees for the attributes listed in these elements.
>

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?


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


>
> For pg_dependencies, we also require some checks on the value for
> "dependency", of course, making sure that this matches with what's
> expected with the "largest" sets of attributes.  In this case, we need
> to track the union of "dependency" and "attributes", with "attributes"
> having at least one element.
>

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.

There's probably some common code between the lists to be shared, differing
only in how they report missing combinations.


> The tests of pg_dependencies need also to be extended more (begun that
> a bit, far from being complete and I'm lacking of time this week due
> to a conference).  One thing that I would add are nested JSON objects
> in the paths where we expect values, for example.  Please note that I
> have done a brush of 0004, while on it, cleaning up typos,
> inconsistencies and making the error codes consistent with the
> ndistinct case where possible.  This is not ready, but that's at least
> it's a start to rely on.
>


>
> In terms of committable bits, it would be better to apply the input
> functions once both parts are ready to go.  For now I am attached a
> v14 with the work I've put into them.  0005~ are not reviewed yet, as
> mentioned previously.  The changes in pg_dependencies are actually
> straight-forward to figure out (well, mostly) once the pg_ndistinct
> changes are OK in shape.
> --
> Michael


+1

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

<div dir=3D"ltr"><div class=3D"gmail_quote gmail_quote_container"><div dir=
=3D"ltr" class=3D"gmail_attr">On Mon, Nov 17, 2025 at 1:56=E2=80=AFAM Micha=
el Paquier &lt;<a href=3D"mailto:michael@paquier.xyz">michael@paquier.xyz</=
a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0p=
x 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On=
 Fri, Nov 14, 2025 at 03:25:27PM +0900, Michael Paquier wrote:<br>
&gt; Thanks for the new versions, I&#39;ll also look at all these across th=
e<br>
&gt; next couple of days.=C2=A0 Probably not at 0005~ for now.<br>
<br>
0001 and 0002 from series v13 have been applied to change the output<br>
functions.<br></blockquote><div><br></div><div>Thanks!<br><br></div><div>Th=
ough, I was thinking some more about the output format. Using jsonb_pretty(=
) makes it readable in one way, and very clumsy in other ways. Instead, I&#=
39;m going to try doing the following:</div><div><br></div><div>replace (nd=
ist_string_value, &#39;},&#39;, E&#39;}\n,&#39;)</div><div><br></div><div>T=
his will result in the output value being formatted in exactly the way desc=
ribed in the commit messages.</div><div><br></div><div>Of course, we could =
make the the actual default format by changing appendStringInfoString(&amp;=
str, &quot;, &quot;) instead.</div><div><div><br></div>One might argue that=
 the output shouldn&#39;t get too flowery, but we&#39;re already adding spa=
ces between items and array elements, and we&#39;ve already made extensive =
changes favoring readability over compactness.</div><div><br></div><div><br=
></div><div>Review of changes to input function patches:</div><div><br></di=
v><div>I&#39;m curious about the re-parameterization of error messages invo=
lving PG_NDISTINCT_KEY_ATTRIBUTES,=C2=A0PG_NDISTINCT_KEY_NDISTINCT, and sim=
ilar keys in dependencies. I like the parameterized version better, and was=
 confused as to why it was removed. Did you change your mind, or was it don=
e for ease of translation?</div><div><br></div><div>Wording changes all loo=
k good. Use of=C2=A0pg_input_error_info() makes sense.</div><div><br></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"><br>
There is an extra thing that bugs me as incorrect for the pg_ndistinct<br>
input, something I have not tackled myself yet.=C2=A0 Your patch checks<br>
that subsets of attributes are included in the longest set found, but<br>
it does not match the guarantees we have in mvndistinct.c: we have to<br>
check that *all* the combinations generated by generator_init() are<br>
satisfied based on the longest of attributes detected.=C2=A0 For example,<b=
r>
this is thought as correct in the input function:<br>
SELECT &#39;[{&quot;attributes&quot; : [-1,2], &quot;ndistinct&quot; : 1},<=
br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{&quot;attributes&quot; : [-1,2,3], &quot=
;ndistinct&quot; : 3}]&#39;::pg_ndistinct;<br>
<br>
However it is obviously not correct as we are missing an element for<br>
the attributes [-1, 3].=C2=A0 The simplest solution would be to export the<=
br>
routines that generate the groups now in mvndistinct.c.=C2=A0 Also we<br>
should make sure that the number of elements in the arrays match with<br>
the number of groups we expect, not only the elements.=C2=A0 I don&#39;t th=
ink<br>
that we need to care much about the values, but we ought to provide<br>
stronger guarantees for the attributes listed in these elements.<br></block=
quote><div><br></div><div>I had a feeling that was going to be requested. M=
y question would be if that we want to stick to modeling the other combinat=
ions=C2=A0after the first longest combination, last longest, or if we want =
to defer those checks altogether until we have to validate against an actua=
l stats object?</div><div></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">
<br>
Except for this argument, the input of pg_ndistinct feels OK in terms<br>
of the guarantees that we&#39;d want to enforce on an import.=C2=A0 The sam=
e<br>
argument applies in terms of attribute number guarantees for<br>
pg_dependencies, based on DependencyGenerator_init() &amp; friends in<br>
dependencies.c.=C2=A0 Could you look at that?<br></blockquote><div><br></di=
v><div>Yes. I had already looked at it to verify that _all_ combinations we=
re always generated (they are), because I had some vague memory of the gene=
rator dropping combinations that were statistically insignificant. In retro=
spect, I have no idea where I got that idea.</div><div></div><div><br></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">
<br>
For pg_dependencies, we also require some checks on the value for<br>
&quot;dependency&quot;, of course, making sure that this matches with what&=
#39;s<br>
expected with the &quot;largest&quot; sets of attributes.=C2=A0 In this cas=
e, we need<br>
to track the union of &quot;dependency&quot; and &quot;attributes&quot;, wi=
th &quot;attributes&quot;<br>
having at least one element.<br></blockquote><div><br></div><div>This is fa=
irly simple to do. The dependency attnum is just appended to the list of at=
tnums, and the combinations are generated the same as ndistinct, though obv=
iously there are no single elements.</div><div><br></div><div>There&#39;s p=
robably some common code between the lists to be shared, differing only in =
how they report missing combinations.</div><div></div><div><br></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">
The tests of pg_dependencies need also to be extended more (begun that<br>
a bit, far from being complete and I&#39;m lacking of time this week due<br=
>
to a conference).=C2=A0 One thing that I would add are nested JSON objects<=
br>
in the paths where we expect values, for example.=C2=A0 Please note that I<=
br>
have done a brush of 0004, while on it, cleaning up typos,<br>
inconsistencies and making the error codes consistent with the<br>
ndistinct case where possible.=C2=A0 This is not ready, but that&#39;s at l=
east<br>
it&#39;s a start to rely on.<br></blockquote><div><br></div><div>=C2=A0</di=
v><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;borde=
r-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
In terms of committable bits, it would be better to apply the input<br>
functions once both parts are ready to go.=C2=A0 For now I am attached a<br=
>
v14 with the work I&#39;ve put into them.=C2=A0 0005~ are not reviewed yet,=
 as<br>
mentioned previously.=C2=A0 The changes in pg_dependencies are actually<br>
straight-forward to figure out (well, mostly) once the pg_ndistinct<br>
changes are OK in shape.<br>
--<br>Michael</blockquote><div><br></div><div>+1=C2=A0</div></div></div>

--0000000000008a2c1e0643cd8bef--