Subject: PL/Perl list value return causes segfault

From: Michael Fuhr <[email-masked]>
Date: Tue, 12 Jul 2005 07:04:52 -0600

In the latest HEAD, a PL/Perl function that returns a list value
instead of a reference causes a segmentation fault:

CREATE FUNCTION foo() RETURNS integer[] AS $$
return (1, 2, 3, 4);
$$ LANGUAGE plperl;

SELECT foo();
server closed the connection unexpectedly

Here's the stack trace:

#0  0xfed45bcc in plperl_call_handler (fcinfo=0xffbfe230) at plperl.c:1031
#1  0x0010e7d4 in ExecMakeFunctionResult (fcache=0x44af00, econtext=0x44ae58, 
    isNull=0x44b470 "\177~\177\177\177\177\177\177", isDone=0x44b4d8) at execQual.c:1031
#2  0x001122b0 in ExecProject (projInfo=0x44af00, isDone=0x44ae58) at execQual.c:3607

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

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 12 Jul 2005 14:59:37 -0400

>#2  0x001122b0 in ExecProject (projInfo=0x44af00, isDone=0x44ae58) at execQual.c:3607

Patch below fixes the SEGV, and you will see instead:

andrew=# select foo();
ERROR:  array value must start with "{" or dimension information

which might not immediately point you to the source of the error :-( , 
but is certainly better than a SEGV.

Note that all plperl functions are called in scalar context, and it is 
always wrong to return a list (as opposed to a listref). In fact, the 
value received might surprise you even if it worked (it would be the 
value of the last member of the list).

cheers

andrew

Index: plperl.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plperl/plperl.c,v
retrieving revision 1.85
diff -c -r1.85 plperl.c
*** plperl.c    12 Jul 2005 01:16:21 -0000      1.85
--- plperl.c    12 Jul 2005 18:52:54 -0000
***************
*** 1021,1027 ****
          char *val;

!         if (prodesc->fn_retisarray && SvTYPE(SvRV(perlret)) == SVt_PVAV)
          {
              array_ret = plperl_convert_to_pg_array(perlret);
              SvREFCNT_dec(perlret);
--- 1021,1028 ----
          char *val;

!         if (prodesc->fn_retisarray && SvROK(perlret) &&
!                       SvTYPE(SvRV(perlret)) == SVt_PVAV)
          {
              array_ret = plperl_convert_to_pg_array(perlret);
              SvREFCNT_dec(perlret);

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 12 Jul 2005 15:33:04 -0400

> value of the last member of the list).

Hmm, I don't know if it's feasible to do in Perl, but maybe check
whether the function wants to return something in list context and throw
an appropiate error message?

-- 
Alvaro Herrera (<alvherre[a]alvh.no-ip.org>)
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 12 Jul 2005 15:45:55 -0400

>an appropiate error message?

In perl, if there is any ambiguity it is the called function that is 
responsible for checking, not the caller. See "perldoc -f wantarray". 
PLPerl explicitly passed G_SCALAR as a flag on all calls to plperl 
routines. So returning a list is a case of pilot error.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Tue, 12 Jul 2005 16:36:17 -0400

> Patch below fixes the SEGV, and you will see instead:

Applied, thanks.

			regards, tom lane

---

From: David Fetter <[email-masked]>
Date: Tue, 12 Jul 2005 14:01:36 -0700

On Tue, Jul 12, 2005 at 03:45:55PM -0400, Andrew Dunstan wrote:
> >>Note that all plperl functions are called in scalar context, and
> >>it is always wrong to return a list (as opposed to a listref). In
> >>fact, the value received might surprise you even if it worked (it
> >>would be the value of the last member of the list).
> >
> >whether the function wants to return something in list context and
> >throw an appropiate error message?
> 
> responsible for checking, not the caller. See "perldoc -f
> wantarray".  PLPerl explicitly passed G_SCALAR as a flag on all
> calls to plperl routines. So returning a list is a case of pilot
> error.

Is this a kind of pilot error that documents could help avert in some
useful way?

Cheers,
D
-- 
David Fetter [email-masked] http://fetter.org/
phone: +1 510 893 6100   mobile: +1 415 235 3778

Remember to vote!

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 12 Jul 2005 18:41:53 -0400

>Is this a kind of pilot error that documents could help avert in some

Sure. "A plperl function must always return a scalar value.More complex 
structures (arrays, records, and sets) can be returned in the 
appropriate context by returning a reference. A list should never be 
returned."  Salt to taste and insert where appropriate.

cheers

andrew

---

From: Michael Fuhr <[email-masked]>
Date: Tue, 12 Jul 2005 17:59:18 -0600

> value of the last member of the list).

Yeah, I knew that returning a list was contrary to what was expected, but
I wanted to see what would happen.  I wasn't expecting a core dump :-(
Thanks for the patch.

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 29 Jul 2005 23:24:37 -0400 (EDT)

Would someone who knows perl update plperl.sgml and send me a patch?

Also, is this still true in 8.1:

      In the current implementation, if you are fetching or returning
      very large data sets, you should be aware that these will all go
      into memory.

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

Andrew Dunstan wrote:
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
>        choose an index scan if your joining column's datatypes do not
>        match
> 

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

---

From: David Fetter <[email-masked]>
Date: Fri, 29 Jul 2005 22:43:54 -0700

>       very large data sets, you should be aware that these will all go

That's no longer true.  Please find enclosed a new patch :)

Cheers,
D

Remember to vote!
Index: doc/src/sgml/plperl.sgml
===================================================================
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/plperl.sgml,v
retrieving revision 2.42
diff -c -r2.42 plperl.sgml
*** doc/src/sgml/plperl.sgml	13 Jul 2005 02:10:42 -0000	2.42
--- doc/src/sgml/plperl.sgml	30 Jul 2005 05:42:56 -0000
***************
*** 46,52 ****
    <para>
     To create a function in the PL/Perl language, use the standard
     <xref linkend="sql-createfunction" endterm="sql-createfunction-title">
!    syntax:
  <programlisting>
  CREATE FUNCTION <replaceable>funcname</replaceable> (<replaceable>argument-types</replaceable>) RETURNS <replaceable>return-type</replaceable> AS $$
      # PL/Perl function body
--- 46,57 ----
    <para>
     To create a function in the PL/Perl language, use the standard
     <xref linkend="sql-createfunction" endterm="sql-createfunction-title">
!    syntax.  A PL/Perl function must always return a scalar value.  You
!    can return more complex structures (arrays, records, and sets) 
!    in the appropriate context by returning a reference.
!    Never return a list.  Here follows an example of a PL/Perl
!    function.
! 
  <programlisting>
  CREATE FUNCTION <replaceable>funcname</replaceable> (<replaceable>argument-types</replaceable>) RETURNS <replaceable>return-type</replaceable> AS $$
      # PL/Perl function body
***************
*** 282,288 ****
    </para>
  
    <para>
!    PL/Perl provides two additional Perl commands:
  
     <variablelist>
      <varlistentry>
--- 287,293 ----
    </para>
  
    <para>
!    PL/Perl provides three additional Perl commands:
  
     <variablelist>
      <varlistentry>
***************
*** 293,303 ****
  
       <term><literal><function>spi_exec_query</>(<replaceable>query</replaceable> [, <replaceable>max-rows</replaceable>])</literal></term>
       <term><literal><function>spi_exec_query</>(<replaceable>command</replaceable>)</literal></term>
       <listitem>
        <para>
!        Executes an SQL command.  Here is an example of a query
!        (<command>SELECT</command> command) with the optional maximum
!        number of rows:
  <programlisting>
  $rv = spi_exec_query('SELECT * FROM my_table', 5);
  </programlisting>
--- 298,315 ----
  
       <term><literal><function>spi_exec_query</>(<replaceable>query</replaceable> [, <replaceable>max-rows</replaceable>])</literal></term>
       <term><literal><function>spi_exec_query</>(<replaceable>command</replaceable>)</literal></term>
+      <term><literal><function>spi_query</>(<replaceable>command</replaceable>)</literal></term>
+      <term><literal><function>spi_fetchrow</>(<replaceable>command</replaceable>)</literal></term>
+ 
       <listitem>
        <para>
!        <literal>spi_exec_query</literal> executes an SQL command and
! returns the entire rowset as a reference to an array of hash
! references.  <emphasis>You should only use this command when you know
! that the result set will be relatively small.</emphasis>  Here is an
! example of a query (<command>SELECT</command> command) with the
! optional maximum number of rows:
! 
  <programlisting>
  $rv = spi_exec_query('SELECT * FROM my_table', 5);
  </programlisting>
***************
*** 345,351 ****
  INSERT INTO test (i, v) VALUES (3, 'third line');
  INSERT INTO test (i, v) VALUES (4, 'immortal');
  
! CREATE FUNCTION test_munge() RETURNS SETOF test AS $$
      my $rv = spi_exec_query('select i, v from test;');
      my $status = $rv-&gt;{status};
      my $nrows = $rv-&gt;{processed};
--- 357,363 ----
  INSERT INTO test (i, v) VALUES (3, 'third line');
  INSERT INTO test (i, v) VALUES (4, 'immortal');
  
! CREATE OR REPLACE FUNCTION test_munge() RETURNS SETOF test AS $$
      my $rv = spi_exec_query('select i, v from test;');
      my $status = $rv-&gt;{status};
      my $nrows = $rv-&gt;{processed};
***************
*** 360,366 ****
  
  SELECT * FROM test_munge();
  </programlisting>
!       </para>
       </listitem>
      </varlistentry>
  
--- 372,416 ----
  
  SELECT * FROM test_munge();
  </programlisting>
!     </para>
!     <para>
!     <literal>spi_query</literal> and <literal>spi_fetchrow</literal>
!     work together as a pair for rowsets which may be large, or for cases
!     where you wish to return rows as they arrive.
!     <literal>spi_fetchrow</literal> works <emphasis>only</emphasis> with
!     <literal>spi_query</literal>. The following example illustrates how
!     you use them together:
! 
! <programlisting>
! CREATE TYPE foo_type AS (the_num INTEGER, the_text TEXT);
! 
! CREATE OR REPLACE FUNCTION lotsa_md5 (INTEGER) RETURNS SETOF foo_type AS $$
!     use Digest::MD5 qw(md5_hex);
!     my $file = '/usr/share/dict/words';
!     my $t = localtime;
!     elog(NOTICE, "opening file $file at $t" );
!     open my $fh, '&lt;', $file # ooh, it's a file access!
!         or elog(ERROR, "Can't open $file for reading: $!");
!     my @words = &lt;$fh&gt;;
!     close $fh;
!     $t = localtime;
!     elog(NOTICE, "closed file $file at $t");
!     chomp(@words);
!     my $row;
!     my $sth = spi_query("SELECT * FROM generate_series(1,$_[0]) AS b(a)");
!     while (defined ($row = spi_fetchrow($sth))) {
!         return_next({
!             the_num =&gt; $row-&gt;{a},
!             the_text =&gt; md5_hex($words[rand @words])
!         });
!     }
!     return;
! $$ LANGUAGE plperlu;
! 
! SELECT * from lotsa_md5(500);
! </programlisting>
!     </para>
! 
       </listitem>
      </varlistentry>
  
***************
*** 716,724 ****
  
      <listitem>
       <para>
!       In the current implementation, if you are fetching or returning
!       very large data sets, you should be aware that these will all go
!       into memory.
       </para>
      </listitem>
     </itemizedlist>
--- 766,776 ----
  
      <listitem>
       <para>
!       If you are fetching or returning very large data sets using
!       <literal>spi_exec_query</literal>, you should be aware that
!       these will all go into memory.  You can avoid this by using
!       <literal>spi_query</literal>/<literal>spi_fetchrow</literal> as
!       illustrated earlier.
       </para>
      </listitem>
     </itemizedlist>

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 30 Jul 2005 09:47:58 -0400

You have rolled 2 problems into one - spi_query+spi_fetchrow does not 
address the issue of returning large data sets.

Suggest instead:

<para>

       If you are fetching very large data sets using
       <literal>spi_exec_query</literal>, you should be aware that
       these will all go into memory.  You can avoid this by using
       <literal>spi_query</literal> and <literal>spi_fetchrow</literal> 
	as illustrated earlier.
</para>
<para>
	A similar problem occurs if a set-returning function passes 
	a large set of rows back to postgres via 
	<literal>return</literal>. You can avoid this 
	problem too by instead using <literal>return_next</literal> for 
	each row returned, as shown previously.
</para>

cheers

andrew

---

From: David Fetter <[email-masked]>
Date: Sat, 30 Jul 2005 17:34:58 -0700

> address the issue of returning large data sets.

[suggestion]

Revised patch attached.  Thanks for catching this :)

Cheers,
D

Remember to vote!
--- doc/src/sgml/plperl.sgml	31 Jul 2005 00:33:00 -0000
  
    <para>
  
     <variablelist>
      <varlistentry>
--- 287,293 ----
    </para>
  
    <para>
  
     <variablelist>
      <varlistentry>
***************
*** 293,303 ****

  

  
--- 372,416 ----

***************
*** 716,725 ****
  
      <listitem>
       <para>
--- 766,785 ----
  
      <listitem>
       <para>
!       If you are fetching very large data sets using
!       <literal>spi_exec_query</literal>, you should be aware that
!       these will all go into memory.  You can avoid this by using
!       <literal>spi_query</literal>/<literal>spi_fetchrow</literal> as
!       illustrated earlier.
       </para>
+      <para>
+         A similar problem occurs if a set-returning function passes a
+         large set of rows back to postgres via <literal>return</literal>. You
+         can avoid this problem too by instead using
+         <literal>return_next</literal> for each row returned, as shown
+         previously.
+      </para>
+ 
      </listitem>
     </itemizedlist>
    </para>

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 12 Aug 2005 17:42:57 -0400 (EDT)

Patch applied.  Thanks.

> Revised patch attached.  Thanks for catching this :)

[ Attachment, skipping... ]

> TIP 1: if posting/reading through Usenet, please send an appropriate
>        subscribe-nomail command to [email-masked] so that your
>        message can get through to the mailing list cleanly