Re: general purpose array_sort

Hi Amit,

On Thu, Oct 3, 2024 at 2:22 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi Junwang,
>
> On Wed, Oct 2, 2024 at 11:46 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > On Wed, Oct 2, 2024 at 9:51 AM jian he <jian.universality@gmail.com> wrote:
> > >
> > > > >
> > > > > + typentry = (TypeCacheEntry *) fcinfo->flinfo->fn_extra;
> > > > > + if (typentry == NULL || typentry->type_id != elmtyp)
> > > > > + {
> > > > > + typentry = lookup_type_cache(elmtyp, sort_asc ? TYPECACHE_LT_OPR :
> > > > > TYPECACHE_GT_OPR);
> > > > > + fcinfo->flinfo->fn_extra = (void *) typentry;
> > > > > + }
> > > > > you need to one-time check typentry->lt_opr or typentry->gt_opr exists?
> > > > > see CreateStatistics.
> > > > >             /* Disallow data types without a less-than operator */
> > > > >             type = lookup_type_cache(attForm->atttypid, TYPECACHE_LT_OPR);
> > > > >             if (type->lt_opr == InvalidOid)
> > > > >                 ereport(ERROR,
> > > > >                         (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > > > >                          errmsg("column \"%s\" cannot be used in
> > > > > statistics because its type %s has no default btree operator class",
> > > > >                                 attname, format_type_be(attForm->atttypid))));
> > > >
> > > > I added an Assert for this part, not sure if that is enough.
> > > >
> > >
> > > i think it really should be:
> > >
> > > if (typentry == NULL || typentry->type_id != elmtyp)
> > > {
> > >  typentry = lookup_type_cache(elmtyp, sort_asc ? TYPECACHE_LT_OPR :
> > > TYPECACHE_GT_OPR);
> > >  fcinfo->flinfo->fn_extra = (void *) typentry;
> > > if ((sort_asc && !OidIsValid(typentry->lt_opr) || (!sort_as &&
> > > OidIsValid(typentry->gt_opr));
> > > ereport(ERROR,....)
> > > }
> > >
> > > Imagine a type that doesn't have TYPECACHE_LT_OPR or TYPECACHE_GT_OPR
> > > then we cannot do the sort, we should just error out.
> > >
> > > I just tried this colour type [1] with (CREATE TYPE colour (INPUT =
> > > colour_in, OUTPUT = colour_out, LIKE = pg_catalog.int4);
> > >
> > > select array_sort('{#FF0000, #FF0000}'::colour[]);
> > > of course it will segfault  with your new Assert.
> > >
> > >
> > > [1] https://github.com/hlinnaka/colour-datatype/blob/master/colour.c
> >
> > Make sense, PFA v5 with Jian's suggestion.
>
> Have you noticed that the tests have failed on Cirrus CI runs of this patch?
>
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5277

Sorry for the late reply due to my vacation. I should have paid
more attention to Cirrus CI earlier ;)

>
> It might be related to the test machines having a different *default*
> locale than your local environment, which could result in a different
> sort order for the test data. You may need to add an explicit COLLATE
> clause to the tests to ensure consistent sorting across systems.

I've changed the tests to use just ASCII characters, then added
*COLLATE "C"* to the tests and CI passed, PFA v6.

>
> --
> Thanks, Amit Langote



-- 
Regards
Junwang Zhao