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

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.