return_next for plperl (was Re: call for help)

At 2005-05-21 20:18:50 +0530, ams(at)oryx(dot)com wrote:
>
> > The second issue is where plperl returns a large result set.

I have attached the following seven patches to address this problem:

1. Trivial. Replaces some errant spaces with tabs.

2. Trivial. Fixes the spelling of Jan's name, and gets rid of many
inane, useless, annoying, and often misleading comments. Here's
a sample: "plperl_init_all() - Initialize all".

(I have tried to add some useful comments here and there, and will
continue to do so now and again.)

3. Trivial. Splits up some long lines.

4. Converts SRFs in PL/Perl to use a Tuplestore and SFRM_Materialize
to return the result set, based on the PL/PgSQL model.

There are two major consequences: result sets will spill to disk when
they can no longer fit in work_mem; and "select foo_srf()" no longer
works. (I didn't lose sleep over the latter, since that form is not
valid in PL/PgSQL, and it's not documented in PL/Perl.)

5. Trivial, but important. Fixes use of "undef" instead of undef. This
would cause empty functions to fail in bizarre ways. I suspect that
there's still another (old) bug here. I'll investigate further.

6. Moves the majority of (4) out into a new plperl_return_next()
function, to make it possible to expose the functionality to
Perl; cleans up some of the code besides.

7. Add an spi_return_next function for use in Perl code.

If you want to apply the patches and try them out, 8-composite.diff is
what you should use. (Note: my patches depend upon Andrew's use-strict
and %_SHARED patches being applied.)

Here's something to try:

create or replace function foo() returns setof record as $$
$i = 0;
for ("World", "PostgreSQL", "PL/Perl") {
spi_return_next({f1=>++$i, f2=>'Hello', f3=>$_});
}
return;
$$ language plperl;
select * from foo() as (f1 integer, f2 text, f3 text);

(Many thanks to Andrews Dunstan and Supernews for their help.)

Questions, comments, and suggestions welcome.

-- ams
Just Another Cut-and-Paste Hacker

On Sun, 2005-05-22 at 21:25 +0530, Abhijit Menon-Sen wrote:
> I have attached the following seven patches to address this problem:

Does anyone with the skills to review this (i.e. someone other than me)
have any comments on this patch?

Otherwise I'll apply it in a day or two.

-Neil

Neil Conway said:
> On Sun, 2005-05-22 at 21:25 +0530, Abhijit Menon-Sen wrote:
>> I have attached the following seven patches to address this problem:
>
> Does anyone with the skills to review this (i.e. someone other than me)
> have any comments on this patch?
>
> Otherwise I'll apply it in a day or two.
>

From recollection, my comments were these:

1. the exposed function should just be called return_next, not
spi_return_next2. either SPI.xs and friends should be renamed or all the non SPI code
should be moved into another .xs file.
3. regression sets need fixing, I think.
4. according to pg practice it should be done as context diffs, or perhaps
as one context diff ;-)

The patches appear to incorporate my 'use strict' enabling patch - that's a
good thing IMNSHO ;-) We can handle the custom var auth setting question
raised in my original submission of the feature later on.

cheers

andrew

cheers

andrew

Mega-patch version applied. Thanks.

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

Abhijit Menon-Sen wrote:
> At 2005-05-21 20:18:50 +0530, ams(at)oryx(dot)com wrote:
> >
> > > The second issue is where plperl returns a large result set.
>
> I have attached the following seven patches to address this problem:
>
> 1. Trivial. Replaces some errant spaces with tabs.
>
> 2. Trivial. Fixes the spelling of Jan's name, and gets rid of many
> inane, useless, annoying, and often misleading comments. Here's
> a sample: "plperl_init_all() - Initialize all".
>
> (I have tried to add some useful comments here and there, and will
> continue to do so now and again.)
>
> 3. Trivial. Splits up some long lines.
>
> 4. Converts SRFs in PL/Perl to use a Tuplestore and SFRM_Materialize
> to return the result set, based on the PL/PgSQL model.
>
> There are two major consequences: result sets will spill to disk when
> they can no longer fit in work_mem; and "select foo_srf()" no longer
> works. (I didn't lose sleep over the latter, since that form is not
> valid in PL/PgSQL, and it's not documented in PL/Perl.)
>
> 5. Trivial, but important. Fixes use of "undef" instead of undef. This
> would cause empty functions to fail in bizarre ways. I suspect that
> there's still another (old) bug here. I'll investigate further.
>
> 6. Moves the majority of (4) out into a new plperl_return_next()
> function, to make it possible to expose the functionality to
> Perl; cleans up some of the code besides.
>
> 7. Add an spi_return_next function for use in Perl code.
>
> If you want to apply the patches and try them out, 8-composite.diff is
> what you should use. (Note: my patches depend upon Andrew's use-strict
> and %_SHARED patches being applied.)
>
> Here's something to try:
>
> create or replace function foo() returns setof record as $$
> $i = 0;
> for ("World", "PostgreSQL", "PL/Perl") {
> spi_return_next({f1=>++$i, f2=>'Hello', f3=>$_});
> }
> return;
> $$ language plperl;
> select * from foo() as (f1 integer, f2 text, f3 text);
>
> (Many thanks to Andrews Dunstan and Supernews for their help.)
>
> Questions, comments, and suggestions welcome.
>
> -- ams
> Just Another Cut-and-Paste Hacker

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: 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

This has broken the regression tests for plperl - see for example

http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=panda&dt=2005-06-04%2021:20:01
That's because, as Abhijit noted in point 4 below, select foo_srf() no
longer works.

At a minimum those calls need to be removed from the regression tests.

See also my later comments in response to Neil's request of a few days ago.
In particular, I would very much like the callback function renamed to a
simple "return_next".

cheers

andrew

Bruce Momjian said:
>
> Mega-patch version applied. Thanks.
>
> ---------------------------------------------------------------------------
>
>
> Abhijit Menon-Sen wrote:
>> At 2005-05-21 20:18:50 +0530, ams(at)oryx(dot)com wrote:
>> >
>> > > The second issue is where plperl returns a large result set.
>>
>> I have attached the following seven patches to address this problem:
>>
>> 1. Trivial. Replaces some errant spaces with tabs.
>>
>> 2. Trivial. Fixes the spelling of Jan's name, and gets rid of many
>> inane, useless, annoying, and often misleading comments. Here's a
>> sample: "plperl_init_all() - Initialize all".
>>
>> (I have tried to add some useful comments here and there, and will
>> continue to do so now and again.)
>>
>> 3. Trivial. Splits up some long lines.
>>
>> 4. Converts SRFs in PL/Perl to use a Tuplestore and SFRM_Materialize
>> to return the result set, based on the PL/PgSQL model.
>>
>> There are two major consequences: result sets will spill to disk
>> when they can no longer fit in work_mem; and "select foo_srf()" no
>> longer works. (I didn't lose sleep over the latter, since that form
>> is not valid in PL/PgSQL, and it's not documented in PL/Perl.)
>>
>> 5. Trivial, but important. Fixes use of "undef" instead of undef. This
>> would cause empty functions to fail in bizarre ways. I suspect that
>> there's still another (old) bug here. I'll investigate further.
>>
>> 6. Moves the majority of (4) out into a new plperl_return_next()
>> function, to make it possible to expose the functionality to
>> Perl; cleans up some of the code besides.
>>
>> 7. Add an spi_return_next function for use in Perl code.
>>
>> If you want to apply the patches and try them out, 8-composite.diff is
>> what you should use. (Note: my patches depend upon Andrew's use-strict
>> and %_SHARED patches being applied.)
>>
>> Here's something to try:
>>
>> create or replace function foo() returns setof record as $$
>> $i = 0;
>> for ("World", "PostgreSQL", "PL/Perl") {
>> spi_return_next({f1=>++$i, f2=>'Hello', f3=>$_});
>> }
>> return;
>> $$ language plperl;
>> select * from foo() as (f1 integer, f2 text, f3 text);
>>

Bruce Momjian said:
> Andrew Dunstan wrote:
>>
>>
>> This has broken the regression tests for plperl - see for example
>>
>>
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=panda&dt=2005-06-04%2021:20:01>> That's because, as Abhijit noted in point 4 below, select foo_srf() no
>> longer works.
>>
>> At a minimum those calls need to be removed from the regression tests.
>>
>> See also my later comments in response to Neil's request of a few days
>> ago. In particular, I would very much like the callback function
>> renamed to a simple "return_next".
>
> OK, would you please submit a patch to fix it. Thanks.
>

I will unless someone beats me to it in the next 2 weeks - I am currently
travelling and don't really have development facilities available.

cheers

andrew

Andrew Dunstan wrote:
>
>
> This has broken the regression tests for plperl - see for example
>
> http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=panda&dt=2005-06-04%2021:20:01
> That's because, as Abhijit noted in point 4 below, select foo_srf() no
> longer works.
>
> At a minimum those calls need to be removed from the regression tests.
>
> See also my later comments in response to Neil's request of a few days ago.
> In particular, I would very much like the callback function renamed to a
> simple "return_next".

OK, would you please submit a patch to fix it. Thanks.

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

At 2005-06-04 17:27:10 -0500, andrew(at)dunslane(dot)net wrote:
>
> > OK, would you please submit a patch to fix it. Thanks.
>
> I will unless someone beats me to it in the next 2 weeks

Here's a patch to do the following:

1. Rename spi_return_next to return_next.
2. Add a new test for return_next.
3. Update the expected output.
4. Update the documentation.

-- ams

Patch applied. Thanks.

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

Abhijit Menon-Sen wrote:
> At 2005-06-04 17:27:10 -0500, andrew(at)dunslane(dot)net wrote:
> >
> > > OK, would you please submit a patch to fix it. Thanks.
> >
> > I will unless someone beats me to it in the next 2 weeks
>
> Here's a patch to do the following:
>
> 1. Rename spi_return_next to return_next.
> 2. Add a new test for return_next.
> 3. Update the expected output.
> 4. Update the documentation.
>
> -- ams

Content-Description: 1-rn-testdoc.diff

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org

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