Re: PL/Perl list value return causes segfault

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/

Michael Fuhr wrote:

>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
>
>

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);

On Tue, Jul 12, 2005 at 02:59:37PM -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).

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)

Alvaro Herrera wrote:

>On Tue, Jul 12, 2005 at 02:59:37PM -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).
>>
>>
>
>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?
>
>
>

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

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Michael Fuhr wrote:
>> In the latest HEAD, a PL/Perl function that returns a list value
>> instead of a reference causes a segmentation fault:

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

Applied, thanks.

regards, tom lane

On Tue, Jul 12, 2005 at 03:45:55PM -0400, Andrew Dunstan wrote:
>
>
> Alvaro Herrera wrote:
>
> >On Tue, Jul 12, 2005 at 02:59:37PM -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).
> >
> >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?
>
> 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.

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

Cheers,
D
--
David Fetter david(at)fetter(dot)org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778

Remember to vote!

David Fetter wrote:

>On Tue, Jul 12, 2005 at 03:45:55PM -0400, Andrew Dunstan wrote:
>
>
>>
>>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.
>>
>>
>
>Is this a kind of pilot error that documents could help avert in some
>useful way?
>
>
>
>

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

On Tue, Jul 12, 2005 at 02:59:37PM -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).

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.

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

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:
>
>
> David Fetter wrote:
>
> >On Tue, Jul 12, 2005 at 03:45:55PM -0400, Andrew Dunstan wrote:
> >
> >
> >>
> >>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.
> >>
> >>
> >
> >Is this a kind of pilot error that documents could help avert in some
> >useful way?
> >
> >
> >
> >
>
> 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
>
> ---------------------------(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
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Fri, Jul 29, 2005 at 11:24:37PM -0400, Bruce Momjian wrote:
>
> 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.

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

Cheers,
D
--
David Fetter david(at)fetter(dot)org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778

Remember to vote!

David Fetter wrote:

>*** 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>
>
>
>
>

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

On Sat, Jul 30, 2005 at 09:47:58AM -0400, Andrew Dunstan wrote:
>
>
> David Fetter wrote:
>
> You have rolled 2 problems into one - spi_query+spi_fetchrow does not
> address the issue of returning large data sets.
>
> Suggest instead:

[suggestion]

Revised patch attached. Thanks for catching this :)

Cheers,
D
--
David Fetter david(at)fetter(dot)org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778

Remember to vote!

Patch applied. Thanks.

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

David Fetter wrote:
> On Sat, Jul 30, 2005 at 09:47:58AM -0400, Andrew Dunstan wrote:
> >
> >
> > David Fetter wrote:
> >
> > You have rolled 2 problems into one - spi_query+spi_fetchrow does not
> > address the issue of returning large data sets.
> >
> > Suggest instead:
>
> [suggestion]
>
> Revised patch attached. Thanks for catching this :)
>
> Cheers,
> D
> --
> David Fetter david(at)fetter(dot)org http://fetter.org/
> phone: +1 510 893 6100 mobile: +1 415 235 3778
>
> Remember to vote!

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

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