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 Dunstanand...@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
--
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 13, 2012 at 16:07, Andrew Dunstan and...@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) ]
--
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 t...@sss.pgh.pa.us wrote:
Alex Hunsaker bada...@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?
*** 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(). The copy is garbage collected
*
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.
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 alvhe...@commandprompt.com 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/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.
On Fri, Jan 6, 2012 at 06:34, Andrew Dunstan and...@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
--
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.
Alex Hunsaker bada...@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
--
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 and...@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;
}
--
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 Dunstanand...@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 -- 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 and...@dunslane.net wrote:
On 01/05/2012 06:31 PM, Alex Hunsaker wrote:
On Thu, Jan 5, 2012 at 16:02, Andrew Dunstanand...@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?
*** 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 EXTERN.h
#include perl.h
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
