Thread: New compile warnings

I am seeing the following compile warnings in CVS. I am using for perl:
Summary of my perl5 (5.0 patchlevel 5 subversion 3) configuration:

---------------------------------------------------------------------------

gmake[2]: Leaving directory `/usr/var/local/src/gen/pgsql/CURRENT/pgsql/src/test/regress'
gmake[1]: Leaving directory `/usr/var/local/src/gen/pgsql/CURRENT/pgsql/src/test'
plperl.c:948: warning: `ret_hv' might be used uninitialized in this function
plperl.c:949: warning: `ret_av' might be used uninitialized in this function
/usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
ppport.h:564: warning: `sv_2pv_nolen' defined but not used
/usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
ppport.h:564: warning: `sv_2pv_nolen' defined but not used
SPI.c:25: warning: no previous prototype for `XS__elog'
SPI.c:40: warning: no previous prototype for `XS__DEBUG'
SPI.c:55: warning: no previous prototype for `XS__LOG'
SPI.c:70: warning: no previous prototype for `XS__INFO'
SPI.c:85: warning: no previous prototype for `XS__NOTICE'
SPI.c:100: warning: no previous prototype for `XS__WARNING'
SPI.c:115: warning: no previous prototype for `XS__ERROR'
SPI.c:130: warning: no previous prototype for `XS__spi_exec_query'
SPI.c:157: warning: no previous prototype for `boot_SPI'
SPI.c:158: warning: unused variable `items'
/usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
ppport.h:564: warning: `sv_2pv_nolen' defined but not used

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
359-1001+  If your life is a hard drive,     |  13 Roberts Road +  Christ can be your backup.        |  Newtown Square,
Pennsylvania19073
 

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I am seeing the following compile warnings in CVS. I am using for perl:
>     Summary of my perl5 (5.0 patchlevel 5 subversion 3) configuration:

I believe these two:

> plperl.c:948: warning: `ret_hv' might be used uninitialized in this function
> plperl.c:949: warning: `ret_av' might be used uninitialized in this function

indicate an actual bug --- at least, it's far from clear that the code
can't try to use an uninitialized value.  I trust that the authors of
plperl will step up and fix it; I'm not sufficiently clear on what cases
they are trying to support to want to touch it.

The others indicate sloppiness in the C code generated by perl's XS
functionality.  There's nothing we can do about them.  FWIW, less
obsolete versions of Perl generate fewer warnings --- the only one of
these that I see on 5.8.0 and up is

> SPI.c:158: warning: unused variable `items'
        regards, tom lane

Tom Lane wrote:

>Bruce Momjian <pgman@candle.pha.pa.us> writes:
>  
>
>>I am seeing the following compile warnings in CVS. I am using for perl:
>>    Summary of my perl5 (5.0 patchlevel 5 subversion 3) configuration:
>>    
>>
>
>I believe these two:
>
>  
>
>>plperl.c:948: warning: `ret_hv' might be used uninitialized in this function
>>plperl.c:949: warning: `ret_av' might be used uninitialized in this function
>>    
>>
>
>indicate an actual bug --- at least, it's far from clear that the code
>can't try to use an uninitialized value.  I trust that the authors of
>plperl will step up and fix it; I'm not sufficiently clear on what cases
>they are trying to support to want to touch it.
>  
>

I have just mentally walked through the code and I am fairly sure this 
is harmless, i.e. we should only refer to the one of these that has 
actually been set in any of the relevant code paths. I agree it is 
sloppy and will have discussions with one or two fellow perpetrators er 
I mean collaborators about cleaning it up.

>The others indicate sloppiness in the C code generated by perl's XS
>functionality.  There's nothing we can do about them.  FWIW, less
>obsolete versions of Perl generate fewer warnings --- the only one of
>these that I see on 5.8.0 and up is
>
>  
>
>>SPI.c:158: warning: unused variable `items'
>>    
>>


This message  can actually be fixed with the addition of the following 2 
lines to SPI.xs:

*BOOT*:   *items* = 0;  /* avoid '*unused **variable*' warning */


These messages are confusing:

/usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
ppport.h:564: warning: `sv_2pv_nolen' defined but not used
/usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
ppport.h:564: warning: `sv_2pv_nolen' defined but not used

AFAICT sv_2pv_nolen should actually be used (by SPI.c) on Bruce's machine, so it's a bit strange.


cheers


andrew

Andrew Dunstan wrote:

>
>>>   
>>
>
>
> This message  can actually be fixed with the addition of the following 
> 2 lines to SPI.xs:
>
> *BOOT*:
>    *items* = 0;  /* avoid '*unused **variable*' warning */
>
>
>

Argghh!! Mozilla mailer C&P strikes again.

BOOT:   items = 0;  /* avoid 'unused  variable' warning */


cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> I don't currently have the resources to clean this up properly. The
> attached patch tries to make clear in a comment what the code is doing,
> and also initializes these variables to NULL. If someone wants to take a
> stab at cleaning this up they are welcome to - I don't expect to be able
> to for quite a while.

OK, applied.  It compiles warning-free now for me (using Perl 5.8.0)

            regards, tom lane

I discovered a further plperl bug last night. If foo() is a SRF and
therefore returns an arrayref, calling select foo() rather than select *
from foo() causes a segfault because this line passes NULL as the argument:
   tupdesc = CreateTupleDescCopy(rsinfo->expectedDesc);

I am not sure I even know what the behaviour should be, and my available
time to work on a fix is severely contrained ATM.

cheers

andrew

On Mon, Nov 22, 2004 at 06:37:46AM -0600, Andrew Dunstan wrote:
> 
> I discovered a further plperl bug last night. If foo() is a SRF and
> therefore returns an arrayref, calling select foo() rather than select *
> from foo() causes a segfault because this line passes NULL as the argument:
> 
>     tupdesc = CreateTupleDescCopy(rsinfo->expectedDesc);

This appears to happen with SETOF composite types, but not with
SETOF base types nor with a single row of a composite type -- would
that be consistent with your analysis?

I ran the following tests on 8.0.0beta5, which I think includes
Tom's latest changes:


CREATE OR REPLACE FUNCTION perl_set_int() RETURNS SETOF INTEGER AS $$
return [0..5];
$$ LANGUAGE plperl;

SELECT perl_set_int();perl_set_int 
--------------           0           1           2           3           4           5
(6 rows)


CREATE TYPE testrowperl AS (f1 integer, f2 text, f3 text);

CREATE OR REPLACE FUNCTION perl_row() RETURNS testrowperl AS $$   return {f2 => 'hello', f1 => 1, f3 => 'world'};
$$ LANGUAGE plperl;

SELECT perl_row();   perl_row     
-----------------(1,hello,world)
(1 row)


CREATE TYPE testsetperl AS (f1 integer, f2 text, f3 text);

CREATE OR REPLACE FUNCTION perl_set() RETURNS SETOF testsetperl AS $$   return [       { f1 => 1, f2 => 'Hello', f3 =>
'World'},       { f1 => 2, f2 => 'Hello', f3 =>  'PostgreSQL' },       { f1 => 3, f2 => 'Hello', f3 =>  'PL/Perl' }
];
$$  LANGUAGE plperl;

SELECT perl_set();
server closed the connection unexpectedly

-- 
Michael Fuhr
http://www.fuhr.org/~mfuhr/

"Andrew Dunstan" <andrew@dunslane.net> writes:
> I discovered a further plperl bug last night. If foo() is a SRF and
> therefore returns an arrayref, calling select foo() rather than select *
> from foo() causes a segfault because this line passes NULL as the argument:

>     tupdesc = CreateTupleDescCopy(rsinfo->expectedDesc);

> I am not sure I even know what the behaviour should be, and my available
> time to work on a fix is severely contrained ATM.

It should do the same as plpgsql: throw an error about "set-returning
function called in a context that cannot accept a set", or however that
was phrased.  I'll put in a fix.
        regards, tom lane

Michael Fuhr <mike@fuhr.org> writes:
> I ran the following tests on 8.0.0beta5, which I think includes
> Tom's latest changes:

Thanks for the examples.  I extended this into a simple regression test
which I've added to CVS.  If anyone would like to add some test cases
for more interesting stuff (triggers and error handling come to mind),
step right up ...
        regards, tom lane

"Andrew Dunstan" <andrew@dunslane.net> writes:
> I discovered a further plperl bug last night. If foo() is a SRF and
> therefore returns an arrayref, calling select foo() rather than select *
> from foo() causes a segfault because this line passes NULL as the argument:

>     tupdesc = CreateTupleDescCopy(rsinfo->expectedDesc);

While looking into this I discovered another serious shortcoming: the
code was using static variables to process SETOF results, which of
course would break completely in any query involving more than one
plperl SETOF function.  I also realized that the ret_hv code we were
concerned about was actually dead code, because there's a test further
up that insists that the Perl result be an array when the function is
declared to return set.  So I wiped out that whole code path, for a nice
improvement in code size and legibility...
        regards, tom lane

On Mon, Nov 22, 2004 at 03:34:17PM -0500, Tom Lane wrote:
> 
> Thanks for the examples.  I extended this into a simple regression test
> which I've added to CVS.  If anyone would like to add some test cases
> for more interesting stuff (triggers and error handling come to mind),
> step right up ...

Here's another test case: a function that doesn't return what it's
supposed to return.

CREATE TYPE footype AS (x INTEGER, y INTEGER);

CREATE FUNCTION foo_good() RETURNS SETOF footype AS $$
return [   {x => 1, y => 2},   {x => 3, y => 4}
];
$$ LANGUAGE plperl;

SELECT * FROM foo_good();x | y 
---+---1 | 23 | 4
(2 rows)

CREATE FUNCTION foo_bad() RETURNS SETOF footype AS $$
return [   [1, 2],   [3, 4]
];
$$ LANGUAGE plperl;

SELECT * FROM foo_bad();
server closed the connection unexpectedly

-- 
Michael Fuhr
http://www.fuhr.org/~mfuhr/

Michael Fuhr <mike@fuhr.org> writes:
> Here's another test case: a function that doesn't return what it's
> supposed to return.

I was wondering about that --- the code paths that expect an array
seemed to be testing the SV type more carefully than those expecting
a hash did.  Sigh.
        regards, tom lane

Michael Fuhr <mike@fuhr.org> writes:
> Here's another test case: a function that doesn't return what it's
> supposed to return.

Fixed, thanks.
        regards, tom lane

On Mon, Nov 22, 2004 at 03:34:17PM -0500, Tom Lane wrote:
> 
> Thanks for the examples.  I extended this into a simple regression test
> which I've added to CVS.  If anyone would like to add some test cases
> for more interesting stuff (triggers and error handling come to mind),
> step right up ...

How far do you want to go with checking return types?  Some of the
following test cases are approaching "garbage in, garbage out"
territory and I don't know how much effort you want to put into
protecting programmers from themselves.  Some of the cases already
raise errors; for consistency I'm inclined to think they all should.

Tests run against plperl.c 1.63, SPI.xs 1.11.


Test Case 1: scalar expected, non-scalar returned

CREATE FUNCTION test1() RETURNS TEXT AS $$
return ["test"];
$$ LANGUAGE plperl;

SELECT test1();     test1       
------------------ARRAY(0x8427a58)
(1 row)


Test Case 2: list of scalars expected, scalar returned

CREATE FUNCTION test2() RETURNS SETOF TEXT AS $$
return "test";  
$$ LANGUAGE plperl;

SELECT * FROM test2();
ERROR:  plperl: set-returning function must return reference to array


Test Case 3: list of scalars expected, list of non-scalars returned

CREATE FUNCTION test3() RETURNS SETOF TEXT AS $$
return [["test 1"], ["test 2"]];
$$ LANGUAGE plperl;

SELECT * FROM test3();     test3       
------------------ARRAY(0x8424e10)ARRAY(0x8424fcc)
(2 rows)


Test Case 4: hash expected, non-hash returned

CREATE TYPE footype AS (txtval TEXT, intval INTEGER);

CREATE FUNCTION test4() RETURNS footype AS $$
return "test"; 
$$ LANGUAGE plperl;

SELECT test4();
ERROR:  plperl: composite-returning function must return a reference to hash


Test Case 5: hash expected, keys not scalars

CREATE FUNCTION test5() RETURNS footype AS $$
return {["txtval"] => "test", ["intval"] => 42};
$$ LANGUAGE plperl;

SELECT test5();
ERROR:  plperl: invalid attribute "ARRAY(0x8431950)" in hash


Test Case 6: hash expected, value not scalar

CREATE FUNCTION test6() RETURNS footype AS $$
return {txtval => ["test"], intval => 42};
$$ LANGUAGE plperl;

SELECT * FROM test6();     txtval      | intval 
------------------+--------ARRAY(0x8433f9c) |     42
(1 row)


Test Case 7: list of hashes expected, list of non-hashes returned

CREATE FUNCTION test7() RETURNS SETOF footype AS $$
return ["test 1", 42];
$$ LANGUAGE plperl;

SELECT * FROM test7();
ERROR:  plperl: element of result array is not a reference to hash


Test Case 8: list of hashes expected, hash key not scalar

CREATE FUNCTION test8() RETURNS SETOF footype AS $$
return [{["txtval"] => "test 1", ["intval"] => 1}];
$$ LANGUAGE plperl;

SELECT * FROM test8();
ERROR:  plperl: invalid attribute "ARRAY(0x8437b48)" in hash


Test Case 9: list of hashes expected, hash value not scalar

CREATE FUNCTION test9() RETURNS SETOF footype AS $$
return [{txtval => ["test 1"], intval => 42}];  
$$ LANGUAGE plperl;

SELECT * FROM test9();     txtval      | intval 
------------------+--------ARRAY(0x8438d74) |     42
(1 row)


-- 
Michael Fuhr
http://www.fuhr.org/~mfuhr/

Michael Fuhr <mike@fuhr.org> writes:
> How far do you want to go with checking return types?  Some of the
> following test cases are approaching "garbage in, garbage out"
> territory and I don't know how much effort you want to put into
> protecting programmers from themselves.  Some of the cases already
> raise errors; for consistency I'm inclined to think they all should.

I think all these cases are actually OK as-is.

> CREATE FUNCTION test1() RETURNS TEXT AS $$
> return ["test"];
> $$ LANGUAGE plperl;

> SELECT test1();
>       test1       
> ------------------
>  ARRAY(0x8427a58)
> (1 row)

This is exactly what Perl will do if you try to coerce an array to a
scalar:

$ perl -e 'print ["test 1"], "\n"'
ARRAY(0xa03ec28)
$

so I don't think a Perl programmer would find it surprising; if anything
he'd probably complain if we *didn't* do that.

I would add these test cases to the regression test were it not that the
addresses are machine-dependent...
        regards, tom lane

On Tue, Nov 23, 2004 at 11:37:22AM -0500, Tom Lane wrote:

> $ perl -e 'print ["test 1"], "\n"'
> ARRAY(0xa03ec28)
> $
> 
> so I don't think a Perl programmer would find it surprising; if anything
> he'd probably complain if we *didn't* do that.

Understood, which is why I mentioned that such cases might be
considered GIGO and therefore not plperl's responsibility.
Personally I'd like to see an error or warning since the result
is near useless and the construct almost certainly not what
the programmer meant, but I recognize that not everybody would.

> I would add these test cases to the regression test were it not that the
> addresses are machine-dependent...

I haven't looked into how the regression tests work -- can test
output be post-processed before comparision with expected results?
If so, then a filter could normalize patterns like ARRAY(0xa03ec28)
into something that would pass regression tests.

-- 
Michael Fuhr
http://www.fuhr.org/~mfuhr/

On Tue, Nov 23, 2004 at 11:37:22AM -0500, Tom Lane wrote:
> 
> > CREATE FUNCTION test1() RETURNS TEXT AS $$
> > return ["test"];
> > $$ LANGUAGE plperl;
> 
> > SELECT test1();
> >       test1       
> > ------------------
> >  ARRAY(0x8427a58)
> > (1 row)
> 
> This is exactly what Perl will do if you try to coerce an array to a
> scalar:
> 
> $ perl -e 'print ["test 1"], "\n"'
> ARRAY(0xa03ec28)
> $

To go a stage further, there's no array-to-scalar coercion happening
there; the [] syntax gives you a reference to an anonymous array, and
a reference to an array is a scalar, even when evaluated in list
context, as Tom's example is. If you wanted to return a list from
a sub in perl you'd just go return("test 1", "test 2").

> so I don't think a Perl programmer would find it surprising; if anything
> he'd probably complain if we *didn't* do that.

Indeed. It would be Perlish to have some magic so that when you called
one PL/Perl function from another you could return an array ref from
the inner one and have it Do What You Mean in the outer one, too.


Richard

Michael Fuhr <mike@fuhr.org> writes:
> On Tue, Nov 23, 2004 at 11:37:22AM -0500, Tom Lane wrote:
>> I would add these test cases to the regression test were it not that the
>> addresses are machine-dependent...

> I haven't looked into how the regression tests work -- can test
> output be post-processed before comparision with expected results?

No, it's just a plain "diff".  If it seemed worth the trouble,
I'd put the hackery right into the SQL:
select perl_func()::text ~ 'ARRAY\\(0x[0-9a-f]+\\)';

but it doesn't really seem worth it.
        regards, tom lane

Richard Poole wrote:

>
>
>Indeed. It would be Perlish to have some magic so that when you called
>one PL/Perl function from another you could return an array ref from
>the inner one and have it Do What You Mean in the outer one, too.
>
>
>
>  
>

There is no way to have one plperl function call another directly - they 
are anonymous and a reference to them is not stored anywhere accessible 
inside the perl interpreter. The only place the reference is stored is 
in a table on the C side of the plperl glue code.

This is an architectural limitation that is not easily overcome.

Back to the original suggestion - I would like to have a lot more magic 
that maps between perl hashrefs and postgres composites, and between 
perl arrayrefs and postgres arrays. A plperl programmer should ideally 
never have to construct or deconstruct the text representation of an 
array or a composite. That will have to be looked at after this release 
- we only just hit the feature freeze cutoff with what we have now, 
which is why a few warts are coming to light.

cheers

andrew

Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > I don't currently have the resources to clean this up properly. The
> > attached patch tries to make clear in a comment what the code is doing,
> > and also initializes these variables to NULL. If someone wants to take a
> > stab at cleaning this up they are welcome to - I don't expect to be able
> > to for quite a while.
>
> OK, applied.  It compiles warning-free now for me (using Perl 5.8.0)

FYI, I still see compile warnings. I assume they are just related to me
using Perl 5.0.5 and will ignore them:

gmake[2]: Leaving directory `/usr/var/local/src/gen/pgsql/CURRENT/pgsql/src/test/regress'
gmake[1]: Leaving directory `/usr/var/local/src/gen/pgsql/CURRENT/pgsql/src/test'
/usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
ppport.h:564: warning: `sv_2pv_nolen' defined but not used
/usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
ppport.h:564: warning: `sv_2pv_nolen' defined but not used
SPI.c:59: warning: no previous prototype for `XS__elog'
SPI.c:78: warning: no previous prototype for `XS__DEBUG'
SPI.c:93: warning: no previous prototype for `XS__LOG'
SPI.c:108: warning: no previous prototype for `XS__INFO'
SPI.c:123: warning: no previous prototype for `XS__NOTICE'
SPI.c:138: warning: no previous prototype for `XS__WARNING'
SPI.c:153: warning: no previous prototype for `XS__ERROR'
SPI.c:168: warning: no previous prototype for `XS__spi_exec_query'
SPI.c:197: warning: no previous prototype for `boot_SPI'
/usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
ppport.h:564: warning: `sv_2pv_nolen' defined but not used

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> FYI, I still see compile warnings. I assume they are just related to me
> using Perl 5.0.5 and will ignore them:

> gmake[2]: Leaving directory `/usr/var/local/src/gen/pgsql/CURRENT/pgsql/src/test/regress'
> gmake[1]: Leaving directory `/usr/var/local/src/gen/pgsql/CURRENT/pgsql/src/test'
> /usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
> ppport.h:564: warning: `sv_2pv_nolen' defined but not used
> /usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
> ppport.h:564: warning: `sv_2pv_nolen' defined but not used
> SPI.c:59: warning: no previous prototype for `XS__elog'
> SPI.c:78: warning: no previous prototype for `XS__DEBUG'
> SPI.c:93: warning: no previous prototype for `XS__LOG'
> SPI.c:108: warning: no previous prototype for `XS__INFO'
> SPI.c:123: warning: no previous prototype for `XS__NOTICE'
> SPI.c:138: warning: no previous prototype for `XS__WARNING'
> SPI.c:153: warning: no previous prototype for `XS__ERROR'
> SPI.c:168: warning: no previous prototype for `XS__spi_exec_query'
> SPI.c:197: warning: no previous prototype for `boot_SPI'
> /usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h:41: warning: `local_patches' defined but not used
> ppport.h:564: warning: `sv_2pv_nolen' defined but not used

Yeah, these all look like sloppy code generation by perlxs to me.
I see none of them on 5.8.* Perls.

            regards, tom lane