Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
On 01/13/2012 07:09 PM, Alex Hunsaker wrote: On Fri, Jan 13, 2012 at 16:07, Andrew Dunstan 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
On Fri, Jan 13, 2012 at 16:07, Andrew Dunstan 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) ] -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
On Fri, Jan 6, 2012 at 14:05, Tom Lane wrote: > Alex Hunsaker 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? *** a/src/pl/plperl/GNUmakefile --- b/src/pl/plperl/GNUmakefile *** *** 72,82 perlchunks.h: $(PERLCHUNKS) all: all-lib ! SPI.c: SPI.xs @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ ! Util.c: Util.xs @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ --- 72,82 all: all-lib ! SPI.c: SPI.xs plperl_helpers.h @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ ! Util.c: Util.xs plperl_helpers.h @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap $(perl_privlibexp)/ExtUtils/typemap $< >$@ *** a/src/pl/plperl/expected/plperl_elog.out --- b/src/pl/plperl/expected/plperl_elog.out *** *** 58,60 select uses_global(); --- 58,62 uses_global worked (1 row) + -- make sure we don't choke on readonly values + do language plperl $$ elog('NOTICE', ${^TAINT}); $$; *** a/src/pl/plperl/plperl_helpers.h --- b/src/pl/plperl/plperl_helpers.h *** *** 47,74 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! * * SvPVutf8() croaks nastily on certain things, like typeglobs and * readonly objects such as $^V. That's a perl bug - it's not supposed to ! * happen. To avoid crashing the backend, we make a copy of the ! * 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 * we error out properly */ ! res = utf_u2e(val, len); /* safe now to garbage collect the new SV */ ! SvREFCNT_dec(nsv); return res; } --- 47,81 { char *val, *res; STRLEN len; /* * get a utf8 encoded char * out of perl. *note* it may not be valid utf8! * * SvPVutf8() croaks nastily on certain things, like typeglobs and * readonly objects such as $^V. That's a perl bug - it's not supposed to ! * happen. To avoid crashing the backend, we make a copy of the sv before ! * passing it to SvPVutf8().
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
Alex Hunsaker 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
On Fri, Jan 6, 2012 at 06:34, Andrew Dunstan 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
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 The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
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(), but did 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
On Thu, Jan 5, 2012 at 16:59, Andrew Dunstan wrote: > > > On 01/05/2012 06:31 PM, Alex Hunsaker wrote: >> >> On Thu, Jan 5, 2012 at 16:02, Andrew Dunstan 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? *** a/src/pl/plperl/Util.xs --- b/src/pl/plperl/Util.xs *** *** 37,47 static void do_util_elog(int level, SV *msg) { MemoryContext oldcontext = CurrentMemoryContext; ! char * volatile cmsg = NULL; PG_TRY(); { - cmsg = sv2cstr(msg); elog(level, "%s", cmsg); pfree(cmsg); } --- 37,46 do_util_elog(int level, SV *msg) { MemoryContext oldcontext = CurrentMemoryContext; ! char * volatile cmsg = sv2cstr(msg); PG_TRY(); { elog(level, "%s", cmsg); pfree(cmsg); } *** a/src/pl/plperl/plperl_helpers.h --- b/src/pl/plperl/plperl_helpers.h *** *** 47,74 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! * ! * SvPVutf8() croaks nastily on certain things, like typeglobs and ! * readonly objects such as $^V. That's a perl bug - it's not supposed to ! * happen. To avoid crashing the backend, we make a copy of the ! * 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 * we error out properly */ ! res = utf_u2e(val, len); /* safe now to garbage collect the new SV */ ! SvREFCNT_dec(nsv); return res; } --- 47,79 { char *val, *res; STRLEN len; ! svtype type = SvTYPE(sv); /* * get a utf8 encoded char * out of perl. *note* it may not be valid utf8! * ! * SvPVutf8() croaks nastily on readonly refs, That's a perl bug - it's not ! * supposed to happen. To avoid crashing the backend, we make a copy of the ! * sv before passing it to SvPVutf8(). */ ! if (SvREADONLY(sv) && ! (type != SVt_IV || ! type != SVt_NV || ! type != SVt_PV)) ! 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 * we error out properly */ ! res = utf_u2e(val, len); /* safe now to garbage collect the new SV */ ! SvREFCNT_dec(sv); return res; } /* * compile with gcc -O2 -ggdb `perl -MExtUtils::Embed -e ccopts -e ldopts` svutf8_ro_test.c */ #include #include int main(void) { char *embed[] = { "", "-e", "0" }; int x; AV *test; PerlInterpreter *perl; perl_construct(perl); perl_parse(perl, NULL, 3, embed, NULL); perl_run(perl); eval_pv("my $scalar = 'string';" "@test = (" "'string', " "$scalar, " "\\$scalar, " "1, " "1.5, " "[], " "{}, " "$^V, ," "v5.0.0, " "sub {}, " "qr//, " "*STDIN, " "bless({}, ''), " ");", 1); test = get_av("test", 0); for(x=0; x<=av_len(test); x++) { char *crap; STRLEN len; SV *sv = *av_fetch(test, x, 0); svtype type = SvTYPE(sv); SvREADONLY_on(sv); if (SvREADONLY(sv) && type != SVt_IV || type != SVt_NV || type != SVt_PV) sv = newSVsv(sv); crap = SvPVutf8(sv, len); } perl_destruct(perl); perl_free(perl); return 0; } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
On 01/05/2012 06:31 PM, Alex Hunsaker wrote: On Thu, Jan 5, 2012 at 16:02, Andrew Dunstan 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
On Thu, Jan 5, 2012 at 16:02, Andrew Dunstan 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; } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers