On Mon, Sep 28, 2015 at 10:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alex Hunsaker <badalex@gmail.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.
