Обсуждение: Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
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?
Вложения
On 01/05/2012 10:59 PM, Alex Hunsaker wrote: > 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. > > I didn't bother adding regression tests-- should I have? [redirecting to -hackers] I have several questions. 1. How much are we actually saving here? newSVsv() ought to be pretty cheap, no? I imagine it's pretty heavily used inside the interpreter. 2. Unless I'm insufficiently caffeinated right now, there's something wrong with this logic: ! if (SvREADONLY(sv)&& ! (type != SVt_IV || ! type != SVt_NV || ! type != SVt_PV)) 3. The above is in any case almost certainly insufficient, because in my tests a typeglob didn't trigger SvREADONLY(), butdid cause a crash. And yes, we should possibly add a regression test or two. Of course, we can't use the cause of the original complaint ($^V)in them, though. cheers andrew
Excerpts from Andrew Dunstan's message of vie ene 06 10:34:30 -0300 2012: > And yes, we should possibly add a regression test or two. Of course, we can't use the cause of the original complaint ($^V)in them, though. Why not? You obviously can't need output it verbatim, but you could compare it with a different function that returns $^V stringified by a different mechanism. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On 01/06/2012 10:49 AM, Alvaro Herrera wrote: > Excerpts from Andrew Dunstan's message of vie ene 06 10:34:30 -0300 2012: > >> And yes, we should possibly add a regression test or two. Of course, we can't use the cause of the original complaint($^V) in them, though. > Why not? You obviously can't need output it verbatim, but you could > compare it with a different function that returns $^V stringified by a > different mechanism. > not sure exactly how to in such a way that exercises this code, but feel free to suggest something ;-) cheers andrew
On Fri, Jan 6, 2012 at 06:34, Andrew Dunstan <andrew@dunslane.net> wrote:
>> PFA that copies if its readonly and its not a scalar.
>>
>> I didn't bother adding regression tests-- should I have?
> I have several questions.
>
> 1. How much are we actually saving here? newSVsv() ought to be pretty cheap,
> no? I imagine it's pretty heavily used inside the interpreter.
It will incur an extra copy for every return value from plperl and
every value passed to a spi function (and possibly other areas I
forgot about). Personally I think avoiding yet another copy of the
return value is worth it.
> 2. Unless I'm insufficiently caffeinated right now, there's something wrong
> with this logic:
>
> ! if (SvREADONLY(sv) &&
> ! (type != SVt_IV ||
> ! type != SVt_NV ||
> ! type != SVt_PV))
Oh my... I dunno exactly what I was smoking last night, but its a good
thing I didn't share :-). Uh so my test program was also completely
wrong, Ill have to redo it all. I've narrowed it down to: if ((type == SVt_PVGV || SvREADONLY(sv))) {
if (type != SVt_PV && type != SVt_NV) { sv = newSVsv(sv); } }
One odd thing is we have to copy SVt_IV (plain numbers) as apparently
that is shared with refs (my $str = 's'; my $ref = \$str;).
Given this, #3 and the other unknowns we might run into in the future
I think if ((SvREADONLY(sv) || type == SVt_GVPV) proposed upthread
makes the most sense.
> 3. The above is in any case almost certainly insufficient, because in my
> tests a typeglob didn't trigger SvREADONLY(), but did cause a crash.
Hrm the glob I was testing was *STDIN. It failed to fail in my test
program because I was not testing *STDIN directly but instead had
@test = (*STDIN); Ugh. Playing with it a bit more it seems only
*STDIN, *STDERR and *STDOUT have problems. For example this seems to
work fine for me:
do LANGUAGE plperlu $$ open(my $fh, '>', '/tmp/t.tmp'); elog(NOTICE, $fh) $$;
> And yes, we should possibly add a regression test or two. Of course, we
> can't use the cause of the original complaint ($^V) in them, though.
I poked around the perl source looking for some candidates elog(INFO,
${^TAINT}) works fine on 5.8.9 and 5.14.2. I thought we could get away
with elog(INFO, *STDIN) but 5.8.9 says:
NOTICE:
While other version of perl (5.14) say:
NOTICE: *main::STDIN
On 01/06/2012 02:02 PM, Alex Hunsaker wrote: >> 3. The above is in any case almost certainly insufficient, because in my >> tests a typeglob didn't trigger SvREADONLY(), but did cause a crash. > Hrm the glob I was testing was *STDIN. It failed to fail in my test > program because I was not testing *STDIN directly but instead had > @test = (*STDIN); Ugh. Playing with it a bit more it seems only > *STDIN, *STDERR and *STDOUT have problems. For example this seems to > work fine for me: > do LANGUAGE plperlu $$ open(my $fh, '>', '/tmp/t.tmp'); elog(NOTICE, $fh) $$; > > $fh isn't a typeglob here, AIUI. But it's definitely not just *STDIN and friends. Try: do LANGUAGE plperlu $$ $foo=1; elog(NOTICE, *foo) $$; cheers andrew
Alex Hunsaker <badalex@gmail.com> writes:
> Oh my... I dunno exactly what I was smoking last night, but its a good
> thing I didn't share :-). Uh so my test program was also completely
> wrong, Ill have to redo it all. I've narrowed it down to:
> if ((type == SVt_PVGV || SvREADONLY(sv)))
> {
> if (type != SVt_PV &&
> type != SVt_NV)
> {
> sv = newSVsv(sv);
> }
> }
Has anyone tried looking at the source code for SvPVutf8 to see exactly
what cases it fails on? The fact that there's an explicit croak() call
makes me think it might not be terribly hard to tell.
regards, tom lane
On Fri, Jan 6, 2012 at 14:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alex Hunsaker <badalex@gmail.com> writes:
>> Oh my... I dunno exactly what I was smoking last night, but its a good
>> thing I didn't share :-). Uh so my test program was also completely
>> wrong, Ill have to redo it all. I've narrowed it down to:
>> if ((type == SVt_PVGV || SvREADONLY(sv)))
>> {
>> if (type != SVt_PV &&
>> type != SVt_NV)
>> {
>> sv = newSVsv(sv);
>> }
>> }
>
> Has anyone tried looking at the source code for SvPVutf8 to see exactly
> what cases it fails on? The fact that there's an explicit croak() call
> makes me think it might not be terribly hard to tell.
Well its easy to find the message, its not so easy to trace it back up
:-). It is perl source code after all. It *looks* like its just:
sv.c:
Perl_sv_pvn_force_flags(SV *sv, STRLEN, I32 flags)
{
[ Flags is SV_GMAGIC ]
if (SvREADONLY(sv) && !(flags & SV_MUTABLE_RETURN))
// more or less...
Perl_croak(aTHX_ "Can't coerce readonly %s to string", ref)
if ((SvTYPE(sv) > SVt_PVLV && SvTYPE(sv) != SVt_PVFM)
|| isGV_with_GP(sv))
Perl_croak(aTHX_ "Can't coerce %s to string in %s",
sv_reftype(sv,0),
}
Given that I added this hunk:
+
+ if (SvREADONLY(sv) ||
+ isGV_with_GP(sv) ||
+ (SvTYPE(sv) > SVt_PVLV && SvTYPE(sv) != SVt_PVFM))
+ sv = newSVsv(sv);
+ else
+ /* increase the reference count so we cant just
SvREFCNT_dec() it when
+ * we are done */
+ SvREFCNT_inc(sv);
And viola all of these work (both in 5.14 and 5.8.9, although 5.8.9
gives different notices...)
do language plperl $$ elog(NOTICE, *foo); $$;
NOTICE: *main::foo
CONTEXT: PL/Perl anonymous code block
do language plperl $$ elog(NOTICE, $^V); $$;
NOTICE: v5.14.2
CONTEXT: PL/Perl anonymous code block
do language plperl $$ elog(NOTICE, ${^TAINT}); $$;
NOTICE: 0
CONTEXT: PL/Perl anonymous code block
So I've done that in the attached patch. ${^TAINT} seemed to be the
only case that gave consistent notices in 5.8.9 and up so I added it
to the regression tests.
Util.c/o not depending on plperl_helpers.h was also throwing me for a
loop so I fixed it and SPI.c...
Thoughts?
Вложения
On 01/12/2012 09:28 PM, Alex Hunsaker wrote:
> Util.c/o not depending on plperl_helpers.h was also throwing me for a
> loop so I fixed it and SPI.c... Thoughts?
Basically looks good, but I'm confused by this:
do language plperl $$ elog('NOTICE', ${^TAINT}); $$;
Why is NOTICE quoted here? It's constant that we've been careful to set up.
cheers
andrew
On Fri, Jan 13, 2012 at 16:07, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 01/12/2012 09:28 PM, Alex Hunsaker wrote:
>>
>> Util.c/o not depending on plperl_helpers.h was also throwing me for a loop
>> so I fixed it and SPI.c... Thoughts?
>
>
> Basically looks good, but I'm confused by this:
>
> do language plperl $$ elog('NOTICE', ${^TAINT}); $$;
>
>
>
> Why is NOTICE quoted here? It's constant that we've been careful to set up.
No reason.
[ Err well It was just part of me flailing around while trying to
figure out why elog was still crashing even thought I had the issue
fixed. The real problem was Util.o was not being recompiled due to
Util.c not depending on plperl_helpers.h) ]
On 01/13/2012 07:09 PM, Alex Hunsaker wrote:
> On Fri, Jan 13, 2012 at 16:07, Andrew Dunstan<andrew@dunslane.net> wrote:
>>
>> On 01/12/2012 09:28 PM, Alex Hunsaker wrote:
>>> Util.c/o not depending on plperl_helpers.h was also throwing me for a loop
>>> so I fixed it and SPI.c... Thoughts?
>>
>> Basically looks good, but I'm confused by this:
>>
>> do language plperl $$ elog('NOTICE', ${^TAINT}); $$;
>>
>>
>>
>> Why is NOTICE quoted here? It's constant that we've been careful to set up.
> No reason.
>
> [ Err well It was just part of me flailing around while trying to
> figure out why elog was still crashing even thought I had the issue
> fixed. The real problem was Util.o was not being recompiled due to
> Util.c not depending on plperl_helpers.h) ]
Except that it doesn't work. The above produces no output in the
regression tests.
I've committed it with that changed and result file changed accordingly.
cheers
andrew