Re: Re: [COMMITTERS] pgsql: Initialise perl library as documented in perl API.

Log Message:
-----------
Initialise perl library as documented in perl API. Backpatch to release 7.4.

Modified Files:
--------------
pgsql/src/pl/plperl:
plperl.c (r1.146 -> r1.147)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/pl/plperl/plperl.c?r1=1.146&r2=1.147)

adunstan(at)postgresql(dot)org (Andrew Dunstan) writes:
> Log Message:
> -----------
> Initialise perl library as documented in perl API. Backpatch to release 7.4.

Hmm, buildfarm says this doesn't work too well on Unixware :-(

regards, tom lane

Tom Lane wrote:
> adunstan(at)postgresql(dot)org (Andrew Dunstan) writes:
>
>> Log Message:
>> -----------
>> Initialise perl library as documented in perl API. Backpatch to release 7.4.
>>
>
> Hmm, buildfarm says this doesn't work too well on Unixware :-(
>
>
>

That's what we have a buildfarm for ;-) There's a failure on FBSD too
by the look of it. I'll dig some more to see what I can find.

I did test on Windows and Linux.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> That's what we have a buildfarm for ;-) There's a failure on FBSD too
> by the look of it. I'll dig some more to see what I can find.

I see this when building HEAD on Fedora 10:

plperl.c: In function 'plperl_init_interp':
plperl.c:450: warning: null argument where non-null required (argument 3)

Seems like a good hint ...

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> That's what we have a buildfarm for ;-) There's a failure on FBSD too
>> by the look of it. I'll dig some more to see what I can find.
>>
>
> I see this when building HEAD on Fedora 10:
>
> plperl.c: In function 'plperl_init_interp':
> plperl.c:450: warning: null argument where non-null required (argument 3)
>
> Seems like a good hint ...
>
>
>
Yeah. I didn't get that. But the odd thing is that on 5.8 especially it
shouldn't matter.

perl 5.8.8's perl.h has:

#ifndef PERL_SYS_INIT3
# define PERL_SYS_INIT3(argvp,argcp,envp) PERL_SYS_INIT(argvp,argcp)
#endif

and the only place it's defined elsewhere that I can see is for OS2 (for
anyone still running it!). These two Unixware machines have 5.8.8. and
the FBSD machine has 5.8.7. So surely it can't be that, unless I'm
missing something badly.

The unixish.h file has this on 5.8:

# define PERL_SYS_INIT(c,v) MALLOC_CHECK_TAINT2(*c,*v)
PERL_FPU_INIT MALLOC_INIT

I'm guessing the problem is actually somewhere in there.

I could construct a dummy environment to pass to perl to quiet that
warning, or I could even clone the environment - I'm mildly reluctant to
pass the real environment to this, as the perlembed man page blandly
tells us that this macro might mangle what is passed to it (even though
we know on 5.8 it doesn't).

cheers

andrew

Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>
>>> That's what we have a buildfarm for ;-) There's a failure on FBSD
>>> too by the look of it. I'll dig some more to see what I can find.
>>>
>>
>> I see this when building HEAD on Fedora 10:
>>
>> plperl.c: In function 'plperl_init_interp':
>> plperl.c:450: warning: null argument where non-null required
>> (argument 3)
>>
>> Seems like a good hint ...
>>
>>
>>
> Yeah. I didn't get that. But the odd thing is that on 5.8 especially
> it shouldn't matter.
>
> perl 5.8.8's perl.h has:
>
> #ifndef PERL_SYS_INIT3
> # define PERL_SYS_INIT3(argvp,argcp,envp) PERL_SYS_INIT(argvp,argcp)
> #endif
>
>
> and the only place it's defined elsewhere that I can see is for OS2
> (for anyone still running it!). These two Unixware machines have
> 5.8.8. and the FBSD machine has 5.8.7. So surely it can't be that,
> unless I'm missing something badly.
>
> The unixish.h file has this on 5.8:
>
> # define PERL_SYS_INIT(c,v) MALLOC_CHECK_TAINT2(*c,*v)
> PERL_FPU_INIT MALLOC_INIT
>
> I'm guessing the problem is actually somewhere in there.
>
>

[hours of digging later]

On FBSD at least, this is failing in the function Perl_do_taint(). I
can't see anything too terrible in the source for this, but to dig
further I'd have to build another perl with debugging turned on. Anyway,
it turns out that this actually isn't called at all if Perl is
configured to use its own malloc() routines instead of those supplied by
the system, as is the case on Fedora, for example, but not FBSD.

So we have a bit of a dilemma. We know that this initialization is
required by the Perl API, and we know that some platforms fail without
it, and we also know that this fails on some platforms which use the
system's malloc() for perl.

I think we need to float a bug upstream to the perl guys on this, but as
a holding position I suggest that we alter the #ifdef test to avoid
calling PERL_SYS_INIT3() where MYMALLOC is defined. It's ugly, but I
can't think of another simple way around it (and we've done worse things
to accommodate platform weirdness ;-) )

BTW, it's not caused by passing NULL as the third argument of
PERL_SYS_INIT3() - changing that was the first thing I tried.

cheers

andrew

Andrew Dunstan wrote:
> [hours of digging later]
>
> On FBSD at least, this is failing in the function Perl_do_taint(). I
> can't see anything too terrible in the source for this, but to dig
> further I'd have to build another perl with debugging turned on. Anyway,
> it turns out that this actually isn't called at all if Perl is
> configured to use its own malloc() routines instead of those supplied by
> the system, as is the case on Fedora, for example, but not FBSD.
>
> So we have a bit of a dilemma. We know that this initialization is
> required by the Perl API, and we know that some platforms fail without
> it, and we also know that this fails on some platforms which use the
> system's malloc() for perl.
>
> I think we need to float a bug upstream to the perl guys on this, but as
> a holding position I suggest that we alter the #ifdef test to avoid
> calling PERL_SYS_INIT3() where MYMALLOC is defined. It's ugly, but I
> can't think of another simple way around it (and we've done worse things
> to accommodate platform weirdness ;-) )
>
> BTW, it's not caused by passing NULL as the third argument of
> PERL_SYS_INIT3() - changing that was the first thing I tried.

I think with an extensive C comment that is acceptable at this point, at
least until we hear from the perl guys. I would think projects would
have hit this issue too.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I think we need to float a bug upstream to the perl guys on this, but as
> a holding position I suggest that we alter the #ifdef test to avoid
> calling PERL_SYS_INIT3() where MYMALLOC is defined.

+1 on both.

> BTW, it's not caused by passing NULL as the third argument of
> PERL_SYS_INIT3() - changing that was the first thing I tried.

Nonetheless, that compiler warning is pretty ugly; please do something
about it. I concur with your feeling that setting up a dummy empty
environment array is probably the safest thing to do.

regards, tom lane

Andrew Dunstan wrote:
>
> I think we need to float a bug upstream to the perl guys on this, but
> as a holding position I suggest that we alter the #ifdef test to avoid
> calling PERL_SYS_INIT3() where MYMALLOC is defined. It's ugly, but I
> can't think of another simple way around it (and we've done worse
> things to accommodate platform weirdness ;-) )
>
>

It turns out that this doesn't work on Cygwin with its latest Perl
(which has MYMALLOC defined). If we call PERL_SYS_INIT3() it fails in
that call, and if we don't call it it fails later. We haven't noticed it
earlier because my Cygwin buildfarm member's Perl is a couple of years
old. I guess we're lucky that it's not a more critical platform, but
there is no guarantee that this is the only place it will fail, now or
in the future. I will see about following up more energetically with the
Perl people.

The workaround on Cygwin is to downgrade the perl installation to 5.8.8.

cheers

andrew