(moving to hackers)
On 6/12/23 05:13, Heikki Linnakangas wrote:
On 10/06/2023 22:28, Joe Conway wrote:
On 6/10/23 15:07, Joe Conway wrote:
On 6/10/23 14:42, Tom Lane wrote:
Joe Conway <m...@joeconway.com> writes:
5/ The attached fixes the issue for me on pg10 and passes check-world.
Comments?
The call in PGLC_localeconv seems *very* oddly placed. Why not
do that before it does any other locale calls? Otherwise you don't
really have reason to believe you're saving the appropriate
values to restore later.
As far as I can tell it really only affects localeconv(), so I tried to
place it close to those. But I am fine with moving it up.
This version is against pg16 (rather than pg10), moves up that hunk,
mentions localeconv() in the comment as the reason for the call, and
fixes some whitespace sloppiness. I will plan to apply to all supported
branches.
Better?
The man page for uselocale(LC_GLOBAL_LOCALE) says: "The calling thread's
current locale is set to the global locale determined by setlocale(3)."
Does that undo the effect of calling uselocale() previously, so if you
later call setlocale(), the new locale takes effect in the thread too?
Or is it equivalent to "uselocale(LC_ALL, setlocale(NULL))", so that it
sets the thread's locale to the current global locale, but later
setlocale() calls have no effect on it?
setlocale() changes the global locale, but uselocale() changes the
locale that is currently active, as I understand it.
Also note that uselocale man page says "Unlike setlocale(3), uselocale()
does not allow selective replacement of individual locale categories.
To employ a locale that differs in only a few categories from the
current locale, use calls to duplocale(3) and newlocale(3) to obtain a
locale object equivalent to the current locale and modify the desired
categories in that object."
In any case, this still doesn't feel like the right place. We have many
more setlocale() calls. Shouldn't we sprinkle them all with
uselocale(LC_GLOBAL_LOCALE)? cache_locale_time() for example. Or rather,
all the places where we use any functions that depend on the current locale.
How about we replace all setlocale() calls with uselocale()?
I don't see us backpatching something that invasive. It might be the
right thing to do for pg17, or even pg16, but I think that is a
different discussion
Shouldn't we restore the old thread-specific locale after the calls? I'm
not sure why libperl calls uselocale(), but we are now overwriting the
locale that it sets.
That is a good question. Though arguably perl is doing the wrong thing
by not resetting the global locale when it is being used embedded.
We have a few other uselocale() calls in pg_locale.c, and we take
care to restore the old locale in those.
I think as long as we are relying on setlocale rather than uselocale in
general (see above), the global locale is where we want things left.
There are a few uselocale() calls in ecpg, and they are protected by
HAVE_USELOCALE. Interestingly, the calls in pg_locale.c are not, but
they are protected by HAVE_LOCALE_T. Seems a little inconsistent.
Possibly something we should clean up, but I think that is separate from
this fix.
In general I think we have 2 or possibly three distinct things here:
1/ how do we fix the misbehavior reported due to libperl in existing
stable branches
2/ what makes most sense going forward (and does that mean pg16 or pg17)
3/ misc code cleanups
I was mostly trying to concentrate on #1, but 2 & 3 are worthy of
discussion.
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
