public inbox for [email protected]
help / color / mirror / Atom feedFrom: jian he <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Corey Huinker <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: Extended Statistics set/restore/clear functions.
Date: Mon, 17 Nov 2025 20:53:35 +0800
Message-ID: <CACJufxHc4Y4SUTH3iWFj+jXOm7NjZBczVdAzQ8BFOyiBvmKD=g@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CADkLM=eGwDZX9DQ_GouzqjMG7x_kCedBS6br2SciJ3wjwivfZg@mail.gmail.com>
<[email protected]>
<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>
<[email protected]>
<[email protected]>
On Mon, Nov 17, 2025 at 2:56 PM Michael Paquier <[email protected]> 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.
>
> And I have looked at 0003 in details for now. Attached is a revised
> version for it, with many adjustments. Some notes:
> - Many portions of the coverage were missing. I have measured the
> coverage at 91% with the updated version attached. This includes
> coverage for some error reporting, something that we rely a lot on for
> this code.
> - The error reports are made simpler, with the token values getting
> hidden. While testing with some fancy values, I have actually noticed
> that the error handlings for the parsing of the int16 and int32 values
> were incorrect, the error reports used what the safe functions
> generated, not the reports from the data type.
> - Passing down arbitrary bytes sequences was leading to these bytes
> reported in the error outputs because we cared about the token values.
> I have added a few tests based on that for the code paths involved.
>
> 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.
>
> 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?
>
hi.
NDistinctSemanticState, last element
NDIST_EXPECT_COMPLETE should follow with a comma, like:
``NDIST_EXPECT_COMPLETE, ``
Lots of tests use pg_input_error_info, which is good.
since currently only pg_input_error_info, pg_input_is_valid
NDistinctParseState.escontext is not NULL.
+ /*
+ * If escontext already set, just use that. Anything else is a generic
+ * JSON parse error.
+ */
+ if (!SOFT_ERROR_OCCURRED(parse_state.escontext))
+ errsave(parse_state.escontext,
+ errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("malformed pg_ndistinct: \"%s\"", str),
+ errdetail("Must be valid JSON."));
seems no coverage for this.
segfault:
SELECT '[{"attributes" : [1,2,3,4,5,67,6,7,8], "ndistinct" : 4}]'::pg_ndistinct;
because src/backend/statistics/mvdistinct.c line: 310, Assert
Assert((item->nattributes >= 2) && (item->nattributes <= STATS_MAX_DIMENSIONS));
+SELECT '[{"attributes" : [2], "ndistinct" : 4}]'::pg_ndistinct;
+ERROR: malformed pg_ndistinct: "[{"attributes" : [2], "ndistinct" : 4}]"
+LINE 1: SELECT '[{"attributes" : [2], "ndistinct" : 4}]'::pg_ndistin...
+ ^
+DETAIL: The "ndistinct" key must contain an array of at least two attributes.
here, it should be
+DETAIL: The "attributes" key must contain an array of at least two attributes.
?
--
jian
https://www.enterprisedb.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]
Subject: Re: Extended Statistics set/restore/clear functions.
In-Reply-To: <CACJufxHc4Y4SUTH3iWFj+jXOm7NjZBczVdAzQ8BFOyiBvmKD=g@mail.gmail.com>
* 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