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

On Fri, Jan 6, 2012 at 06:34, Andrew Dunstan <andrew(at)dunslane(dot)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