On Wed, 2024-08-28 at 18:43 +0200, Andreas Karlsson wrote:
> On 8/15/24 12:55 AM, Jeff Davis wrote:
> > This overlaps a bit with what Peter already proposed here:
> >
> > https://www.postgresql.org/message-id/4f562d84-87f4-44dc-8946-01d6c437936f%40eisentraut.org
> >
> > right?
>
> Maybe I am missing something but not as far as I can see. It is
> related
> but I do not see any overlap.
v2-0002:
Oh, I see, pattern_char_isalpha() only has one caller, and it never
passes a NULL for pg_locale_t, so my complaint doesn't affect your
patch. This is about ready, then.
> > > 0003-Remove-dubious-check-against-default-locale.patch
> > >
> > > This patch removes a check against DEFAULT_COLLATION_OID which I
> > > thought
> > > looked really dubious. Shouldn't this just be a simple check for
> > > if
> > > the
> > > locale is deterministic? Since we know we have a valid locale
> > > that
> > > should be enough, right?
> >
> > Looks good to me. The code was correct in the sense that the
> > default
> > collation is always deterministic, but (a) Peter is working on non-
> > deterministic default collations; and (b) it was a redundant check.
>
> Suspected so.
>
> I have attached a set of rebased patches with an added assert. Maybe
> I
> should start a new thread though.
v2-0001:
* There was a performance regression for repeated lookups, such as a "t
< 'xyz'" predicate. I committed 12d3345c0d (to remember the last
collation lookup), which will prevent that regression.
* This patch may change the handling of collation oid 0, and I'm not
sure whether that was intentional or not. lc_collate_is_c(0) returned
false, whereas pg_newlocale_from_collation(0)->collate_is_c raised
Assert or threw an cache lookup error. I'm not sure if this is an
actual problem, but looking at the callers, they should be more
defensive if they expect a collation oid of 0 to work at all.
* The comment in lc_collate_is_c() said that it returned false with the
idea that the caller would throw a useful error, and I think that
comment has been wrong for a while. If it returns false, the caller is
expected to call pg_newlocale_from_collation(), which would Assert on a
debug build. Should we remove that Assert, too, so that it will
consistently throw a cache lookup failure?
Regards,
Jeff Davis
