Re: BUG #13638: Exception texts from plperl has bad encoding

Hello,

just reporting that the attached patch solves my problem. Thanks.

Best Regards
Michal Leinweber

On 2015-09-29 05:51, Alex Hunsaker wrote:

> On Mon, Sep 28, 2015 at 10:28 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> Alex Hunsaker <badalex(at)gmail(dot)com> writes:
>>> Yeah, I was just looking at that. I couldn't find a good way to
>>> support
>>> 5.8. Doing: ...
>>> Works, but screws up the error line numbers.
>>
>> I looked into this and determined that the problem is that the
>> location
>> info doesn't get appended to ERRSV until after Perl has popped the
>> stack.
>
>> I tried this implementation of croak_cstr: ...
>
> Yeah that seems to work great!
>
>> * I don't see mess() in the perlapi man page, so is it okay to use?
>
> Looks like they added it to perlapi in 5.12, given that it exists and
> seems to work in 5.8 and 5.10 I think we are ok to use it. I tested
> your above usage on 5.8, 5.10, 5.12.
>
>> * Is there any leakage involved in this? (I don't know what prompted
>> you to add sv_2mortal() to the other implementation, but maybe we need
>> it here too?)
>
> No, $@ is global and mess() also uses a global. (unless perl is
> destructing).
>
> In the croak_sv() case, we never use/assign what cstr2sv() returns
> anywhere in perl land, so it always has a refcount of 1. We have
> similar usage of sv_2mortal(cstr2sv()) albeit not on the same line in
> plperl.c.
>
>> * Is there some other reason why this is wrong or a bad idea? Since
>> we'd
>> only use this with versions lacking croak_sv(), future-proofing
>> doesn't
>> seem like a good argument against it, but maybe there is another one.
>
> The only argument that comes to mind is AFAIK perl 5.8 and 5.10 are
> more or less unmaintained so it might not be worth the effort to make
> them work. And it's been this way forever.
>
>> BTW, I think we can't actually use this regression test case, [...]
>> However,
>
>> other test cases may already provide sufficient coverage if we're not
>> going to test encoding conversion.
>
> Agreed. I'll attach it separately just for easy verification that
> everything is working as intended.
>
>> Also, I'm inclined to leave do_util_elog() alone other than replacing
>> croak("%s", edata->message) with croak_cstr(edata->message).
>
> Done that way.
>
> The attached was tested on 5.8.9, 5.10.1, 5.12.5, 5.14.4, 5.18.2 and
> 5.22.0.