public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Corey Huinker <[email protected]>
Cc: jian he <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: Extended Statistics set/restore/clear functions.
Date: Tue, 25 Nov 2025 16:59:28 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CADkLM=d_mmTn3+ChL-z_ZomwOKT-BssQU+=6_a8WDv6A4b7uAA@mail.gmail.com>
<[email protected]>
<[email protected]>
<CACJufxGGzkb58BU+YyTa9cBAawhybwk2cPFZ1XupS-8xuAzN9A@mail.gmail.com>
<[email protected]>
<CADkLM=czQ0UUyBn3AiHZfJJWRwXH5e2xn056yG0mpuXpW09RSw@mail.gmail.com>
<CACJufxHVKiVEvCsc11oxOM=0GgjqChFwi-ii4RJuw95ZFB_fHQ@mail.gmail.com>
<CADkLM=eRop0t==hg0TU3KuDH6DgnUxQ6vZxV9th=6vW1JG36MQ@mail.gmail.com>
<CACJufxGgEnOzFq0HjmkKBTnEUgToGFC0g0A4wPWoJMzOuoWUhw@mail.gmail.com>
<CADkLM=crdyK3w7-EDEdDfXHGidxm2ms+bQYNFOZUD4eOLuLXEQ@mail.gmail.com>
<[email protected]>
> On Nov 25, 2025, at 15:28, Michael Paquier <[email protected]> wrote:
>
> On Sat, Nov 22, 2025 at 03:26:19AM -0500, Corey Huinker wrote:
>> I added a comment debating the feasibility of testing for subsets of
>> attribute sets in pg_dependencies. Basically, I think we can't have the
>> test at all, but I haven't removed it just yet pending consensus.
>
> + * Verify that all attnum sets are a proper subset of the first longest
> + * attnum set.
> + *
> + * TODO:
> + *
> + * I'm fairly certain that because statisticsally insignificant dependency
> + * combinations are not stored, there is a chance that the longest dependency
> + * does not exist, and therefore this test cannot be done. I have left the
> + * test in place for the time being until the issue can be definitively
> + * settled.
>
> As you have already quoted upthread, statext_dependencies_build()
> settles the issue on this one, I think. It is entirely possible that
> any group returned by DependencyGenerator generates a degree value
> that would prevent a given group to be stored, and this could as well
> be the largest possible group there could be in the set. So we cannot
> do any of that for dependencies, unfortunately. We can always rely on
> the list of attributes when assigning the json blob to the stats
> object, at least, cross-checking that each attribute list matches with
> the numbers of the stats object. At least we can check for
> duplicates, which is better than nothing at all.
>
> Regarding the suggested check where we'd want to enforce all the
> groups of attributes to be listed depending on the longest set we have
> found, at the end estimate_multivariate_ndistinct() checks the items
> listed one-by-one, giving up if we cannot find something in the list
> of items. I think that I am going to be content with the patch as it
> is, without this piece. Let's add an extra SQL test to treat that as
> valid input, though. So I am feeling OK with the input for ndistinct
> at this stage. I have noticed a couple of issues in passing,
> adjusting them. We are reaching more than 90% of coverage with the
> tests, and I am not sure that we can actually reach the rest except if
> one of the previous steps failed.
>
> So That's one. Now into the second patch for the input of the
> dependencies.
>
> +SELECT '[{"attributes" : [2], "dependency" : 4, "degree": "NaN"}]'::pg_dependencies;
> +SELECT '[{"attributes" : [2], "dependency" : 4, "degree": "-inf"}]'::pg_dependencies;
> +SELECT '[{"attributes" : [2], "dependency" : 4, "degree": "inf"}]'::pg_dependencies;
> +SELECT '[{"attributes" : [2], "dependency" : 4, "degree": "-inf"}]'::pg_dependencies::text::pg_dependencies;
>
> Okay, I have to admit that these ones are fun. I doubt that anybody
> would actually do that, and these do not produce valid json objects,
> which is what the last case shows. Hmm, it makes sense to keep these,
> and I'm still siding that we should not care too much about applying
> checks on the values and complicate the input function more than that,
> so fine by me.
>
> There were a couple of things in the tests, missing quite a few soft
> errors. Many typos, grammar mistakes in the whole. Also, please do
> not split the error strings into multiple lines to make these
> greppable. There is also no need for a break after a return. In some
> cases, a return was used where a break made more sense as the default
> path returned a failure..
>
> The TODO in build_mvdependencies() could be an elog(), but I have left
> it untouched for the errdetail().
>
> We're reaching 91% of coverage here, not bad. The rest does not seem
> reachable, as far as I can see.
>
> With that said, a v18 for the first two patches with the input
> functions. Comments and/or opinions?
> --
> Michael
> <v18-0001-Add-working-input-function-for-pg_ndistinct.patch><v18-0002-Add-working-input-function-for-pg_dependencies.patch>
I don’t see any of my comments are addressed in v18.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
view thread (91+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Extended Statistics set/restore/clear functions.
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox