Thread: pgsql: Fix breakage from earlier plperl fix.
Fix breakage from earlier plperl fix. Apparently the perl garbage collector was a bit too eager, so here we control when the new SV is garbage collected. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/bd0e74a9ce98c65c94565fb603dcc7b710cd4227 Modified Files -------------- src/pl/plperl/plperl_helpers.h | 23 +++++++++++++++-------- 1 files changed, 15 insertions(+), 8 deletions(-)
On Thu, Jan 5, 2012 at 16:02, Andrew Dunstan <andrew@dunslane.net> wrote:
> Fix breakage from earlier plperl fix.
>
> Apparently the perl garbage collector was a bit too eager, so here
> we control when the new SV is garbage collected.
I know im a little late to the party...
I can't help but think this seems a bit inefficient for the common
case. Would it be worth only copying the sv when its a glob or
readonly? Something like the below? I tested a few more svtypes that
were easy to make (code, regexp) and everything seems peachy.
[ BTW it seems readonly in general is fine, for example elog(ERROR,
1); worked previously as well as elog(ERROR, "1");. Both of those sv's
are readonly. ISTM, at least on my version of perl (5.14.2), globs and
readonly vstrings seem to be the problem children. I think we could
get away with testing if its a glob or vstring. But I don't have time
right now to test all the way back to perl 5.8 and everything
in-between, Ill find it if anyone is interested. ]
--
*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***************
*** 47,53 **** sv2cstr(SV *sv)
{
char *val, *res;
STRLEN len;
- SV *nsv;
/*
* get a utf8 encoded char * out of perl. *note* it may not be valid utf8!
--- 47,52 ----
***************
*** 58,65 **** sv2cstr(SV *sv)
* sv before passing it to SvPVutf8(). The copy is garbage collected
* when we're done with it.
*/
! nsv = newSVsv(sv);
! val = SvPVutf8(nsv, len);
/*
* we use perl's length in the event we had an embedded null byte to ensure
--- 57,68 ----
* sv before passing it to SvPVutf8(). The copy is garbage collected
* when we're done with it.
*/
! if(SvTYPE(sv) == SVt_PVGV || SvREADONLY(sv))
! sv = newSVsv(sv);
! else
! SvREFCNT_inc(sv);
!
! val = SvPVutf8(sv, len);
/*
* we use perl's length in the event we had an embedded null byte to ensure
***************
*** 68,74 **** sv2cstr(SV *sv)
res = utf_u2e(val, len);
/* safe now to garbage collect the new SV */
! SvREFCNT_dec(nsv);
return res;
}
--- 71,77 ----
res = utf_u2e(val, len);
/* safe now to garbage collect the new SV */
! SvREFCNT_dec(sv);
return res;
}
On 01/05/2012 06:31 PM, Alex Hunsaker wrote: > On Thu, Jan 5, 2012 at 16:02, Andrew Dunstan<andrew@dunslane.net> wrote: >> Fix breakage from earlier plperl fix. >> >> Apparently the perl garbage collector was a bit too eager, so here >> we control when the new SV is garbage collected. > I know im a little late to the party... > > I can't help but think this seems a bit inefficient for the common > case. Would it be worth only copying the sv when its a glob or > readonly? Something like the below? I tested a few more svtypes that > were easy to make (code, regexp) and everything seems peachy. I'm not so concerned about elog() use, and anyway there the most common case surely will be passing a readonly string. I'm more concerned about all the other places we call sv2cstr(). "SvTYPE(sv) == SVt_PVGV" is what I was looking for in vain in the perl docs. So, yes, we should probably adjust this one more time, but ideally we need a better test than just SvREADONLY(). If you want to follow up your investigation of exactly when we need a copied SV and see how much you can narrow it down that would be great. cheers andrew
On Thu, Jan 5, 2012 at 16:59, Andrew Dunstan <andrew@dunslane.net> wrote: > > > On 01/05/2012 06:31 PM, Alex Hunsaker wrote: >> >> On Thu, Jan 5, 2012 at 16:02, Andrew Dunstan<andrew@dunslane.net> wrote: >>> >>> Fix breakage from earlier plperl fix. >> I can't help but think this seems a bit inefficient > > So, yes, we should probably adjust this one more time, but ideally we need a > better test than just SvREADONLY(). If you want to follow up your > investigation of exactly when we need a copied SV and see how much you can > narrow it down that would be great. After further digging I found it chokes on any non scalar (IOW any reference). I attached a simple c program that I tested with 5.8.9, 5.10.1, 5.12.4 and 5.14.2 (for those who did not know about it, perlbrew made testing across all those perls relatively painless). PFA that copies if its readonly and its not a scalar. Also I fixed up Tom's complaint about having sv2cstr() inside do_util_elog's PG_TRY block. I didn't bother fixing the ones in plperl.c tho-- some seemed like they would require quite a bit of rejiggering. I didn't bother adding regression tests-- should I have?
Attachment
Alex Hunsaker <badalex@gmail.com> writes:
> PFA that copies if its readonly and its not a scalar. Also I fixed up
> Tom's complaint about having sv2cstr() inside do_util_elog's PG_TRY
> block.
On reflection I don't believe that just moving that call will improve
matters. sv2cstr also contains a palloc, and if that throws an error,
it had better do so within a PG error context not the Perl one.
Making things safer will most likely require refactoring sv2cstr to
keep the Perl and PG operations entirely separate.
regards, tom lane