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: jian he <jian.universality@gmail.com>
Date: Tue, 18 Nov 2025 13:07:23 +0800
Message-ID: 
 <CACJufxGGzkb58BU+YyTa9cBAawhybwk2cPFZ1XupS-8xuAzN9A@mail.gmail.com>
Subject: Re: Extended Statistics set/restore/clear functions.
To: Michael Paquier <michael@paquier.xyz>
Cc: Corey Huinker <corey.huinker@gmail.com>, Tomas Vondra <tomas@vondra.me>,
	pgsql-hackers@lists.postgresql.org, tgl@sss.pgh.pa.us
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CACJufxGGzkb58BU%2BYyTa9cBAawhybwk2cPFZ1XupS-8xuAzN9A%40mail.gmail.com>
Precedence: bulk

On Mon, Nov 17, 2025 at 2:56=E2=80=AFPM 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.
>

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

in src/backend/statistics/mvdistinct.c, we have:
Assert(AttributeNumberIsValid(item->attributes[j]));

should we disallow 0 in key attributes?
SELECT '[{"attributes" : [0,1], "ndistinct" : 4}]'::pg_ndistinct;
I didn't find a way to trigger this Assert yet.


+ errsave(parse->escontext,
+ errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("malformed pg_ndistinct: \"%s\"", parse->str),
+ errdetail("Invalid \"%s\" value.", PG_NDISTINCT_KEY_ATTRIBUTES));

+ errsave(parse->escontext,
+ errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("malformed pg_ndistinct: \"%s\"", parse->str),
+ errdetail("Invalid \"%s\" value.",
+  PG_NDISTINCT_KEY_NDISTINCT));

the errdetail is way too generic?
similar to ``select 'a'::int;``
we can
DETAIL:  Invalid input syntax for type integer: "a"
HINT: "ndistinct" value expected to be a type of integer.

what do you think?


we already have "fname" in ndistinct_object_field_start,
we can also print out the "fname", like:
    errsave(parse->escontext,
            errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
            errmsg("malformed pg_ndistinct: \"%s\"", parse->str),
            errdetail("Unexpected key \"%s\"", fname),
            errhint("Only allowed keys are \"%s\" and \"%s\".",
                      PG_NDISTINCT_KEY_ATTRIBUTES,
                      PG_NDISTINCT_KEY_NDISTINCT));


SELECT '[{"attributes" : [2,3], "ndistinct" : 4, "ndistinct" :
14}]'::pg_ndistinct;
               pg_ndistinct
-------------------------------------------
 [{"attributes": [2, 3], "ndistinct": 14}]

SELECT '[{"attributes" : [2,3], "ndistinct" : 4, "attributes" :
[]}]'::pg_ndistinct;
               pg_ndistinct
------------------------------------------
 [{"attributes": [2, 3], "ndistinct": 4}]

Is the above output what we expected?


+ /*
+ * We need at least two attribute numbers for a ndistinct item, anything
+ * less is malformed.
+ */
+ natts =3D parse->attnum_list->length;
here, we can use list_length.

+ if (parse->attnum_list !=3D NIL)
+ if (parse->distinct_items !=3D NIL)
here, we can also use list_length.


--
jian
https://www.enterprisedb.com/