Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Fix breakage from earlier plperl fix.
- bd0e74a9ce98 9.2.0 cited

pgsql: Fix breakage from earlier plperl fix.

Andrew Dunstan <andrew@dunslane.net> — 2012-01-05T23:02:48Z

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(-)

Re: pgsql: Fix breakage from earlier plperl fix.

Alex Hunsaker <badalex@gmail.com> — 2012-01-05T23:31:41Z

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;
  }

Re: pgsql: Fix breakage from earlier plperl fix.

Andrew Dunstan <andrew@dunslane.net> — 2012-01-05T23:59:53Z

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

Re: pgsql: Fix breakage from earlier plperl fix.

Alex Hunsaker <badalex@gmail.com> — 2012-01-06T03:59:33Z

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?

Re: pgsql: Fix breakage from earlier plperl fix.

Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-06T07:26:07Z

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

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Andrew Dunstan <andrew@dunslane.net> — 2012-01-06T13:34:30Z

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

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Alvaro Herrera <alvherre@commandprompt.com> — 2012-01-06T15:49:26Z

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

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Andrew Dunstan <andrew@dunslane.net> — 2012-01-06T16:13:40Z


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

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Alex Hunsaker <badalex@gmail.com> — 2012-01-06T19:02:49Z

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

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Andrew Dunstan <andrew@dunslane.net> — 2012-01-06T19:13:41Z


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

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-06T21:05:09Z

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

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Alex Hunsaker <badalex@gmail.com> — 2012-01-13T02:28:22Z

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?

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Andrew Dunstan <andrew@dunslane.net> — 2012-01-13T23:07:43Z

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

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Alex Hunsaker <badalex@gmail.com> — 2012-01-14T00:09:17Z

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) ]

Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.

Andrew Dunstan <andrew@dunslane.net> — 2012-01-15T21:21:53Z


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