Re: Extended Statistics set/restore/clear functions.

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Corey Huinker <corey(dot)huinker(at)gmail(dot)com>, jian he <jian(dot)universality(at)gmail(dot)com>, Tomas Vondra <tomas(at)vondra(dot)me>, pgsql-hackers(at)lists(dot)postgresql(dot)org, tgl(at)sss(dot)pgh(dot)pa(dot)us
Subject: Re: Extended Statistics set/restore/clear functions.
Date: 2025-11-25 08:59:28
Message-ID: EFE9F959-01C6-45D3-9EEE-F06C766EF3CA@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Nov 25, 2025, at 15:28, Michael Paquier <michael(at)paquier(dot)xyz> 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/

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message VASUKI M 2025-11-25 09:11:32 Re: BUG #19095: Test if function exit() is used fail when linked static
Previous Message Chao Li 2025-11-25 08:50:29 Re: pg_waldump: support decoding of WAL inside tarfile