Skip site navigation (1) Skip section navigation (2)

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

From: Alex Hunsaker <badalex(at)gmail(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
Date: 2012-01-06 19:02:49
Message-ID: CAFaPBrSFdL+sMeaphaDzF1P3j6AwhK=Xt63coOkYDVHS8gf=pg@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-committerspgsql-hackers
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

In response to

Responses

pgsql-hackers by date

Next:From: Andrew DunstanDate: 2012-01-06 19:13:41
Subject: Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
Previous:From: Tom LaneDate: 2012-01-06 18:45:23
Subject: Re: Progress on fast path sorting, btree index creation time

pgsql-committers by date

Next:From: Andrew DunstanDate: 2012-01-06 19:13:41
Subject: Re: [COMMITTERS] pgsql: Fix breakage from earlier plperl fix.
Previous:From: Tom LaneDate: 2012-01-06 18:31:53
Subject: pgsql: Fix typo, pg_types_date.h => pgtypes_date.h.

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group