Re: BUG #17946: LC_MONETARY & DO LANGUAGE plperl - BUG - Mailing list pgsql-hackers

On 8/15/23 10:40, Heikki Linnakangas wrote:
> 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.

I guess you could probably argue that we should flip this around, and 
only enter the perl locale when calling into libperl, and exit the perl 
locale every time we reemerge under plperl.c control. That seems pretty 
drastic and potentially messy though.

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

Hmm, true enough I guess.

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

No one does it seems, at least not currently

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

I can see that as a better strategy, but "other functions in addition to 
eval_pv()" (I assume you mean eval_pv rather than eval_pl) is a tricky 
one to answer.

I ran the attached script like so (from cwd src/pl/plperl) like so:
```
symbols-used.sh /lib/x86_64-linux-gnu/libperl.so.5.34 plperl.so
```
and get a fairly long list of exported libperl functions that get linked 
into plperl.so:

```
Matched symbols:
boot_DynaLoader
perl_alloc
Perl_av_extend
Perl_av_fetch
Perl_av_len
Perl_av_push
*Perl_call_list
*Perl_call_pv
*Perl_call_sv
perl_construct
Perl_croak
Perl_croak_nocontext
Perl_croak_sv
Perl_croak_xs_usage
Perl_die
*Perl_eval_pv
Perl_free_tmps
Perl_get_sv
Perl_gv_add_by_type
Perl_gv_stashpv
Perl_hv_clear
Perl_hv_common
Perl_hv_common_key_len
Perl_hv_iterinit
Perl_hv_iternext
Perl_hv_iternext_flags
Perl_hv_iternextsv
Perl_hv_ksplit
Perl_looks_like_number
Perl_markstack_grow
Perl_mg_get
Perl_newRV
Perl_newRV_noinc
Perl_newSV
Perl_newSViv
Perl_newSVpv
Perl_newSVpvn
Perl_newSVpvn_flags
Perl_newSVsv
Perl_newSVsv_flags
Perl_newSV_type
Perl_newSVuv
Perl_newXS
Perl_newXS_flags
*perl_parse
Perl_pop_scope
Perl_push_scope
*perl_run
Perl_save_item
Perl_savetmps
Perl_stack_grow
Perl_sv_2bool
Perl_sv_2bool_flags
Perl_sv_2iv
Perl_sv_2iv_flags
Perl_sv_2mortal
Perl_sv_2pv
Perl_sv_2pvbyte
Perl_sv_2pvbyte_flags
Perl_sv_2pv_flags
Perl_sv_2pvutf8
Perl_sv_2pvutf8_flags
Perl_sv_bless
Perl_sv_free
Perl_sv_free2
Perl_sv_isa
Perl_sv_newmortal
Perl_sv_setiv
Perl_sv_setiv_mg
Perl_sv_setsv
Perl_sv_setsv_flags
Perl_sys_init
Perl_sys_init3
Perl_xs_boot_epilog
Perl_xs_handshake
```

I marked the ones that look like perhaps we should care about in the 
above list with an asterisk:

*Perl_call_list
*Perl_call_pv
*Perl_call_sv
*Perl_eval_pv
*perl_run

but perhaps there are others?

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

Or perhaps don't assume that we want the global locale and swap between 
pg_locale_obj (whatever it is) and perl_locale_obj?

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

I feel more comfortable that we have a "belt and suspenders" method to 
restore the locale that was in use by Postgres before entering perl.

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

So in other words plperl and plperlu both used in the same query? I 
don't see how we could get from one to the other without going through 
the outer "postgres" locale first. Or are you thinking something else?

> PS. please pgindent

ok

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com