Horton

Thread

Commits

  1. Introduce a SQL-callable function array_sort(anyarray).

  2. Fix ARRAY_SUBLINK and ARRAY[] for int2vector and oidvector input.

  3. Re-implement the ereport() macro using __VA_ARGS__.

  1. general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-09-27T13:15:45Z

    Hi hackers,

per David's suggestion, this patch implements general
purpose array sort.

We can do the following with this patch:

SELECT array_sort('{1.1,3.3,5.5,2.2,4.4,6.6}'::float8[], 'asc');
SELECT array_sort('{abc DEF 123abc,ábc sßs ßss DÉF,DŽxxDŽ džxxDž
Džxxdž,ȺȺȺ,ⱥⱥⱥ,ⱥȺ}'::text[]);
SELECT array_sort('{abc DEF 123abc,ábc sßs ßss DÉF,DŽxxDŽ džxxDž
Džxxdž,ȺȺȺ,ⱥⱥⱥ,ⱥȺ}'::text[], 'asc', 'pg_c_utf8');

-- 
Regards
Junwang Zhao

  2. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-09-28T11:52:00Z

    On Fri, Sep 27, 2024 at 9:15 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> Hi hackers,
>
> per David's suggestion, this patch implements general
> purpose array sort.
>
> We can do the following with this patch:
>
> SELECT array_sort('{1.1,3.3,5.5,2.2,4.4,6.6}'::float8[], 'asc');
> SELECT array_sort('{abc DEF 123abc,ábc sßs ßss DÉF,DŽxxDŽ džxxDž
> Džxxdž,ȺȺȺ,ⱥⱥⱥ,ⱥȺ}'::text[]);
> SELECT array_sort('{abc DEF 123abc,ábc sßs ßss DÉF,DŽxxDŽ džxxDž
> Džxxdž,ȺȺȺ,ⱥⱥⱥ,ⱥȺ}'::text[], 'asc', 'pg_c_utf8');
>
> --
> Regards
> Junwang Zhao

PFA v2, use COLLATE keyword to supply the collation suggested by
Andreas offlist.

SELECT array_sort('{abc DEF 123abc,ábc sßs ßss DÉF,DŽxxDŽ džxxDž
Džxxdž,ȺȺȺ,ⱥⱥⱥ,ⱥȺ}'::text[]);
SELECT array_sort('{abc DEF 123abc,ábc sßs ßss DÉF,DŽxxDŽ džxxDž
Džxxdž,ȺȺȺ,ⱥⱥⱥ,ⱥȺ}'::text[] COLLATE "pg_c_utf8");

I also created a CF entry[1] so it can be easily reviewed.

[1]: https://commitfest.postgresql.org/50/5277/

-- 
Regards
Junwang Zhao

  3. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-09-28T14:40:00Z

    On Sat, Sep 28, 2024 at 7:52 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> PFA v2, use COLLATE keyword to supply the collation suggested by
> Andreas offlist.
>
this is better. otherwise we need extra care to handle case like:
SELECT array_sort('{1,3,5,2,4,6}'::int[] COLLATE "pg_c_utf8");


+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>array_sort</primary>
+        </indexterm>
+        <function>array_sort</function> ( <type>anyarray</type>
<optional>, <parameter>dir</parameter> </optional>)
+        <returnvalue>anyarray</returnvalue>
+       </para>
+       <para>
+        Sorts the array in either ascending or descending order.
+        <parameter>dir</parameter> must be <literal>asc</literal>
+        or <literal>desc</literal>. The array must be empty or one-dimensional.
+       </para>
+       <para>
+        <literal>array_sort(ARRAY[1,2,5,6,3,4])</literal>
+        <returnvalue>{1,2,3,4,5,6}</returnvalue>
+       </para></entry>
+      </row>
I am confused with <parameter>dir</parameter>. I guess you want to say
"direction"
But here, I think <parameter>sort_asc</parameter> would be more appropriate?


<parameter>dir</parameter> can have only two potential values, make it
as a boolean would be more easier?
you didn't mention information:  "by default, it will sort by
ascending order; the sort collation by default is using the array
element type's collation"

tuplesort_begin_datum can do null-first, null-last, so the
one-dimension array can allow null values.

Based on the above and others, I did some refactoring, feel free to take it.
my changes, changed the function signature, so you need to pay
attention to sql test file.

  4. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-09-29T02:05:34Z

    On Sat, Sep 28, 2024 at 10:41 PM jian he <jian.universality@gmail.com> wrote:
>
> On Sat, Sep 28, 2024 at 7:52 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > PFA v2, use COLLATE keyword to supply the collation suggested by
> > Andreas offlist.
> >
> this is better. otherwise we need extra care to handle case like:
> SELECT array_sort('{1,3,5,2,4,6}'::int[] COLLATE "pg_c_utf8");
>
>
> +      <row>
> +       <entry role="func_table_entry"><para role="func_signature">
> +        <indexterm>
> +         <primary>array_sort</primary>
> +        </indexterm>
> +        <function>array_sort</function> ( <type>anyarray</type>
> <optional>, <parameter>dir</parameter> </optional>)
> +        <returnvalue>anyarray</returnvalue>
> +       </para>
> +       <para>
> +        Sorts the array in either ascending or descending order.
> +        <parameter>dir</parameter> must be <literal>asc</literal>
> +        or <literal>desc</literal>. The array must be empty or one-dimensional.
> +       </para>
> +       <para>
> +        <literal>array_sort(ARRAY[1,2,5,6,3,4])</literal>
> +        <returnvalue>{1,2,3,4,5,6}</returnvalue>
> +       </para></entry>
> +      </row>
> I am confused with <parameter>dir</parameter>. I guess you want to say
> "direction"
> But here, I think <parameter>sort_asc</parameter> would be more appropriate?

This doc is mostly copied and edited from intarray.sgml sort part.

And the logic is basically the same, you can check the intarray module.

>
>
> <parameter>dir</parameter> can have only two potential values, make it
> as a boolean would be more easier?
> you didn't mention information:  "by default, it will sort by
> ascending order; the sort collation by default is using the array
> element type's collation"
>
> tuplesort_begin_datum can do null-first, null-last, so the
> one-dimension array can allow null values.

The following(create extension intarry first) will give an error, I
keep the same for array_sort.

SELECT sort('{1234234,-30,234234, null}');

>
> Based on the above and others, I did some refactoring, feel free to take it.
> my changes, changed the function signature, so you need to pay
> attention to sql test file.

Thanks for your refactor, I will take some in the next version.


-- 
Regards
Junwang Zhao

  5. Re: general purpose array_sort

    David G. Johnston <david.g.johnston@gmail.com> — 2024-09-29T02:50:38Z

    On Sat, Sep 28, 2024 at 7:05 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

> On Sat, Sep 28, 2024 at 10:41 PM jian he <jian.universality@gmail.com>
> wrote:
> >
> > <parameter>dir</parameter> can have only two potential values, make it
> > as a boolean would be more easier?
> > you didn't mention information:  "by default, it will sort by
> > ascending order; the sort collation by default is using the array
> > element type's collation"
> >
> > tuplesort_begin_datum can do null-first, null-last, so the
> > one-dimension array can allow null values.
>
> The following(create extension intarry first) will give an error, I
> keep the same for array_sort.
>
> SELECT sort('{1234234,-30,234234, null}');
>
>
I would suggest accepting:
asc
desc
asc nulls first
asc nulls last *
desc nulls first *
desc nulls last

As valid inputs for "dir" - and that the starred options are the defaults
when null position is omitted.

In short, mimic create index.

David J.

  6. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-09-30T05:01:25Z

    On Sun, Sep 29, 2024 at 10:51 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Sat, Sep 28, 2024 at 7:05 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>>
>> On Sat, Sep 28, 2024 at 10:41 PM jian he <jian.universality@gmail.com> wrote:
>> >
>> > <parameter>dir</parameter> can have only two potential values, make it
>> > as a boolean would be more easier?
>> > you didn't mention information:  "by default, it will sort by
>> > ascending order; the sort collation by default is using the array
>> > element type's collation"
>> >
>> > tuplesort_begin_datum can do null-first, null-last, so the
>> > one-dimension array can allow null values.
>>
>> The following(create extension intarry first) will give an error, I
>> keep the same for array_sort.
>>
>> SELECT sort('{1234234,-30,234234, null}');
>>
>
> I would suggest accepting:
> asc
> desc
> asc nulls first
> asc nulls last *
> desc nulls first *
> desc nulls last
>
> As valid inputs for "dir" - and that the starred options are the defaults when null position is omitted.
>
> In short, mimic create index.
>
> David J.
>

PFA v3 with David's suggestion addressed.

-- 
Regards
Junwang Zhao

  7. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-09-30T15:13:06Z

    On Mon, Sep 30, 2024 at 1:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> > I would suggest accepting:
> > asc
> > desc
> > asc nulls first
> > asc nulls last *
> > desc nulls first *
> > desc nulls last
> >
> > As valid inputs for "dir" - and that the starred options are the defaults when null position is omitted.
> >
> > In short, mimic create index.
> >
> > David J.
> >
>
> PFA v3 with David's suggestion addressed.
>

I think just adding 2 bool arguments (asc/desc, nulls last/not nulls
last) would be easier.
but either way, (i don't have a huge opinion)
but document the second argument, imagine case
SELECT array_sort('{a,B}'::text[] , E'aSc NulLs  LaST \t\r\n');
would be tricky?


errmsg("multidimensional arrays sorting are not supported")));
write a sql test to trigger the error message that would be great.

you can add two or one example to collate.icu.utf8.sql to demo that it
actually works with COLLATE  collation_name
like:
SELECT array_sort('{a,B}'::text[] COLLATE case_insensitive);
SELECT array_sort('{a,B}'::text[] COLLATE "C");


#define WHITESPACE " \t\n\r"
you may also check function scanner_isspace


+ 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))));

  8. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-01T05:23:07Z

    Hi Jian,

On Mon, Sep 30, 2024 at 11:13 PM jian he <jian.universality@gmail.com> wrote:
>
> On Mon, Sep 30, 2024 at 1:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > > I would suggest accepting:
> > > asc
> > > desc
> > > asc nulls first
> > > asc nulls last *
> > > desc nulls first *
> > > desc nulls last
> > >
> > > As valid inputs for "dir" - and that the starred options are the defaults when null position is omitted.
> > >
> > > In short, mimic create index.
> > >
> > > David J.
> > >
> >
> > PFA v3 with David's suggestion addressed.
> >
>
> I think just adding 2 bool arguments (asc/desc, nulls last/not nulls
> last) would be easier.
Yeah, this would be easier, it's just the intarray module use
the direction parameter, I keep it here for the same user
experience, I don't insist if some committer thinks 2 bool arguments
would be a better option.

> but either way, (i don't have a huge opinion)
> but document the second argument, imagine case
> SELECT array_sort('{a,B}'::text[] , E'aSc NulLs  LaST \t\r\n');
> would be tricky?

The case you provide should give the correct results, but
I doubt users will do this.
I'm not good at document wording, so you might give me some help
with the document part.

>
>
> errmsg("multidimensional arrays sorting are not supported")));
> write a sql test to trigger the error message that would be great.
>
> you can add two or one example to collate.icu.utf8.sql to demo that it
> actually works with COLLATE  collation_name
> like:
> SELECT array_sort('{a,B}'::text[] COLLATE case_insensitive);
> SELECT array_sort('{a,B}'::text[] COLLATE "C");
>
Fixed.

>
> #define WHITESPACE " \t\n\r"
> you may also check function scanner_isspace
>
Fixed.

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

-- 
Regards
Junwang Zhao

  9. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-10-02T01:50:47Z

    > >
> > + 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

  10. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-02T14:46:15Z

    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.


-- 
Regards
Junwang Zhao

  11. Re: general purpose array_sort

    Amit Langote <amitlangote09@gmail.com> — 2024-10-03T06:22:24Z

    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

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.

-- 
Thanks, Amit Langote

  12. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-09T14:10:27Z

    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

  13. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-09T15:46:19Z

    On Wed, Oct 9, 2024 at 10:10 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> 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.

Sadly the CI only passed on my own github repo, it failed on
cfbot[1], will dig into the reason later because I can not open the cirrus
ci page right now ;(

[1] https://cirrus-ci.com/task/5815925960605696

>
> >
> > --
> > Thanks, Amit Langote
>
>
>
> --
> Regards
> Junwang Zhao



-- 
Regards
Junwang Zhao

  14. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-10T02:30:06Z

    On Wed, Oct 9, 2024 at 11:46 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Wed, Oct 9, 2024 at 10:10 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > 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.
>
> Sadly the CI only passed on my own github repo, it failed on
> cfbot[1], will dig into the reason later because I can not open the cirrus
> ci page right now ;(
>
> [1] https://cirrus-ci.com/task/5815925960605696
>

Seems the failure is not related to this patch, I guess the reason for
this is the stop phase doesn't
release the port properly?

2024-10-09 14:53:10.079 UTC [43052][checkpointer] LOG:  checkpoint
complete: wrote 5617 buffers (34.3%), wrote 3 SLRU buffers; 0 WAL
file(s) added, 0 removed, 3 recycled; write=0.107 s, sync=0.001 s,
total=0.107 s; sync files=0, longest=0.000 s, average=0.000 s;
distance=45239 kB, estimate=45239 kB; lsn=0/414A138, redo
lsn=0/414A138
2024-10-09 14:53:10.084 UTC [43050][postmaster] LOG:  database system
is shut down
2024-10-09 14:53:10.215 UTC [43270][postmaster] LOG:  starting
PostgreSQL 18devel on x86_64-freebsd, compiled by clang-17.0.6, 64-bit
2024-10-09 14:53:10.215 UTC [43270][postmaster] LOG:  could not bind
IPv4 address "127.0.0.1": Address already in use
2024-10-09 14:53:10.215 UTC [43270][postmaster] HINT:  Is another
postmaster already running on port 11643? If not, wait a few seconds
and retry.
2024-10-09 14:53:10.215 UTC [43270][postmaster] WARNING:  could not
create listen socket for "127.0.0.1"
2024-10-09 14:53:10.218 UTC [43270][postmaster] FATAL:  could not
create any TCP/IP sockets
2024-10-09 14:53:10.218 UTC [43270][postmaster] LOG:  database system
is shut down


https://api.cirrus-ci.com/v1/artifact/task/5815925960605696/testrun/build/testrun/ssl/001_ssltests/log/001_ssltests_primary.log

> >
> > >
> > > --
> > > Thanks, Amit Langote
> >
> >
> >
> > --
> > Regards
> > Junwang Zhao
>
>
>
> --
> Regards
> Junwang Zhao



-- 
Regards
Junwang Zhao

  15. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-10-11T05:12:40Z

    tricky case:
should we allow array element type to be composite/domain?
currently seems to work fine.


create table t(b int[]);
insert into t values ('{{1,3}}'), ('{{1,2}}');

 select array_sort((select array_agg(t) from t), 'desc');
            array_sort
-----------------------------------
 {"(\"{{1,3}}\")","(\"{{1,2}}\")"}


select array_sort((t.b)) from t;
ERROR:  multidimensional arrays sorting are not supported


select array_sort((select array_agg(t.b) from t));
ERROR:  multidimensional arrays sorting are not supported

  16. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-13T14:43:22Z

    Hi Jian,

On Fri, Oct 11, 2024 at 1:12 PM jian he <jian.universality@gmail.com> wrote:
>
> tricky case:
> should we allow array element type to be composite/domain?
> currently seems to work fine.
>
>
> create table t(b int[]);
> insert into t values ('{{1,3}}'), ('{{1,2}}');
>
>  select array_sort((select array_agg(t) from t), 'desc');
>             array_sort
> -----------------------------------
>  {"(\"{{1,3}}\")","(\"{{1,2}}\")"}
>
>
> select array_sort((t.b)) from t;
> ERROR:  multidimensional arrays sorting are not supported
>
>
> select array_sort((select array_agg(t.b) from t));
> ERROR:  multidimensional arrays sorting are not supported

I tried the above cases, and the first one works because it's
a one dim array of composite type, the other two fails because
they are multidimensional.

It seems there is not much meaning to sort composite type,
so are you proposing we should error on that?

-- 
Regards
Junwang Zhao

  17. Re: general purpose array_sort

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-13T20:10:39Z

    Junwang Zhao <zhjwpku@gmail.com> writes:
> It seems there is not much meaning to sort composite type,
> so are you proposing we should error on that?

It's hardly "general purpose" if it randomly refuses to
sort certain types.  I would say it should be able to sort
anything that ORDER BY will handle --- and that certainly
includes the cases shown here.

			regards, tom lane

  18. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-23T14:28:05Z

    Sorry for the late reply.

On Mon, Oct 14, 2024 at 4:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Junwang Zhao <zhjwpku@gmail.com> writes:
> > It seems there is not much meaning to sort composite type,
> > so are you proposing we should error on that?
>
> It's hardly "general purpose" if it randomly refuses to
> sort certain types.  I would say it should be able to sort
> anything that ORDER BY will handle --- and that certainly
> includes the cases shown here.

Yeah, agreed.

PFA v7 with multi-array support.

>
>                         regards, tom lane



-- 
Regards
Junwang Zhao

  19. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-10-24T12:39:57Z

    On Wed, Oct 23, 2024 at 10:28 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> PFA v7 with multi-array support.
>

if (ARR_NDIM(array) == 1)
{
}
else
{
}
can be simplified.
i think beginning part of array_sort can be like the following:
(newline emitted)

---------------------------------------------------------------------
    if (ARR_NDIM(array) < 1)
        PG_RETURN_ARRAYTYPE_P(array);
    if (dirstr != NULL)
    {
        if (!parse_sort_order(text_to_cstring(dirstr), &sort_asc, &nulls_first))
            ereport(ERROR,
                    (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                     errmsg("second parameter must be a valid sort
direction")));
    }
    elmtyp = ARR_ELEMTYPE(array);
    if (ARR_NDIM(array) > 1)
        elmtyp = get_array_type(elmtyp);
    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);
        if ((sort_asc && !OidIsValid(typentry->lt_opr)) ||
            (!sort_asc && !OidIsValid(typentry->gt_opr)))
            ereport(ERROR,
                    (errcode(ERRCODE_UNDEFINED_FUNCTION),
                    errmsg("could not identify an ordering operator
for type %s",
                            format_type_be(elmtyp))));
        fcinfo->flinfo->fn_extra = (void *) typentry;
    }
---------------------------------------------------------------------
/*
 * array_sort
 *
 * Sorts the array in either ascending or descending order.
 * The array must be empty or one-dimensional.
 */
comments need to be updated.


typedef enum
    PARSE_SORT_ORDER_DONE
} ParseSortOrderState;

last one, should have comma, like
"PARSE_SORT_ORDER_DONE, "

  20. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-24T14:33:00Z

    On Thu, Oct 24, 2024 at 8:40 PM jian he <jian.universality@gmail.com> wrote:
>
> On Wed, Oct 23, 2024 at 10:28 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > PFA v7 with multi-array support.
> >
>
> if (ARR_NDIM(array) == 1)
> {
> }
> else
> {
> }
> can be simplified.
> i think beginning part of array_sort can be like the following:
> (newline emitted)
>
> ---------------------------------------------------------------------
>     if (ARR_NDIM(array) < 1)
>         PG_RETURN_ARRAYTYPE_P(array);
>     if (dirstr != NULL)
>     {
>         if (!parse_sort_order(text_to_cstring(dirstr), &sort_asc, &nulls_first))
>             ereport(ERROR,
>                     (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>                      errmsg("second parameter must be a valid sort
> direction")));
>     }
>     elmtyp = ARR_ELEMTYPE(array);
>     if (ARR_NDIM(array) > 1)
>         elmtyp = get_array_type(elmtyp);

I'm wondering should we cache the type entry for both element type and
the corresponding array type, for example if we have a table:

create table t(b int[]);
insert into t values ('{1,3}'),('{{2,3}}'),('{{1,2},{0,2}}');

with 1-d array and m-d array interleaved, then the following query will
call lookup_type_cache multiple times.

select array_sort((t.b)) from t;

>     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);
>         if ((sort_asc && !OidIsValid(typentry->lt_opr)) ||
>             (!sort_asc && !OidIsValid(typentry->gt_opr)))
>             ereport(ERROR,
>                     (errcode(ERRCODE_UNDEFINED_FUNCTION),
>                     errmsg("could not identify an ordering operator
> for type %s",
>                             format_type_be(elmtyp))));
>         fcinfo->flinfo->fn_extra = (void *) typentry;
>     }
> ---------------------------------------------------------------------
> /*
>  * array_sort
>  *
>  * Sorts the array in either ascending or descending order.
>  * The array must be empty or one-dimensional.
>  */
> comments need to be updated.

will fix it in the next version of patch.

>
>
> typedef enum
>     PARSE_SORT_ORDER_DONE
> } ParseSortOrderState;
>
> last one, should have comma, like
> "PARSE_SORT_ORDER_DONE, "

will fix it.


-- 
Regards
Junwang Zhao

  21. Re: general purpose array_sort

    Aleksander Alekseev <aleksander@timescale.com> — 2024-10-24T14:57:52Z

    Hi,

> It's hardly "general purpose" if it randomly refuses to
> sort certain types.  I would say it should be able to sort
> anything that ORDER BY will handle --- and that certainly
> includes the cases shown here.

I wonder how useful / convenient the new function will be considering
that we already have CTEs and can do:

SELECT array_agg(x ORDER BY x) FROM unnest(ARRAY[5,1,3,2,4]) AS x;

Perhaps there are use cases I didn't consider?

-- 
Best regards,
Aleksander Alekseev

  22. Re: general purpose array_sort

    David G. Johnston <david.g.johnston@gmail.com> — 2024-10-24T15:11:33Z

    On Thu, Oct 24, 2024 at 7:58 AM Aleksander Alekseev <
aleksander@timescale.com> wrote:

> Hi,
>
> > It's hardly "general purpose" if it randomly refuses to
> > sort certain types.  I would say it should be able to sort
> > anything that ORDER BY will handle --- and that certainly
> > includes the cases shown here.
>
> I wonder how useful / convenient the new function will be considering
> that we already have CTEs and can do:
>
> SELECT array_agg(x ORDER BY x) FROM unnest(ARRAY[5,1,3,2,4]) AS x;
>
> Perhaps there are use cases I didn't consider?
>
>
Succinctness of expression.  Plus I'm under the impression that a function
doing this is going to be somewhat faster than composing two functions
together within a multi-node subtree.

I feel like the same observation could have been made for array_shuffle but
we added that.  This function being added feels to me like just completing
the set.

David J.

  23. Re: general purpose array_sort

    Aleksander Alekseev <aleksander@timescale.com> — 2024-10-24T15:27:46Z

    Hi David,

>> > It's hardly "general purpose" if it randomly refuses to
>> > sort certain types.  I would say it should be able to sort
>> > anything that ORDER BY will handle --- and that certainly
>> > includes the cases shown here.
>>
>> I wonder how useful / convenient the new function will be considering
>> that we already have CTEs and can do:
>>
>> SELECT array_agg(x ORDER BY x) FROM unnest(ARRAY[5,1,3,2,4]) AS x;
>>
>> Perhaps there are use cases I didn't consider?
>>
>
> Succinctness of expression.  Plus I'm under the impression that a function doing this is going to be somewhat faster than composing two functions together within a multi-node subtree.
>
> I feel like the same observation could have been made for array_shuffle but we added that.  This function being added feels to me like just completing the set.

Right. To clarify, I'm not opposed to array_sort(). In fact personally
I would prefer using it instead of array_agg() + unnest().

Just making an observation / thinking out loud that the requirement to
support everything ORDER BY handles / supports (and will handle /
support?) might make this function impractical to maintain.
array_shuffle() or a recently proposed array_reverse() [1] are rather
simple since they just move the array elements without looking at
them. array_sort() does look at the elements and thus is very
different.

Particularly, does the function really need to support dir => asc |
desc | asc nulls first | etc... ? Maybe array_sort() + array_reverse(
array_sort() ) will handle most of the practical cases? I don't know.

[1]: https://commitfest.postgresql.org/50/5314/
--
Best regards,
Aleksander Alekseev

  24. Re: general purpose array_sort

    David G. Johnston <david.g.johnston@gmail.com> — 2024-10-24T15:40:04Z

    On Thu, Oct 24, 2024 at 8:27 AM Aleksander Alekseev <
aleksander@timescale.com> wrote:

>
> Just making an observation / thinking out loud that the requirement to
> support everything ORDER BY handles / supports (and will handle /
> support?) might make this function impractical to maintain.
>
> Particularly, does the function really need to support dir => asc |
> desc | asc nulls first | etc... ? Maybe array_sort() + array_reverse(
> array_sort() ) will handle most of the practical cases? I don't know.
>
> [1]: https://commitfest.postgresql.org/50/5314/
>
>
Composing function calls here seems quite undesirable from a performance
standpoint, but maybe I over-estimate the cost of
exploding-manipulating-freezing an array datum.  Also, while I'm not in a
good position to judge the challenge of implementing the sort parameters I
would rather have them than not since the order by API has them (plus
performance).  I also, maybe unreasonably, believe that our extensible type
system has already limited the maintenance burden.

David J.

  25. Re: general purpose array_sort

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-24T16:06:32Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> Composing function calls here seems quite undesirable from a performance
> standpoint, but maybe I over-estimate the cost of
> exploding-manipulating-freezing an array datum.  Also, while I'm not in a
> good position to judge the challenge of implementing the sort parameters I
> would rather have them than not since the order by API has them (plus
> performance).  I also, maybe unreasonably, believe that our extensible type
> system has already limited the maintenance burden.

Oh!  I had not actually looked at the details of what was being
proposed here.  I imagined "array_sort(anyarray)" and the sort would
use the default sort order for the array's element type.  This
business with a textual representation of a sort clause seems like
over-engineering ... except that it's also under-engineered, because
the parsing is lame and incomplete.  (No USING option, and the fact
that collation comes from somewhere else seems impossibly confusing.)
Let's drop that.  As already noted, if you need a non-default sort
order you can build what you want with a sub-select.  The ambition
here should be to make easy cases easy.

			regards, tom lane

  26. Re: general purpose array_sort

    David G. Johnston <david.g.johnston@gmail.com> — 2024-10-24T16:47:54Z

    On Thu, Oct 24, 2024 at 9:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> This business with a textual representation of a sort clause seems like
> over-engineering ... except that it's also under-engineered, because
> the parsing is lame and incomplete.  (No USING option, and the fact
> that collation comes from somewhere else seems impossibly confusing.)
> Let's drop that.
>

I can accept this outcome though an optional three-valued boolean sort
order (ascending and descending only) I'd argue is worth keeping.  null
value placement too I guess, three-valued boolean (nulls_first).

David J.

  27. Re: general purpose array_sort

    Aleksander Alekseev <aleksander@timescale.com> — 2024-10-24T17:18:55Z

    Hi,

> I can accept this outcome though an optional three-valued boolean sort order (ascending and descending only) I'd argue is worth keeping.  null value placement too I guess, three-valued boolean (nulls_first).

Perhaps these optional arguments deserve separate discussions. I
suggest merging something everyone agrees on first. This will simplify
the review process and allow us to deliver value to the users quickly.
Arguments like `reverse => true` and `nulls_first => true` can always
be implemented and added as separate patches.

-- 
Best regards,
Aleksander Alekseev

  28. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-25T12:02:02Z

    On Fri, Oct 25, 2024 at 1:19 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> > I can accept this outcome though an optional three-valued boolean sort order (ascending and descending only) I'd argue is worth keeping.  null value placement too I guess, three-valued boolean (nulls_first).
>
> Perhaps these optional arguments deserve separate discussions. I
> suggest merging something everyone agrees on first. This will simplify
> the review process and allow us to deliver value to the users quickly.
> Arguments like `reverse => true` and `nulls_first => true` can always
> be implemented and added as separate patches.

As this patch uses the tuplesort infrastructure, we need to supply the
sortOperator, sortCollation and nullsFirstFlag, I tend to agree with
David. I admit that the parsing part is not good, so I will remove it
by using two boolean parameters Jian suggested earlier.

Will send out another version by tomorrow.

>
> --
> Best regards,
> Aleksander Alekseev



-- 
Regards
Junwang Zhao

  29. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-26T04:20:30Z

    On Fri, Oct 25, 2024 at 8:02 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Fri, Oct 25, 2024 at 1:19 AM Aleksander Alekseev
> <aleksander@timescale.com> wrote:
> >
> > Hi,
> >
> > > I can accept this outcome though an optional three-valued boolean sort order (ascending and descending only) I'd argue is worth keeping.  null value placement too I guess, three-valued boolean (nulls_first).
> >
> > Perhaps these optional arguments deserve separate discussions. I
> > suggest merging something everyone agrees on first. This will simplify
> > the review process and allow us to deliver value to the users quickly.
> > Arguments like `reverse => true` and `nulls_first => true` can always
> > be implemented and added as separate patches.
>
> As this patch uses the tuplesort infrastructure, we need to supply the
> sortOperator, sortCollation and nullsFirstFlag, I tend to agree with
> David. I admit that the parsing part is not good, so I will remove it
> by using two boolean parameters Jian suggested earlier.
>
> Will send out another version by tomorrow.

Based on the previous discussion, I split it into two patches in V8.

0001 is the general sort part without `is_ascending` or `nulls_first`,
the sort order is determined by the "<" operator of the element type.
It also cached the type entry of both eletyp and the corresponding
array type.

0002 adds the `is_ascending` and `nulls_first` part, it now uses
two boolean parameters instead of parsing one text parameter.

>
> >
> > --
> > Best regards,
> > Aleksander Alekseev
>
>
>
> --
> Regards
> Junwang Zhao



-- 
Regards
Junwang Zhao

  30. Re: general purpose array_sort

    Aleksander Alekseev <aleksander@timescale.com> — 2024-10-28T16:48:05Z

    Hi,

> Based on the previous discussion, I split it into two patches in V8.
>
> 0001 is the general sort part without `is_ascending` or `nulls_first`,
> the sort order is determined by the "<" operator of the element type.
> It also cached the type entry of both eletyp and the corresponding
> array type.
>
> 0002 adds the `is_ascending` and `nulls_first` part, it now uses
> two boolean parameters instead of parsing one text parameter.

Thanks for the update patch set. Here are some comments.

0001:

> +{ oid => '8810', descr => 'sort array',
> +  proname => 'array_sort', provolatile => 'v', prorettype => 'anyarray',
> +  proargtypes => 'anyarray', prosrc => 'array_sort'},

I would expect that array_sort() should be IMMUTABLE. Is there a
reason for it to be VOLATILE?

> +        <function>array_sort</function> ( <type>anyarray</type> <optional> COLLATE <replaceable>collation_name</replaceable> </optional>)
> +        <returnvalue>anyarray</returnvalue>

It seems to me that the part about using COLLATE should be moved
below, to the description / examples section, since it's not part of
the function signature.

Also the description should be more specific about how NULLs are
sorted. NULLs also should be covered by tests.

0002:

> <parameter>is_ascending</parameter>

I really believe this name is not the best one. I suggest using
`reverse => true`. `nulls_first` is OK.

> +Datum
> +array_sort_order(PG_FUNCTION_ARGS)
> +{
> +    return array_sort(fcinfo);
> +}
> +
> +Datum
> +array_sort_order_nulls_first(PG_FUNCTION_ARGS)
> +{
> +    return array_sort(fcinfo);
> +}

Any reason not to specify array_sort in pg_proc.dat?

The tests cover is_ascending => true | false, which is OK, but only
(is_ascending = true, nulls_first => true) and (is_ascending => false,
nulls_fist => false). For the case when both optional arguments are
specified you have to test at least 4 combinations.

-- 
Best regards,
Aleksander Alekseev

  31. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-10-29T08:28:46Z

    On Tue, Oct 29, 2024 at 12:48 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:.
>
> 0001:
>
> > +{ oid => '8810', descr => 'sort array',
> > +  proname => 'array_sort', provolatile => 'v', prorettype => 'anyarray',
> > +  proargtypes => 'anyarray', prosrc => 'array_sort'},
>
> I would expect that array_sort() should be IMMUTABLE. Is there a
> reason for it to be VOLATILE?
>

https://www.postgresql.org/docs/current/sql-createfunction.html says:

IMMUTABLE indicates that the function cannot modify the database and always
returns the same result when given the same argument values; that is, it does
not do database lookups or otherwise use information not directly present in its
argument list. If this option is given, any call of the function with
all-constant arguments can be immediately replaced with the function value.


+ {
+ typentry = lookup_type_cache(elmtyp, TYPECACHE_LT_OPR);
+ if (!OidIsValid(typentry->lt_opr))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_FUNCTION),
+ errmsg("could not identify ordering operator for type %s",
+ format_type_be(elmtyp))));

This error can happen. I think this conflicts with the doc IMMUTABLE
description.

  32. Re: general purpose array_sort

    Aleksander Alekseev <aleksander@timescale.com> — 2024-10-29T12:14:42Z

    Hi Jian,

> IMMUTABLE indicates that the function cannot modify the database and always
> returns the same result when given the same argument values; that is, it does
> not do database lookups or otherwise use information not directly present in its
> argument list. If this option is given, any call of the function with
> all-constant arguments can be immediately replaced with the function value.
>
>
> + {
> + typentry = lookup_type_cache(elmtyp, TYPECACHE_LT_OPR);
> + if (!OidIsValid(typentry->lt_opr))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_FUNCTION),
> + errmsg("could not identify ordering operator for type %s",
> + format_type_be(elmtyp))));
>
> This error can happen. I think this conflicts with the doc IMMUTABLE
> description.

lookup_type_cache() is used at least by array_position() which is
marked as IMMUTABLE, so I believe this is fine. Similarly functions
dealing with timezones can return different results between the DBMS
restarts / updates, but we don't care and mark them IMMUTABLE anyway.
Otherwise we couldn't use these functions in functional indexes which
will make them rather useless.

-- 
Best regards,
Aleksander Alekseev

  33. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-29T14:49:33Z

    Hi Aleksander,

On Tue, Oct 29, 2024 at 12:48 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> > Based on the previous discussion, I split it into two patches in V8.
> >
> > 0001 is the general sort part without `is_ascending` or `nulls_first`,
> > the sort order is determined by the "<" operator of the element type.
> > It also cached the type entry of both eletyp and the corresponding
> > array type.
> >
> > 0002 adds the `is_ascending` and `nulls_first` part, it now uses
> > two boolean parameters instead of parsing one text parameter.
>
> Thanks for the update patch set. Here are some comments.
>
> 0001:
>
> > +{ oid => '8810', descr => 'sort array',
> > +  proname => 'array_sort', provolatile => 'v', prorettype => 'anyarray',
> > +  proargtypes => 'anyarray', prosrc => 'array_sort'},
>
> I would expect that array_sort() should be IMMUTABLE. Is there a
> reason for it to be VOLATILE?

I saw Jian's reply about this, but I tend to agree with you, so remove
provolatile => 'v'.

>
> > +        <function>array_sort</function> ( <type>anyarray</type> <optional> COLLATE <replaceable>collation_name</replaceable> </optional>)
> > +        <returnvalue>anyarray</returnvalue>
>
> It seems to me that the part about using COLLATE should be moved
> below, to the description / examples section, since it's not part of
> the function signature.

Agree, fixed with my own words, help needed with the wording.

>
> Also the description should be more specific about how NULLs are
> sorted. NULLs also should be covered by tests.

Fixed.

>
> 0002:
>
> > <parameter>is_ascending</parameter>
>
> I really believe this name is not the best one. I suggest using
> `reverse => true`. `nulls_first` is OK.

Not sure about this, I think `is_ascending` has a more precise
meaning, while `reverse` doesn't show any hint about ascending or
descending, just keep it right now, let's see others' opinions.

>
> > +Datum
> > +array_sort_order(PG_FUNCTION_ARGS)
> > +{
> > +    return array_sort(fcinfo);
> > +}
> > +
> > +Datum
> > +array_sort_order_nulls_first(PG_FUNCTION_ARGS)
> > +{
> > +    return array_sort(fcinfo);
> > +}
>
> Any reason not to specify array_sort in pg_proc.dat?

It is specified in 0001 (see oid => '8810').

>
> The tests cover is_ascending => true | false, which is OK, but only
> (is_ascending = true, nulls_first => true) and (is_ascending => false,
> nulls_fist => false). For the case when both optional arguments are
> specified you have to test at least 4 combinations.

The omitted two is the same as the two with two parameters specified,
anyway, add all 4 cases in v9.

>
> --
> Best regards,
> Aleksander Alekseev



-- 
Regards
Junwang Zhao

  34. Re: general purpose array_sort

    Aleksander Alekseev <aleksander@timescale.com> — 2024-10-30T13:29:00Z

    Hi,

Thanks for the updated patch set.

> > > +Datum
> > > +array_sort_order(PG_FUNCTION_ARGS)
> > > +{
> > > +    return array_sort(fcinfo);
> > > +}
> > > +
> > > +Datum
> > > +array_sort_order_nulls_first(PG_FUNCTION_ARGS)
> > > +{
> > > +    return array_sort(fcinfo);
> > > +}
> >
> > Any reason not to specify array_sort in pg_proc.dat?
>
> It is specified in 0001 (see oid => '8810').

What I meant was that I don't think these wrapper functions are
needed. I think you can just do:

```
+{ oid => '8811', descr => 'sort array',
+  proname => 'array_sort', prorettype => 'anyarray',
+  proargtypes => 'anyarray bool', prosrc => 'array_sort'}, <--
array_sort is used directly in `prosrc`
```

... unless I'm missing something.

-- 
Best regards,
Aleksander Alekseev

  35. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-30T14:17:18Z

    Hi,

On Wed, Oct 30, 2024 at 9:29 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> Thanks for the updated patch set.
>
> > > > +Datum
> > > > +array_sort_order(PG_FUNCTION_ARGS)
> > > > +{
> > > > +    return array_sort(fcinfo);
> > > > +}
> > > > +
> > > > +Datum
> > > > +array_sort_order_nulls_first(PG_FUNCTION_ARGS)
> > > > +{
> > > > +    return array_sort(fcinfo);
> > > > +}
> > >
> > > Any reason not to specify array_sort in pg_proc.dat?
> >
> > It is specified in 0001 (see oid => '8810').
>
> What I meant was that I don't think these wrapper functions are
> needed. I think you can just do:
>
> ```
> +{ oid => '8811', descr => 'sort array',
> +  proname => 'array_sort', prorettype => 'anyarray',
> +  proargtypes => 'anyarray bool', prosrc => 'array_sort'}, <--
> array_sort is used directly in `prosrc`
> ```
>
> ... unless I'm missing something.

There is a opr sanity check for this[1], if we remove these wrapper functions,
regression test will fail with:

- oid | proname | oid | proname
------+---------+-----+---------
-(0 rows)
+ oid  |  proname   | oid  |  proname
+------+------------+------+------------
+ 8811 | array_sort | 8812 | array_sort
+ 8810 | array_sort | 8811 | array_sort
+ 8810 | array_sort | 8812 | array_sort
+(3 rows)


[1]:

-- Considering only built-in procs (prolang = 12), look for multiple uses
-- of the same internal function (ie, matching prosrc fields). It's OK to
-- have several entries with different pronames for the same internal function,
-- but conflicts in the number of arguments and other critical items should
-- be complained of. (We don't check data types here; see next query.)
-- Note: ignore aggregate functions here, since they all point to the same
-- dummy built-in function.

SELECT p1.oid, p1.proname, p2.oid, p2.proname
FROM pg_proc AS p1, pg_proc AS p2
WHERE p1.oid < p2.oid AND
p1.prosrc = p2.prosrc AND
p1.prolang = 12 AND p2.prolang = 12 AND
(p1.prokind != 'a' OR p2.prokind != 'a') AND
(p1.prolang != p2.prolang OR
p1.prokind != p2.prokind OR
p1.prosecdef != p2.prosecdef OR
p1.proleakproof != p2.proleakproof OR
p1.proisstrict != p2.proisstrict OR
p1.proretset != p2.proretset OR
p1.provolatile != p2.provolatile OR
p1.pronargs != p2.pronargs);

>
> --
> Best regards,
> Aleksander Alekseev



-- 
Regards
Junwang Zhao

  36. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-10-30T14:41:40Z

    On Wed, Oct 30, 2024 at 10:17 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> Hi,
>
> On Wed, Oct 30, 2024 at 9:29 PM Aleksander Alekseev
> <aleksander@timescale.com> wrote:
> >
> > Hi,
> >
> > Thanks for the updated patch set.
> >
> > > > > +Datum
> > > > > +array_sort_order(PG_FUNCTION_ARGS)
> > > > > +{
> > > > > +    return array_sort(fcinfo);
> > > > > +}
> > > > > +
> > > > > +Datum
> > > > > +array_sort_order_nulls_first(PG_FUNCTION_ARGS)
> > > > > +{
> > > > > +    return array_sort(fcinfo);
> > > > > +}
> > > >
> > > > Any reason not to specify array_sort in pg_proc.dat?
> > >
> > > It is specified in 0001 (see oid => '8810').
> >
> > What I meant was that I don't think these wrapper functions are
> > needed. I think you can just do:
> >
> > ```
> > +{ oid => '8811', descr => 'sort array',
> > +  proname => 'array_sort', prorettype => 'anyarray',
> > +  proargtypes => 'anyarray bool', prosrc => 'array_sort'}, <--
> > array_sort is used directly in `prosrc`
> > ```
> >
> > ... unless I'm missing something.
>
> There is a opr sanity check for this[1], if we remove these wrapper functions,
> regression test will fail with:
>
> - oid | proname | oid | proname
> ------+---------+-----+---------
> -(0 rows)
> + oid  |  proname   | oid  |  proname
> +------+------------+------+------------
> + 8811 | array_sort | 8812 | array_sort
> + 8810 | array_sort | 8811 | array_sort
> + 8810 | array_sort | 8812 | array_sort
> +(3 rows)
>
>
> [1]:
>
> -- Considering only built-in procs (prolang = 12), look for multiple uses
> -- of the same internal function (ie, matching prosrc fields). It's OK to
> -- have several entries with different pronames for the same internal function,
> -- but conflicts in the number of arguments and other critical items should
> -- be complained of. (We don't check data types here; see next query.)
> -- Note: ignore aggregate functions here, since they all point to the same
> -- dummy built-in function.
>
> SELECT p1.oid, p1.proname, p2.oid, p2.proname
> FROM pg_proc AS p1, pg_proc AS p2
> WHERE p1.oid < p2.oid AND
> p1.prosrc = p2.prosrc AND
> p1.prolang = 12 AND p2.prolang = 12 AND
> (p1.prokind != 'a' OR p2.prokind != 'a') AND
> (p1.prolang != p2.prolang OR
> p1.prokind != p2.prokind OR
> p1.prosecdef != p2.prosecdef OR
> p1.proleakproof != p2.proleakproof OR
> p1.proisstrict != p2.proisstrict OR
> p1.proretset != p2.proretset OR
> p1.provolatile != p2.provolatile OR
> p1.pronargs != p2.pronargs);
>
> >
> > --
> > Best regards,
> > Aleksander Alekseev
>
>
>
> --
> Regards
> Junwang Zhao

CFbot failed with doc build, v10 fixed that.

-- 
Regards
Junwang Zhao

  37. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-11-03T03:33:05Z

    On Wed, Oct 30, 2024 at 10:41 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Wed, Oct 30, 2024 at 10:17 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > Hi,
> >
> > On Wed, Oct 30, 2024 at 9:29 PM Aleksander Alekseev
> > <aleksander@timescale.com> wrote:
> > >
> > > Hi,
> > >
> > > Thanks for the updated patch set.
> > >
> > > > > > +Datum
> > > > > > +array_sort_order(PG_FUNCTION_ARGS)
> > > > > > +{
> > > > > > +    return array_sort(fcinfo);
> > > > > > +}
> > > > > > +
> > > > > > +Datum
> > > > > > +array_sort_order_nulls_first(PG_FUNCTION_ARGS)
> > > > > > +{
> > > > > > +    return array_sort(fcinfo);
> > > > > > +}
> > > > >
> > > > > Any reason not to specify array_sort in pg_proc.dat?
> > > >
> > > > It is specified in 0001 (see oid => '8810').
> > >
> > > What I meant was that I don't think these wrapper functions are
> > > needed. I think you can just do:
> > >
> > > ```
> > > +{ oid => '8811', descr => 'sort array',
> > > +  proname => 'array_sort', prorettype => 'anyarray',
> > > +  proargtypes => 'anyarray bool', prosrc => 'array_sort'}, <--
> > > array_sort is used directly in `prosrc`
> > > ```
> > >
> > > ... unless I'm missing something.
> >
> > There is a opr sanity check for this[1], if we remove these wrapper functions,
> > regression test will fail with:
> >
> > - oid | proname | oid | proname
> > ------+---------+-----+---------
> > -(0 rows)
> > + oid  |  proname   | oid  |  proname
> > +------+------------+------+------------
> > + 8811 | array_sort | 8812 | array_sort
> > + 8810 | array_sort | 8811 | array_sort
> > + 8810 | array_sort | 8812 | array_sort
> > +(3 rows)
> >
> >
> > [1]:
> >
> > -- Considering only built-in procs (prolang = 12), look for multiple uses
> > -- of the same internal function (ie, matching prosrc fields). It's OK to
> > -- have several entries with different pronames for the same internal function,
> > -- but conflicts in the number of arguments and other critical items should
> > -- be complained of. (We don't check data types here; see next query.)
> > -- Note: ignore aggregate functions here, since they all point to the same
> > -- dummy built-in function.
> >
> > SELECT p1.oid, p1.proname, p2.oid, p2.proname
> > FROM pg_proc AS p1, pg_proc AS p2
> > WHERE p1.oid < p2.oid AND
> > p1.prosrc = p2.prosrc AND
> > p1.prolang = 12 AND p2.prolang = 12 AND
> > (p1.prokind != 'a' OR p2.prokind != 'a') AND
> > (p1.prolang != p2.prolang OR
> > p1.prokind != p2.prokind OR
> > p1.prosecdef != p2.prosecdef OR
> > p1.proleakproof != p2.proleakproof OR
> > p1.proisstrict != p2.proisstrict OR
> > p1.proretset != p2.proretset OR
> > p1.provolatile != p2.provolatile OR
> > p1.pronargs != p2.pronargs);
> >
> > >
> > > --
> > > Best regards,
> > > Aleksander Alekseev
> >
> >
> >
> > --
> > Regards
> > Junwang Zhao
>
> CFbot failed with doc build, v10 fixed that.
>
> --
> Regards
> Junwang Zhao


Rebase needed due to array_reverse committed, PFA v11.


-- 
Regards
Junwang Zhao

  38. Re: general purpose array_sort

    Michael Paquier <michael@paquier.xyz> — 2024-11-04T05:46:39Z

    On Sun, Nov 03, 2024 at 11:33:05AM +0800, Junwang Zhao wrote:
> Rebase needed due to array_reverse committed, PFA v11.

There has been another conflict since you have posted this version
(noticed that after my business in 027124a872d7).  I have looked at
0001.

+    if (ARR_NDIM(array) < 1)
+        PG_RETURN_ARRAYTYPE_P(array);
There is no point in doing a sort if the array has only one element.
You can add a check based on "ARR_DIMS(array)[0] < 2" to achieve that.

+    typentry = lookup_type_cache(elmtyp, TYPECACHE_LT_OPR);
+    if (!OidIsValid(typentry->lt_opr))
+        ereport(ERROR,
+                (errcode(ERRCODE_UNDEFINED_FUNCTION),
+                 errmsg("could not identify ordering operator for type %s",
+                    format_type_be(elmtyp))));

The patch introduces two error paths based on the fact that ordering
operators could not be found depending on a data type that lacks the
ordering operator and the array ordering operator part.  It is right
to issue an error if these are lacking, like the various stats paths.
Should we have some regression tests with specific data types for
these errors, though?  The stats paths don't care much about these
error cases, but it does not mean that we should not care about them.
In short, let's have negative test coverage if we can.

+typedef struct ArraySortCachedInfo
+{
+    TypeCacheEntry *typentry;
+    TypeCacheEntry *array_typentry;
+} ArraySortCachedInfo;

Let's put that at the top of the file, with a comment about how it
links to array_sort() for the caching with fn_extra.  Let's also
document the meaning of the fields.

FWIW, I am confused by this implementation, where you have to allocate
the two TypeCacheEntry because of the fact that you have to deal with
the 1-dimension case and the multi-dimension case.  In the context of
a single function call, why do you need both typentry and
array_typentry, actually?  Wouldn't it be enough to use one typentry
that points to the typcache, meaning that you don't really need to use
the extra business with fn_mcxt, no?  If you require both (because I
may be wrong), perhaps you should have a regression test that's able
to break when removing array_typentry, changing the code to only rely
on typentry.   Note: I have just removed array_typentry in a quick
test, current coverage was happy about it.  Feel free to prove me
wrong.

Agreed that the function should be immutable.  The results are fixed
depending on the input even with the COLLATE clauses appended.

Let's add something when there is only one element in the first
dimension of the array, say two cases one with an int and one with an
array of ints like:
SELECT array_sort('{1}'::int[]);
SELECT array_sort('{{1}}'::int[]);
--
Michael

  39. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-11-04T07:16:35Z

    On Mon, Nov 4, 2024 at 1:46 PM Michael Paquier <michael@paquier.xyz> wrote:
>
>
> +    typentry = lookup_type_cache(elmtyp, TYPECACHE_LT_OPR);
> +    if (!OidIsValid(typentry->lt_opr))
> +        ereport(ERROR,
> +                (errcode(ERRCODE_UNDEFINED_FUNCTION),
> +                 errmsg("could not identify ordering operator for type %s",
> +                    format_type_be(elmtyp))));
>
> The patch introduces two error paths based on the fact that ordering
> operators could not be found depending on a data type that lacks the
> ordering operator and the array ordering operator part.  It is right
> to issue an error if these are lacking, like the various stats paths.
> Should we have some regression tests with specific data types for
> these errors, though?  The stats paths don't care much about these
> error cases, but it does not mean that we should not care about them.
> In short, let's have negative test coverage if we can.
>
select distinct oprleft::regtype from pg_operator where oprname = '='
and oprleft = oprright
except all
select distinct oprleft::regtype from pg_operator where oprname = '<'
and oprleft = oprright;
returns

hstore
cid
aclitem
xid
line

simple tests case using xid data type would be

SELECT array_sort('{{1,2,3}}'::xid[]);


> +typedef struct ArraySortCachedInfo
> +{
> +    TypeCacheEntry *typentry;
> +    TypeCacheEntry *array_typentry;
> +} ArraySortCachedInfo;
>
> Let's put that at the top of the file, with a comment about how it
> links to array_sort() for the caching with fn_extra.  Let's also
> document the meaning of the fields.
>
> FWIW, I am confused by this implementation, where you have to allocate
> the two TypeCacheEntry because of the fact that you have to deal with
> the 1-dimension case and the multi-dimension case.  In the context of
> a single function call, why do you need both typentry and
> array_typentry, actually?  Wouldn't it be enough to use one typentry
> that points to the typcache, meaning that you don't really need to use
> the extra business with fn_mcxt, no?  If you require both (because I
> may be wrong), perhaps you should have a regression test that's able
> to break when removing array_typentry, changing the code to only rely
> on typentry.   Note: I have just removed array_typentry in a quick
> test, current coverage was happy about it.  Feel free to prove me
> wrong.
>

drop table if exists t;
CREATE TABLE t (a int[]);
insert into t values ('{1,3}'),('{1,2,3}'),('{11}');
insert into t values ('{{1,12}}'), ('{{4,3}}');
SELECT array_sort(a) from t;

In the above case,
tuplesort_begin_datum needs the int type information and int[] type information.
otherwise the cached TypeCacheEntry is being used to sort mult-dimension array,
which will make the result false.

  40. Re: general purpose array_sort

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2024-11-04T11:33:51Z

    On Sun, 3 Nov 2024 at 03:33, Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> PFA v11.
>

Testing this with an array with non-default lower bounds, it fails to
preserve the array bounds, which I think it should (note:
array_reverse() and array_shuffle() do preserve the bounds):

SELECT array_reverse(a), array_shuffle(a), array_sort(a)
  FROM (VALUES ('[10:12][20:21]={{1,2},{10,20},{3,4}}'::int[])) v(a);

-[ RECORD 1 ]-+-------------------------------------
array_reverse | [10:12][20:21]={{3,4},{10,20},{1,2}}
array_shuffle | [10:12][20:21]={{10,20},{3,4},{1,2}}
array_sort    | [1:3][20:21]={{1,2},{3,4},{10,20}}

Regards,
Dean

  41. Re: general purpose array_sort

    Michael Paquier <michael@paquier.xyz> — 2024-11-05T01:12:58Z

    On Mon, Nov 04, 2024 at 03:16:35PM +0800, jian he wrote:
> drop table if exists t;
> CREATE TABLE t (a int[]);
> insert into t values ('{1,3}'),('{1,2,3}'),('{11}');
> insert into t values ('{{1,12}}'), ('{{4,3}}');
> SELECT array_sort(a) from t;
> 
> In the above case,
> tuplesort_begin_datum needs the int type information and int[] type information.
> otherwise the cached TypeCacheEntry is being used to sort mult-dimension array,
> which will make the result false.

All these behaviors need more extensive testing.

This brings me an extra question around the caching.  Would the
sorting be able to behave correctly when feeding to a single
array_sort() context array values that have multiple COLLATE clauses?
Or merge_collation_state() would be smart enough to make sure that
collation conflicts never happen to begin with?  I am wondering if we
should worry about multiple VALUES, CTEs, or PL functions where
array_sort() could be fed into its cache values that lead to
unpredictible results for some values.  This stuff should perhaps have
more testing around such behaviors, stressing what kind of
interactions we have between the sorting of multiple values and the
caching, in the context of a single array_sort() call.
--
Michael

  42. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-11-05T07:12:50Z

    On Mon, Nov 4, 2024 at 7:34 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> Testing this with an array with non-default lower bounds, it fails to
> preserve the array bounds, which I think it should (note:
> array_reverse() and array_shuffle() do preserve the bounds):
>
> SELECT array_reverse(a), array_shuffle(a), array_sort(a)
>   FROM (VALUES ('[10:12][20:21]={{1,2},{10,20},{3,4}}'::int[])) v(a);
>
> -[ RECORD 1 ]-+-------------------------------------
> array_reverse | [10:12][20:21]={{3,4},{10,20},{1,2}}
> array_shuffle | [10:12][20:21]={{10,20},{3,4},{1,2}}
> array_sort    | [1:3][20:21]={{1,2},{3,4},{10,20}}
>

if i understand it correctly,
array_create_iterator cannot cope with top dimension bound information.
since input array arguments already have dims, lbs information.
so at the end of array_sort directly copy
from the input array argument to astate.

tuplesort_performsort won't need array bounds, we should be safe?



v12-0001 same as v11-0001-general-purpose-array_sort.patch, only
resolve git conflict
v12-0002 preserve array bound information.
v12-0003 cache ArrayMetaState.

after v12-0003 now
typedef struct ArraySortCachedInfo
{
    TypeCacheEntry *typentry;
    TypeCacheEntry *array_typentry;
    ArrayMetaState array_meta;
} ArraySortCachedInfo;

function array_create_iterator, get_typlenbyvalalign
will do cache search, we can cache ArrayMetaState.
so multiple array_create_iterator calls won't need to call get_typlenbyvalalign.
every time.


0002, I also have a 3 dimensional array test.
create table t(a int[]);
insert into t values ('[-1:-0]={7,1}'::int[]),
('[-2:-0][20:21]={{1,2},{10,20},{1,-4}}'),
('[-2:-0][20:22]={{-11,2,-1},{-11,2, 1},{-11,-4, 10}}'),
('[-13:-10][0:1][20:22]={
{{1,2,112},{1,2,-123}},
{{10,-20,1},{11,123,3}},
{{10,-20,1},{11,-123,-9}},
{{1,2,-11},{1,2,211}}}'::int[]);
SELECT array_sort(t.a) from t;
SELECT array_sort((t.a) [-13:-10][0:1][21:22]) from t where array_ndims(a) = 3;
SELECT array_sort((t.a) [-13:-11][0:1][21:22]) from t where array_ndims(a) = 3;
SELECT array_sort((t.a) [-13:-11][0:0][20:21]) from t where array_ndims(a) = 3;

The test output is ok to me.

  43. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-11-05T12:30:24Z

    Hi jian,

On Tue, Nov 5, 2024 at 3:13 PM jian he <jian.universality@gmail.com> wrote:
>
> On Mon, Nov 4, 2024 at 7:34 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > Testing this with an array with non-default lower bounds, it fails to
> > preserve the array bounds, which I think it should (note:
> > array_reverse() and array_shuffle() do preserve the bounds):
> >
> > SELECT array_reverse(a), array_shuffle(a), array_sort(a)
> >   FROM (VALUES ('[10:12][20:21]={{1,2},{10,20},{3,4}}'::int[])) v(a);
> >
> > -[ RECORD 1 ]-+-------------------------------------
> > array_reverse | [10:12][20:21]={{3,4},{10,20},{1,2}}
> > array_shuffle | [10:12][20:21]={{10,20},{3,4},{1,2}}
> > array_sort    | [1:3][20:21]={{1,2},{3,4},{10,20}}
> >
>
> if i understand it correctly,
> array_create_iterator cannot cope with top dimension bound information.
> since input array arguments already have dims, lbs information.
> so at the end of array_sort directly copy
> from the input array argument to astate.
>
> tuplesort_performsort won't need array bounds, we should be safe?
>
>
>
> v12-0001 same as v11-0001-general-purpose-array_sort.patch, only
> resolve git conflict
> v12-0002 preserve array bound information.
> v12-0003 cache ArrayMetaState.
>
> after v12-0003 now
> typedef struct ArraySortCachedInfo
> {
>     TypeCacheEntry *typentry;
>     TypeCacheEntry *array_typentry;
>     ArrayMetaState array_meta;
> } ArraySortCachedInfo;
>
> function array_create_iterator, get_typlenbyvalalign
> will do cache search, we can cache ArrayMetaState.
> so multiple array_create_iterator calls won't need to call get_typlenbyvalalign.
> every time.
>
>
> 0002, I also have a 3 dimensional array test.
> create table t(a int[]);
> insert into t values ('[-1:-0]={7,1}'::int[]),
> ('[-2:-0][20:21]={{1,2},{10,20},{1,-4}}'),
> ('[-2:-0][20:22]={{-11,2,-1},{-11,2, 1},{-11,-4, 10}}'),
> ('[-13:-10][0:1][20:22]={
> {{1,2,112},{1,2,-123}},
> {{10,-20,1},{11,123,3}},
> {{10,-20,1},{11,-123,-9}},
> {{1,2,-11},{1,2,211}}}'::int[]);
> SELECT array_sort(t.a) from t;
> SELECT array_sort((t.a) [-13:-10][0:1][21:22]) from t where array_ndims(a) = 3;
> SELECT array_sort((t.a) [-13:-11][0:1][21:22]) from t where array_ndims(a) = 3;
> SELECT array_sort((t.a) [-13:-11][0:0][20:21]) from t where array_ndims(a) = 3;
>
> The test output is ok to me.

Thanks for the bounds preserve solution, I just looked at 0002,

+ if (astate->arraystate != NULL)
+ {
+ memcpy(astate->arraystate->dims, dims, ndim * sizeof(int));
+ memcpy(astate->arraystate->lbs, lbs, ndim * sizeof(int));
+ Assert(ndim == astate->arraystate->ndims);
+ }

It seems to me we only need to set astate->arraystate->lbs[0] = lbs[0] ?

-- 
Regards
Junwang Zhao

  44. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-11-06T07:10:23Z

    On Tue, Nov 5, 2024 at 8:30 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
>
> Thanks for the bounds preserve solution, I just looked at 0002,
>
> + if (astate->arraystate != NULL)
> + {
> + memcpy(astate->arraystate->dims, dims, ndim * sizeof(int));
> + memcpy(astate->arraystate->lbs, lbs, ndim * sizeof(int));
> + Assert(ndim == astate->arraystate->ndims);
> + }
>
> It seems to me we only need to set astate->arraystate->lbs[0] = lbs[0] ?
>
yes.

> + memcpy(astate->arraystate->dims, dims, ndim * sizeof(int));
thinking about it, this is wrong. we should just do Assert
        for(int i = 0; i < ndim; i++)
        {
            Assert(astate->arraystate->dims[i] == dims[i]);
        }

or just remove
 memcpy(astate->arraystate->dims, dims, ndim * sizeof(int));

  45. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-11-07T13:56:39Z

    Hi Michael,

On Mon, Nov 4, 2024 at 1:46 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sun, Nov 03, 2024 at 11:33:05AM +0800, Junwang Zhao wrote:
> > Rebase needed due to array_reverse committed, PFA v11.
>
> There has been another conflict since you have posted this version
> (noticed that after my business in 027124a872d7).  I have looked at
> 0001.
>
> +    if (ARR_NDIM(array) < 1)
> +        PG_RETURN_ARRAYTYPE_P(array);
> There is no point in doing a sort if the array has only one element.
> You can add a check based on "ARR_DIMS(array)[0] < 2" to achieve that.

Yeah, this is reasonable but one case I can't be sure:

SELECT array_sort('{{2,3,4}}'::xid[]);

This will return the array as is, but xid doesn't have a LT_OPR, should
I error out in this case? like:

could not identify ordering operator for type xid[]

>
> +    typentry = lookup_type_cache(elmtyp, TYPECACHE_LT_OPR);
> +    if (!OidIsValid(typentry->lt_opr))
> +        ereport(ERROR,
> +                (errcode(ERRCODE_UNDEFINED_FUNCTION),
> +                 errmsg("could not identify ordering operator for type %s",
> +                    format_type_be(elmtyp))));
>
> The patch introduces two error paths based on the fact that ordering
> operators could not be found depending on a data type that lacks the
> ordering operator and the array ordering operator part.  It is right
> to issue an error if these are lacking, like the various stats paths.
> Should we have some regression tests with specific data types for
> these errors, though?  The stats paths don't care much about these
> error cases, but it does not mean that we should not care about them.
> In short, let's have negative test coverage if we can.
>
> +typedef struct ArraySortCachedInfo
> +{
> +    TypeCacheEntry *typentry;
> +    TypeCacheEntry *array_typentry;
> +} ArraySortCachedInfo;
>
> Let's put that at the top of the file, with a comment about how it
> links to array_sort() for the caching with fn_extra.  Let's also
> document the meaning of the fields.

Will fix it in the following patch set.

>
> FWIW, I am confused by this implementation, where you have to allocate
> the two TypeCacheEntry because of the fact that you have to deal with
> the 1-dimension case and the multi-dimension case.  In the context of
> a single function call, why do you need both typentry and
> array_typentry, actually?  Wouldn't it be enough to use one typentry
> that points to the typcache, meaning that you don't really need to use
> the extra business with fn_mcxt, no?  If you require both (because I
> may be wrong), perhaps you should have a regression test that's able
> to break when removing array_typentry, changing the code to only rely
> on typentry.   Note: I have just removed array_typentry in a quick
> test, current coverage was happy about it.  Feel free to prove me
> wrong.
>
> Agreed that the function should be immutable.  The results are fixed
> depending on the input even with the COLLATE clauses appended.
>
> Let's add something when there is only one element in the first
> dimension of the array, say two cases one with an int and one with an
> array of ints like:
> SELECT array_sort('{1}'::int[]);
> SELECT array_sort('{{1}}'::int[]);

Will add.

> --
> Michael



-- 
Regards
Junwang Zhao

  46. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-11-07T14:06:04Z

    On Tue, Nov 5, 2024 at 9:13 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Nov 04, 2024 at 03:16:35PM +0800, jian he wrote:
> > drop table if exists t;
> > CREATE TABLE t (a int[]);
> > insert into t values ('{1,3}'),('{1,2,3}'),('{11}');
> > insert into t values ('{{1,12}}'), ('{{4,3}}');
> > SELECT array_sort(a) from t;
> >
> > In the above case,
> > tuplesort_begin_datum needs the int type information and int[] type information.
> > otherwise the cached TypeCacheEntry is being used to sort mult-dimension array,
> > which will make the result false.
>
> All these behaviors need more extensive testing.
>
> This brings me an extra question around the caching.  Would the
> sorting be able to behave correctly when feeding to a single
> array_sort() context array values that have multiple COLLATE clauses?
> Or merge_collation_state() would be smart enough to make sure that
> collation conflicts never happen to begin with?  I am wondering if we
> should worry about multiple VALUES, CTEs, or PL functions where
> array_sort() could be fed into its cache values that lead to
> unpredictible results for some values.  This stuff should perhaps have
> more testing around such behaviors, stressing what kind of
> interactions we have between the sorting of multiple values and the
> caching, in the context of a single array_sort() call.

I'm afraid this can not be achieved in my current implementation, a simple
case is:

SELECT array_sort('{foo,bar,null,CCC,Abc,bbc}'::text[]);
{Abc,bar,bbc,CCC,foo,NULL}
SELECT array_sort('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C");
{Abc,CCC,bar,bbc,foo,NULL}

SELECT array_sort(a COLLATE "C") FROM (VALUES
('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C"),
('{foo,bar,null,CCC,Abc,bbc}'::text[])) v(a);
{Abc,CCC,bar,bbc,foo,NULL}
{Abc,CCC,bar,bbc,foo,NULL}

Maybe add some documents to specify this?

> --
> Michael



-- 
Regards
Junwang Zhao

  47. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-11-07T14:26:35Z

    Hi jian,

On Tue, Nov 5, 2024 at 3:13 PM jian he <jian.universality@gmail.com> wrote:
>
> On Mon, Nov 4, 2024 at 7:34 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > Testing this with an array with non-default lower bounds, it fails to
> > preserve the array bounds, which I think it should (note:
> > array_reverse() and array_shuffle() do preserve the bounds):
> >
> > SELECT array_reverse(a), array_shuffle(a), array_sort(a)
> >   FROM (VALUES ('[10:12][20:21]={{1,2},{10,20},{3,4}}'::int[])) v(a);
> >
> > -[ RECORD 1 ]-+-------------------------------------
> > array_reverse | [10:12][20:21]={{3,4},{10,20},{1,2}}
> > array_shuffle | [10:12][20:21]={{10,20},{3,4},{1,2}}
> > array_sort    | [1:3][20:21]={{1,2},{3,4},{10,20}}
> >
>
> if i understand it correctly,
> array_create_iterator cannot cope with top dimension bound information.
> since input array arguments already have dims, lbs information.
> so at the end of array_sort directly copy
> from the input array argument to astate.
>
> tuplesort_performsort won't need array bounds, we should be safe?
>
>
>
> v12-0001 same as v11-0001-general-purpose-array_sort.patch, only
> resolve git conflict
> v12-0002 preserve array bound information.
> v12-0003 cache ArrayMetaState.
>
> after v12-0003 now
> typedef struct ArraySortCachedInfo
> {
>     TypeCacheEntry *typentry;
>     TypeCacheEntry *array_typentry;
>     ArrayMetaState array_meta;
> } ArraySortCachedInfo;
>
> function array_create_iterator, get_typlenbyvalalign
> will do cache search, we can cache ArrayMetaState.
> so multiple array_create_iterator calls won't need to call get_typlenbyvalalign.
> every time.
>
>
> 0002, I also have a 3 dimensional array test.
> create table t(a int[]);
> insert into t values ('[-1:-0]={7,1}'::int[]),
> ('[-2:-0][20:21]={{1,2},{10,20},{1,-4}}'),
> ('[-2:-0][20:22]={{-11,2,-1},{-11,2, 1},{-11,-4, 10}}'),
> ('[-13:-10][0:1][20:22]={
> {{1,2,112},{1,2,-123}},
> {{10,-20,1},{11,123,3}},
> {{10,-20,1},{11,-123,-9}},
> {{1,2,-11},{1,2,211}}}'::int[]);
> SELECT array_sort(t.a) from t;
> SELECT array_sort((t.a) [-13:-10][0:1][21:22]) from t where array_ndims(a) = 3;
> SELECT array_sort((t.a) [-13:-11][0:1][21:22]) from t where array_ndims(a) = 3;
> SELECT array_sort((t.a) [-13:-11][0:0][20:21]) from t where array_ndims(a) = 3;
>
> The test output is ok to me.

Merged into V13 with some change and added you as a co-author.
There is one issue left as Micheal pointed out that feeding to a single
array_sort() context array values that have multiple COLLATE clauses,
I have no idea how to resolve it ;(

-- 
Regards
Junwang Zhao

  48. Re: general purpose array_sort

    Robert Haas <robertmhaas@gmail.com> — 2024-11-07T14:29:05Z

    On Thu, Nov 7, 2024 at 8:56 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
> Yeah, this is reasonable but one case I can't be sure:
>
> SELECT array_sort('{{2,3,4}}'::xid[]);
>
> This will return the array as is, but xid doesn't have a LT_OPR, should
> I error out in this case? like:
>
> could not identify ordering operator for type xid[]

Yes, I think that case needs to error out. It seems best to identify
the ordering operator before you decide whether or not you have >1
element.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  49. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-11-07T14:51:51Z

    On Thu, Nov 7, 2024 at 10:29 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Nov 7, 2024 at 8:56 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > Yeah, this is reasonable but one case I can't be sure:
> >
> > SELECT array_sort('{{2,3,4}}'::xid[]);
> >
> > This will return the array as is, but xid doesn't have a LT_OPR, should
> > I error out in this case? like:
> >
> > could not identify ordering operator for type xid[]
>
> Yes, I think that case needs to error out. It seems best to identify
> the ordering operator before you decide whether or not you have >1
> element.

Got it, will do this in the next version.

>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com



-- 
Regards
Junwang Zhao

  50. Re: general purpose array_sort

    Michael Paquier <michael@paquier.xyz> — 2024-11-08T00:01:20Z

    On Thu, Nov 07, 2024 at 09:29:05AM -0500, Robert Haas wrote:
> Yes, I think that case needs to error out. It seems best to identify
> the ordering operator before you decide whether or not you have >1
> element.

+1.
--
Michael

  51. Re: general purpose array_sort

    Michael Paquier <michael@paquier.xyz> — 2024-11-08T00:51:57Z

    On Thu, Nov 07, 2024 at 10:06:04PM +0800, Junwang Zhao wrote:
> I'm afraid this can not be achieved in my current implementation, a simple
> case is:
> 
> SELECT array_sort('{foo,bar,null,CCC,Abc,bbc}'::text[]);
> {Abc,bar,bbc,CCC,foo,NULL}
> SELECT array_sort('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C");
> {Abc,CCC,bar,bbc,foo,NULL}
> 
> SELECT array_sort(a COLLATE "C") FROM (VALUES
> ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C"),
> ('{foo,bar,null,CCC,Abc,bbc}'::text[])) v(a);
> {Abc,CCC,bar,bbc,foo,NULL}
> {Abc,CCC,bar,bbc,foo,NULL}
> 
> Maybe add some documents to specify this?

So, if I use that:
CREATE COLLATION case_sensitive (provider = icu, locale = '');
=# SELECT array_sort('{Abc,CCC,bar,bbc,foo,NULL}'::text[]
         COLLATE "case_sensitive");
        array_sort
----------------------------
 {Abc,bar,bbc,CCC,foo,NULL}
(1 row)
=# SELECT array_sort('{Abc,CCC,bar,bbc,foo,NULL}'::text[]
         COLLATE "C");
        array_sort
----------------------------
 {Abc,CCC,bar,bbc,foo,NULL}
(1 row)

What takes priority is the collation defined with the array_sort,
which is fine:
=# SELECT array_sort(a collate "case_sensitive") FROM
     (VALUES ('{foo,bar,null,CCC,Abc,bbc}'::text[]),
             ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C" )) v(a);
        array_sort
----------------------------
 {Abc,bar,bbc,CCC,foo,NULL}
 {Abc,bar,bbc,CCC,foo,NULL}
(2 rows)
=# SELECT array_sort(a collate "C") FROM
     (VALUES ('{foo,bar,null,CCC,Abc,bbc}'::text[]),
             ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "case_sensitive" )) v(a);
        array_sort
----------------------------
 {Abc,CCC,bar,bbc,foo,NULL}
 {Abc,CCC,bar,bbc,foo,NULL}
(2 rows)

The case where the collation is defined in the set of values is a bit
more troubling to me, as it depends on what the values want to be
applied, still that's OK because the collation applied is the one
coming from the set of values:
=# SELECT array_sort(a) FROM
     (VALUES ('{foo,bar,null,CCC,Abc,bbc}'::text[]),
             ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "case_sensitive" )) v(a);
         array_sort
----------------------------
 {Abc,bar,bbc,CCC,foo,NULL}
 {Abc,bar,bbc,CCC,foo,NULL}
(2 rows)
=# SELECT array_sort(a) FROM
     (VALUES ('{foo,bar,null,CCC,Abc,bbc}'::text[]),
             ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C" )) v(a);
         array_sort
----------------------------
 {Abc,CCC,bar,bbc,foo,NULL}
 {Abc,CCC,bar,bbc,foo,NULL}
(2 rows)

I am wondering if there are more fancy cases where the saved cache
could force a state that would lead to puzzling results, say with
different collations that should be applied.  I'd recommend to
research that more, to reflect that in the docs and to add tests that
show what we should expect in these cases within 0001 because this
new function is mimicking in the context of a function execution
multiple query clauses where restrictions are applied when analyzing
the query, close to the parser.

For example, UNION and UNION ALL require a common collation when
processing a set of expressions related to them, which would be OK.
Perhaps I lack some imagination to be able to break things.
--
Michael

  52. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2024-11-09T14:45:10Z

    Hi Michael,

On Fri, Nov 8, 2024 at 8:52 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Nov 07, 2024 at 10:06:04PM +0800, Junwang Zhao wrote:
> > I'm afraid this can not be achieved in my current implementation, a simple
> > case is:
> >
> > SELECT array_sort('{foo,bar,null,CCC,Abc,bbc}'::text[]);
> > {Abc,bar,bbc,CCC,foo,NULL}
> > SELECT array_sort('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C");
> > {Abc,CCC,bar,bbc,foo,NULL}
> >
> > SELECT array_sort(a COLLATE "C") FROM (VALUES
> > ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C"),
> > ('{foo,bar,null,CCC,Abc,bbc}'::text[])) v(a);
> > {Abc,CCC,bar,bbc,foo,NULL}
> > {Abc,CCC,bar,bbc,foo,NULL}
> >
> > Maybe add some documents to specify this?
>
> So, if I use that:
> CREATE COLLATION case_sensitive (provider = icu, locale = '');
> =# SELECT array_sort('{Abc,CCC,bar,bbc,foo,NULL}'::text[]
>          COLLATE "case_sensitive");
>         array_sort
> ----------------------------
>  {Abc,bar,bbc,CCC,foo,NULL}
> (1 row)
> =# SELECT array_sort('{Abc,CCC,bar,bbc,foo,NULL}'::text[]
>          COLLATE "C");
>         array_sort
> ----------------------------
>  {Abc,CCC,bar,bbc,foo,NULL}
> (1 row)
>
> What takes priority is the collation defined with the array_sort,
> which is fine:
> =# SELECT array_sort(a collate "case_sensitive") FROM
>      (VALUES ('{foo,bar,null,CCC,Abc,bbc}'::text[]),
>              ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C" )) v(a);
>         array_sort
> ----------------------------
>  {Abc,bar,bbc,CCC,foo,NULL}
>  {Abc,bar,bbc,CCC,foo,NULL}
> (2 rows)
> =# SELECT array_sort(a collate "C") FROM
>      (VALUES ('{foo,bar,null,CCC,Abc,bbc}'::text[]),
>              ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "case_sensitive" )) v(a);
>         array_sort
> ----------------------------
>  {Abc,CCC,bar,bbc,foo,NULL}
>  {Abc,CCC,bar,bbc,foo,NULL}
> (2 rows)
>
> The case where the collation is defined in the set of values is a bit
> more troubling to me, as it depends on what the values want to be
> applied, still that's OK because the collation applied is the one
> coming from the set of values:
> =# SELECT array_sort(a) FROM
>      (VALUES ('{foo,bar,null,CCC,Abc,bbc}'::text[]),
>              ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "case_sensitive" )) v(a);
>          array_sort
> ----------------------------
>  {Abc,bar,bbc,CCC,foo,NULL}
>  {Abc,bar,bbc,CCC,foo,NULL}
> (2 rows)
> =# SELECT array_sort(a) FROM
>      (VALUES ('{foo,bar,null,CCC,Abc,bbc}'::text[]),
>              ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C" )) v(a);
>          array_sort
> ----------------------------
>  {Abc,CCC,bar,bbc,foo,NULL}
>  {Abc,CCC,bar,bbc,foo,NULL}
> (2 rows)
>
> I am wondering if there are more fancy cases where the saved cache
> could force a state that would lead to puzzling results, say with
> different collations that should be applied.  I'd recommend to
> research that more, to reflect that in the docs and to add tests that
> show what we should expect in these cases within 0001 because this
> new function is mimicking in the context of a function execution
> multiple query clauses where restrictions are applied when analyzing
> the query, close to the parser.
>
> For example, UNION and UNION ALL require a common collation when
> processing a set of expressions related to them, which would be OK.
> Perhaps I lack some imagination to be able to break things.
> --
> Michael

While trying to come up with more test cases, it comes to me if the
PG_GET_COLLATION() has already done the work to give array_sort
the right collation oid? I did not pass the typentry->typcollation but
PG_GET_COLLATION()  to tuplesort_begin_datum.

I tried:

CREATE COLLATION case_sensitive (provider = icu, locale = '');

create table t1(a int, b text[] COLLATE "C");
create table t2(a int, b text[] COLLATE "case_sensitive");

insert into t1 values (1, '{foo,bar,null,CCC,Abc,bbc}'::text[]);
insert into t2 values (2, '{foo,bar,null,CCC,Abc,bbc}'::text[]);

select array_sort(b) from t1;
select array_sort(b) from t2;

Set breakpoint to see the collation oid, they all give the expected result.

For the following cases:

SELECT array_sort(a) FROM
     (VALUES ('{foo,bar,null,CCC,Abc,bbc}'::text[]),
              ('{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE
"case_sensitive" )) v(a);

WITH t AS (select '{foo,bar,null,CCC,Abc,bbc}'::text[] a UNION ALL
select '{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "case_sensitive"
a) SELECT array_sort(a)  from t;

The collation seems to have been decided in select_common_collation of
the transform phase.

For:

WITH t AS (select '{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "C" a
UNION ALL select
     '{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "case_sensitive"a)
SELECT array_sort(a) from t;
ERROR:  collation mismatch between explicit collations "C" and "case_sensitive"
LINE 2:      '{foo,bar,null,CCC,Abc,bbc}'::text[] COLLATE "case_sens...

merge_collation_state gives out an ERROR since its explicit collation.
But for implicit collation,
select_common_collation sets InvalidOid to context.collation, so the
following works:

select b from t1 UNION ALL select b from t2;

But since the context has the InvalidOid as collation,
PG_GET_COLLATION() in arrary_sort
got InvalidOid, the following errors:

WITH t3 AS (select b from t1 UNION ALL select b from t2) select
array_sort(b) from t3;
ERROR:  could not determine which collation to use for string comparison
HINT:  Use the COLLATE clause to set the collation explicitly.

The error message comes from tuplesort_begin_datum's call stack, we
can do explicit COLLATE to make it work:

WITH t3 AS (select b from t1 UNION ALL select b from t2) select
array_sort(b collate "C") from t3;

Based on the above analysis, I think it's ok to pass PG_GET_COLLATION()
to tuplesort_begin_datum.

PFA v14 with Robert's comment addressed.

-- 
Regards
Junwang Zhao

  53. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-11-15T09:05:00Z

    On Fri, Nov 8, 2024 at 8:52 AM Michael Paquier <michael@paquier.xyz> wrote:
>

> I am wondering if there are more fancy cases where the saved cache
> could force a state that would lead to puzzling results, say with
> different collations that should be applied.  I'd recommend to
> research that more, to reflect that in the docs and to add tests that
> show what we should expect in these cases within 0001 because this
> new function is mimicking in the context of a function execution
> multiple query clauses where restrictions are applied when analyzing
> the query, close to the parser.
>
> For example, UNION and UNION ALL require a common collation when
> processing a set of expressions related to them, which would be OK.
> Perhaps I lack some imagination to be able to break things.
> --


We had 3 error occurrences of
ERROR:  could not determine which collation to use for string comparison
in collate.linux.utf8.out.
one is UNION ALL, another two is do comparison with two text arguments.

here array_sort only takes one argument, there is not that much place
to go wrong?
potential misbehavior would be only about UNION ALL?

UNION ALL for two tables, for collation, we can both implicit;  both
explicit' one implicit,one explicit.
if both explicit, then it will error out quite easily.
if one side is explicit, another side explicitly, then we use
explicitly, which is what we expected.

the trick is that both are implicit.

drop table t1,t2;
create table t1(a int, b text[] COLLATE "C");
create table t2(a int, b text[] COLLATE case_sensitive);
insert into t1 values (1, '{foo,bar,null,CCC,Abc,bbc}'::text[]);
insert into t2 values (2, '{foo,bar,null,CCC,Abc,bbc}'::text[]);

create domain dtxt as text[] collate case_insensitive;
CREATE OR REPLACE FUNCTION mytxt_coll(x text[]) RETURNS dtxt LANGUAGE
plpgsql AS $$
declare
  xx text[] COLLATE case_insensitive;
begin
    xx := x;
    return xx collate case_insensitive;
end
$$;

--these three fail.
select array_sort(b) from (select b from t1 union all select b from t2) sub;
select array_sort(b) from (select mytxt_coll(b) as b from t2 union all
select b from t1 ) sub;
select array_sort(b) from (select b from
mytxt_coll('{foo,bar,null,CCC,Abc,bbc}'::text[] collate
case_insensitive) f(b) union all select b from t1) sub;

-----
select array_sort(b) from (select b from t1 union all select b from
mytxt_coll('{foo,bar,null,CCC,Abc,bbc}'::text[]) f(b)) sub;
select array_sort(b) from (select b from
mytxt_coll('{foo,bar,null,CCC,Abc,bbc}'::text[]) f(b) union all select
b from t1 ) sub;

these two query outputs are the same, which is what we expected per
quote from manual:
<<>>
otherwise, all input expressions must have the same implicit collation
derivation or the default collation.
If any non-default collation is present, that is the result of the
collation combination.
Otherwise, the result is the default collation.
<<>>
https://www.postgresql.org/docs/current/collation.html#COLLATION-CONCEPTS

also we have varstr_sortsupport->check_collation_set to make sure we
have a single valid collation for array_sort.

overall, I think the current implementation works fine.

  54. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2024-12-22T04:28:25Z

    hi.
I did some cosmetic changes:

expand commit message.
remove unnecessary parentheses around errcode, per
https://git.postgresql.org/cgit/postgresql.git/commit/?id=e3a87b4991cc2d00b7a3082abb54c5f12baedfd1
change two errorcode from ERRCODE_UNDEFINED_FUNCTION
to ERRCODE_FEATURE_NOT_SUPPORTED
in func.sgml, some lines are too long, adjust it to several lines.

  55. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2025-03-09T13:41:49Z

    hi.

patch rebased, also did some minor comments tweak.

  56. Re: general purpose array_sort

    jian he <jian.universality@gmail.com> — 2025-03-16T13:49:58Z

    hi.

before commit 4618045bee4a6d3efcb489c319649d8dd9aaa738 ([0])

select array_sort(array(select '1 4'::int2vector union all select '1
2'::int2vector));
        array_sort
--------------------------
 [1:2][0:1]={{1,2},{1,4}}
(1 row)

after

select array_sort(array(select '1 4'::int2vector union all select '1
2'::int2vector));
  array_sort
---------------
 {"1 2","1 4"}
(1 row)

now look closer, the second is the expected result...
I didn't dig deeper why commit 4618045bee made this patch result correct,
but I guess it would be best to include such a test case,
so I've attached a patch.


[0] https://git.postgresql.org/cgit/postgresql.git/commit/?id=4618045bee4a6d3efcb489c319649d8dd9aaa738

  57. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2025-03-17T11:42:38Z

    On Sun, Mar 16, 2025 at 9:50 PM jian he <jian.universality@gmail.com> wrote:
>
> hi.
>
> before commit 4618045bee4a6d3efcb489c319649d8dd9aaa738 ([0])

It seems that patch is treating int2vector and oidvector as scalar
types.

>
> select array_sort(array(select '1 4'::int2vector union all select '1
> 2'::int2vector));
>         array_sort
> --------------------------
>  [1:2][0:1]={{1,2},{1,4}}
> (1 row)
>

This query:
select array(select '1 4'::int2vector union all select '1 2'::int2vector);
get
[1:2][0:1]={{1,4},{1,2}}

So the behavior of array_sort is consistent.

> after
>
> select array_sort(array(select '1 4'::int2vector union all select '1
> 2'::int2vector));
>   array_sort
> ---------------
>  {"1 2","1 4"}
> (1 row)
>
> now look closer, the second is the expected result...
> I didn't dig deeper why commit 4618045bee made this patch result correct,
> but I guess it would be best to include such a test case,
> so I've attached a patch.
>

No objection for the test case, thanks.

>
> [0] https://git.postgresql.org/cgit/postgresql.git/commit/?id=4618045bee4a6d3efcb489c319649d8dd9aaa738



-- 
Regards
Junwang Zhao

  58. Re: general purpose array_sort

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-30T21:58:11Z

    I spent some time looking at the v17 patchset.  There were some pretty
strange things in it --- why were some of the variants of array_sort()
marked as volatile, for example?  But the two things I'd like to
suggest functionality-wise are:

* The second argument of the variants with booleans should be defined
as true=descending, not true=ascending.  It seems a little odd to me
for the default of a boolean option not to be "false".  Also, then
you don't need an inversion between the second and third arguments.
I'm not dead set on this but it just seems a little cleaner.

* I see that the code is set up to detect an unsortable input type
before it takes the fast exit for "no sort required".  I think this
is poor engineering: we ought to make the fast path as fast as
possible.  The can't-sort case is so rare in real-world usage that
I do not think it matters if the error isn't thrown by every possible
call.  Besides which, it is inconsistent anyway: consider
	SELECT array_sort(NULL::xid[]);
which will not error because it will never reach the C code.  Why's
that okay but delivering an answer for "array_sort('{1}'::xid[])"
is not?  I think "throw error only if we must sort and cannot" is
a perfectly fine definition.

At the code level, I didn't like the way that the multiple entry
points were set up.  I think it's generally cleaner code to have
a worker function with plain C call and return coding and make
all the SQL-visible functions be wrappers around that.  Also the
caching mechanism was overcomplicated, in particular because we
do not need a cache lookup to know which sort operators apply to
arrays.

So all that leads me to v18 attached.  (I merged the two patches
into one, didn't see much value in splitting them.)

In v18, it's somewhat annoying that the typcache doesn't cache
the typarray field; we would not need a separate get_array_type()
lookup if it did.  I doubt there is any real reason for that except
that pg_type.typarray didn't exist when the typcache was invented.
So I'm tempted to add it.  But I looked at existing callers of
get_array_type() and none of them are adjacent to typcache lookups,
so only array_sort would be helped immediately.  I left it alone
for the moment; wonder if anyone else has an opinion?

			regards, tom lane

  59. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2025-03-31T11:02:19Z

    Hi Tom,

On Mon, Mar 31, 2025 at 5:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I spent some time looking at the v17 patchset.  There were some pretty
> strange things in it --- why were some of the variants of array_sort()
> marked as volatile, for example?

I think this was due to some copy-paste of the code nearby.

> But the two things I'd like to
> suggest functionality-wise are:
>
> * The second argument of the variants with booleans should be defined
> as true=descending, not true=ascending.  It seems a little odd to me
> for the default of a boolean option not to be "false".  Also, then
> you don't need an inversion between the second and third arguments.
> I'm not dead set on this but it just seems a little cleaner.
>
Agreed.

> * I see that the code is set up to detect an unsortable input type
> before it takes the fast exit for "no sort required".  I think this
> is poor engineering: we ought to make the fast path as fast as
> possible.  The can't-sort case is so rare in real-world usage that
> I do not think it matters if the error isn't thrown by every possible
> call.  Besides which, it is inconsistent anyway: consider
>         SELECT array_sort(NULL::xid[]);
> which will not error because it will never reach the C code.  Why's
> that okay but delivering an answer for "array_sort('{1}'::xid[])"
> is not?  I think "throw error only if we must sort and cannot" is
> a perfectly fine definition.
Agreed.

>
> At the code level, I didn't like the way that the multiple entry
> points were set up.  I think it's generally cleaner code to have
> a worker function with plain C call and return coding and make
> all the SQL-visible functions be wrappers around that.  Also the
> caching mechanism was overcomplicated, in particular because we
> do not need a cache lookup to know which sort operators apply to
> arrays.

Agreed, your refactor made the code cleaner.

>
> So all that leads me to v18 attached.  (I merged the two patches
> into one, didn't see much value in splitting them.)
>
> In v18, it's somewhat annoying that the typcache doesn't cache
> the typarray field; we would not need a separate get_array_type()
> lookup if it did.  I doubt there is any real reason for that except
> that pg_type.typarray didn't exist when the typcache was invented.
> So I'm tempted to add it.  But I looked at existing callers of
> get_array_type() and none of them are adjacent to typcache lookups,
> so only array_sort would be helped immediately.  I left it alone
> for the moment; wonder if anyone else has an opinion?

The need for `elmtyp` and `array_type` here because a column can
have arrays with varying dimensions. Maybe other callers don't share
this behavior?


>
>                         regards, tom lane
>


-- 
Regards
Junwang Zhao

  60. Re: general purpose array_sort

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-31T17:11:58Z

    Junwang Zhao <zhjwpku@gmail.com> writes:
> On Mon, Mar 31, 2025 at 5:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In v18, it's somewhat annoying that the typcache doesn't cache
>> the typarray field; we would not need a separate get_array_type()
>> lookup if it did.  I doubt there is any real reason for that except
>> that pg_type.typarray didn't exist when the typcache was invented.
>> So I'm tempted to add it.  But I looked at existing callers of
>> get_array_type() and none of them are adjacent to typcache lookups,
>> so only array_sort would be helped immediately.  I left it alone
>> for the moment; wonder if anyone else has an opinion?

> The need for `elmtyp` and `array_type` here because a column can
> have arrays with varying dimensions. Maybe other callers don't share
> this behavior?

Maybe.  I think some of what's going on here is that because for a
long time we only had pg_type.typelem and not pg_type.typarray,
code was written to not need to look up the array type if at all
possible.  So there are simply not that many users.  Anyway it
seems really cheap to add this field to the typcache now.

Attached 0001 is the same as v18, and then 0002 is the proposed
addition to typcache.

			regards, tom lane

  61. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2025-04-01T02:27:24Z

    On Tue, Apr 1, 2025 at 1:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Junwang Zhao <zhjwpku@gmail.com> writes:
> > On Mon, Mar 31, 2025 at 5:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> In v18, it's somewhat annoying that the typcache doesn't cache
> >> the typarray field; we would not need a separate get_array_type()
> >> lookup if it did.  I doubt there is any real reason for that except
> >> that pg_type.typarray didn't exist when the typcache was invented.
> >> So I'm tempted to add it.  But I looked at existing callers of
> >> get_array_type() and none of them are adjacent to typcache lookups,
> >> so only array_sort would be helped immediately.  I left it alone
> >> for the moment; wonder if anyone else has an opinion?
>
> > The need for `elmtyp` and `array_type` here because a column can
> > have arrays with varying dimensions. Maybe other callers don't share
> > this behavior?
>
> Maybe.  I think some of what's going on here is that because for a
> long time we only had pg_type.typelem and not pg_type.typarray,
> code was written to not need to look up the array type if at all
> possible.  So there are simply not that many users.  Anyway it
> seems really cheap to add this field to the typcache now.
>
> Attached 0001 is the same as v18, and then 0002 is the proposed
> addition to typcache.

I've applied the patches to master and regression passed.

0002 is neat, I am +1 for this improvement.

>
>                         regards, tom lane
>


-- 
Regards
Junwang Zhao

  62. Re: general purpose array_sort

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-01T22:05:38Z

    Junwang Zhao <zhjwpku@gmail.com> writes:
> On Tue, Apr 1, 2025 at 1:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Attached 0001 is the same as v18, and then 0002 is the proposed
>> addition to typcache.

> I've applied the patches to master and regression passed.
> 0002 is neat, I am +1 for this improvement.

Hearing no further comments, pushed.

			regards, tom lane

  63. Re: general purpose array_sort

    Junwang Zhao <zhjwpku@gmail.com> — 2025-04-01T23:25:12Z

    On Wed, Apr 2, 2025 at 6:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Junwang Zhao <zhjwpku@gmail.com> writes:
> > On Tue, Apr 1, 2025 at 1:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Attached 0001 is the same as v18, and then 0002 is the proposed
> >> addition to typcache.
>
> > I've applied the patches to master and regression passed.
> > 0002 is neat, I am +1 for this improvement.
>
> Hearing no further comments, pushed.
>
>                         regards, tom lane

Thanks for pushing, and I noticed the corresponding CF entry has
been marked as committed, thanks for taking care of it.

-- 
Regards
Junwang Zhao