Re: general purpose array_sort

From: Junwang Zhao <zhjwpku(at)gmail(dot)com>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: jian he <jian(dot)universality(at)gmail(dot)com>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "andreas(at)proxel(dot)se" <andreas(at)proxel(dot)se>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: general purpose array_sort
Date: 2024-10-10 02:30:06
Message-ID: CAEG8a3K8fk8iJo_xhhBxWg_AE1_-qXDCwNURLnH2gUMwcjh=3g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Oct 9, 2024 at 11:46 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> On Wed, Oct 9, 2024 at 10:10 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> >
> > Hi Amit,
> >
> > On Thu, Oct 3, 2024 at 2:22 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > >
> > > Hi Junwang,
> > >
> > > On Wed, Oct 2, 2024 at 11:46 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> > > > On Wed, Oct 2, 2024 at 9:51 AM jian he <jian(dot)universality(at)gmail(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2024-10-10 02:58:19 Re: First draft of PG 17 release notes
Previous Message Richard Guo 2024-10-10 02:23:23 Re: Allow pushdown of HAVING clauses with grouping sets