Re: BUG #17946: LC_MONETARY & DO LANGUAGE plperl - BUG

(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 <mail(at)joeconway(dot)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