Re: tiny step toward threading: reduce dependence on setlocale()

Committed v2-0001.

On Tue, 2024-09-03 at 22:04 -0700, Jeff Davis wrote:

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

For functions that do call pg_newlocale_from_collation() when
lc_collation_is_c() returns false, the behavior is unchanged.

There are only 3 callers which don't follow that pattern:

* spg_text_inner_consistent: gets collation ID from the index, and
text is a collatable type so it will be valid

* match_pattern_prefix: same

* make_greater_string: I changed the order of the tests in
make_greater_string so that if len=0 and collation=0, it won't throw an
error. If len !=0, it goes into varstr_cmp(), which will call
pg_newlocale_from_collation().

> * 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?

I fixed this by replacing the assert with an elog(ERROR, ...), so that
it will consistently show a "cache lookup failed for collation 0"
regardless of whether it's a debug build or not. It's not expected that
the error will be encountered.

Regards,
Jeff Davis