Message-ID: <e2ee29af-76f5-a2ea-09a9-e03d99ad4b87@iki.fi>
Date: Tue, 15 Aug 2023 17:40:53 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: BUG #17946: LC_MONETARY & DO LANGUAGE plperl - BUG
Content-Language: en-US
To: Joe Conway <mail@joeconway.com>, Tristan Partin <tristan@neon.tech>
Cc: gdo@leader.it, Tom Lane <tgl@sss.pgh.pa.us>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <17946-3e84cb577e9551c3@postgresql.org>
 <1564676.1685043220@sss.pgh.pa.us>
 <555bb26e-c27c-ab65-028f-3fd5b82df9de@iki.fi>
 <CT887GSPI31W.BQCSW35JL67L@gonk>
 <ca35e375-be82-322d-8c81-78127d8fb33f@joeconway.com>
 <5676b739-0854-f924-4b97-466e3504c692@joeconway.com>
 <f1856a5d-3703-66ff-e7c7-01d2489315c1@joeconway.com>
 <665279.1686422574@sss.pgh.pa.us>
 <c23d6ec1-f882-af48-82d9-54e9f3e15809@joeconway.com>
 <574a1afa-5c6e-829c-0fed-a9ff44719f9f@joeconway.com>
 <49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi>
 <41a4898e-1cdb-960d-f17e-d71886407e04@joeconway.com>
 <18c6979a-126a-e530-7cc4-3b6984b1997b@joeconway.com>
 <ac52326b-874b-372f-220c-60b424a9a833@joeconway.com>
 <fb4f7abd-6c66-1a35-feea-115d44e6c02a@iki.fi>
 <839d893b-1681-f6b6-e1ba-685c34c582ca@joeconway.com>
 <933780cc-2265-a47a-9f39-dd9208bd2c64@iki.fi>
 <ec6fa20d-e691-198a-4a13-e761771b9dec@joeconway.com>
 <CTSOE3SS70RW.YE7NMLMQSQSO@gonk>
 <82626186-baba-0b1e-9cfb-53e341b51343@joeconway.com>
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <82626186-baba-0b1e-9cfb-53e341b51343@joeconway.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/e2ee29af-76f5-a2ea-09a9-e03d99ad4b87%40iki.fi>
Precedence: bulk

On 01/08/2023 16:48, Joe Conway wrote:
> Any further comments on the posted patch[1]? I would like to apply/push
> this prior to the beta and minor releases next week.

I'm not sure about the placement of the uselocale() calls. In 
plperl_spi_exec(), for example, I think we should switch to the global 
locale right after the check_spi_usage_allowed() call. Otherwise, if an 
error happens in BeginInternalSubTransaction() or in pg_verifymbstr(), 
the error would be processed with the perl locale. Maybe that's 
harmless, error processing hardly cares about LC_MONETARY, but seems 
wrong in principle.

Hmm, come to think of it, if BeginInternalSubTransaction() throws an 
error, we just jump out of the perl interpreter? That doesn't seem cool. 
But that's not new with this patch.

If I'm reading correctly, compile_plperl_function() calls 
select_perl_context(), which calls plperl_trusted_init(), which calls 
uselocale(). So it leaves locale set to the perl locale. Who sets it back?

How about adding a small wrapper around eval_pl() that sets and unsets 
the locale(), just when we enter the interpreter? It's easier to see 
that we are doing the calls in right places, if we make them as close as 
possible to entering/exiting the interpreter. Are there other functions 
in addition to eval_pl() that need to be called with the perl locale?

> /*
>  * plperl_xact_callback --- cleanup at main-transaction end.
>  */
> static void
> plperl_xact_callback(XactEvent event, void *arg)
> {
> 	/* ensure global locale is the current locale */
> 	if (uselocale((locale_t) 0) != LC_GLOBAL_LOCALE)
> 		perl_locale_obj = uselocale(LC_GLOBAL_LOCALE);
> }

So the assumption is that the if current locale is not LC_GLOBAL_LOCALE, 
then it was the perl locale. Seems true today, but this could confusion 
if anything else calls uselocale(). In particular, if another PL 
implementation copies this, and you use plperl and the other PL at the 
same time, they would get mixed up. I think we need another "bool 
perl_locale_obj_in_use" variable to track explicitly whether the perl 
locale is currently active.

If we are careful to put the uselocale() calls in the right places so 
that we never ereport() while in perl locale, this callback isn't 
needed. Maybe it's still a good idea, though, to be extra sure that 
things get reset to a sane state if something unexpected happens.

If multiple interpreters are used, is the single perl_locale_obj 
variable still enough? Each interpreter can have their own locale I believe.

PS. please pgindent

-- 
Heikki Linnakangas
Neon (https://neon.tech)