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)