Re: patch fixing the old RETURN NEXT bug

Hello All,

I'm proposing the fix of this bug:
http://archives.postgresql.org/pgsql-hackers/2005-02/msg00498.php

The exact SQL code exposing the error:

----------

create table usno (ra real, dec real, bmag real, rmag real,ipix int8);
CREATE OR REPLACE FUNCTION xxx(refcursor) RETURNS refcursor AS '
DECLARE query varchar;
BEGIN
query = ''SELECT * FROM usno'';
OPEN $1 FOR EXECUTE query;
RETURN $1;
END;
' LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION yyy() RETURNS SETOF usno AS '
DECLARE rec record;
DECLARE cur refcursor;
BEGIN
cur=xxx(''curs_name'');
LOOP
FETCH cur into rec;
EXIT WHEN NOT FOUND;
RETURN NEXT rec;
END LOOP;
RETURN;
END;
' LANGUAGE plpgsql;

insert into usno values(1,2,3,4);
select * from yyy();
alter table usno add column errbox box;
select * from yyy();
alter table usno drop column errbox;
select * from yyy();

-------

The problem with that is in fact in pl_exec.c in function
compatible_tupdesc(), which do not check for the deleted attributes.

The patch is attached.

Regards,
Sergey

*****************************************************
Sergey E. Koposov
Max Planck Institute for Astronomy
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math(at)sai(dot)msu(dot)ru

"Sergey E. Koposov" <math(at)sai(dot)msu(dot)ru> writes:
> The problem with that is in fact in pl_exec.c in function
> compatible_tupdesc(), which do not check for the deleted attributes.

Is that really the only problem?

regards, tom lane

On Sun, 12 Feb 2006, Tom Lane wrote:

> "Sergey E. Koposov" <math(at)sai(dot)msu(dot)ru> writes:
> > The problem with that is in fact in pl_exec.c in function
> > compatible_tupdesc(), which do not check for the deleted attributes.
>
> Is that really the only problem?

I cannot be completely sure in that, but at least the submitted patch
eliminate the problem with the SQL code from my previous email.

Regards,
Sergey

*****************************************************
Sergey E. Koposov
Max Planck Institute for Astronomy
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math(at)sai(dot)msu(dot)ru

On Sun, 12 Feb 2006, Tom Lane wrote:
> "Sergey E. Koposov" <math(at)sai(dot)msu(dot)ru> writes:
> > The problem with that is in fact in pl_exec.c in function
> > compatible_tupdesc(), which do not check for the deleted attributes.
>
> Is that really the only problem?

Tom,

Are there any problems with that patch to not be applied ?
(Sorry if I'm hurrying)

Regards,
Sergey

*****************************************************
Sergey E. Koposov
Max Planck Institute for Astronomy
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math(at)sai(dot)msu(dot)ru

"Sergey E. Koposov" <math(at)sai(dot)msu(dot)ru> writes:
> Are there any problems with that patch to not be applied ?

Hasn't been reviewed yet ... see nearby discussions about shortage
of patch reviewers ...

> (Sorry if I'm hurrying)

At the moment it's not unusual for nontrivial patches to sit around
for a month or two, unless they happen to attract special interest of
one of the committers.

regards, tom lane

[redirecting to -hackers]

Tom Lane wrote:

>At the moment it's not unusual for nontrivial patches to sit around
>for a month or two, unless they happen to attract special interest of
>one of the committers.
>
>
>
>

Yeah. That's just horrible. It makes people lose interest and can hurt
because of bitrot too. I wish we could aim at some maximum time at least
for a first cut review. If there are areas where only one or two people
have sufficient expertise, that's also a worry.

cheers

andrew

On Sun, 2006-02-12 at 20:15 +0300, Sergey E. Koposov wrote:
> I'm proposing the fix of this bug:
> http://archives.postgresql.org/pgsql-hackers/2005-02/msg00498.php

I think the suggested logic for compatible_tupdesc() is still wrong. For
example, the patch rejects the following:

create table usno (ra real, dec real, bmag real, rmag real, ipix int8);
create function ret_next_check() returns setof usno as $$
declare
r record;
begin
for r in select * from usno loop
return next r;
end loop;
return;
end;
$$ language plpgsql;

insert into usno values (1.0, 2.0, 3.0, 4.0, 5);
select * from ret_next_check();
alter table usno drop column ipix;
select * from ret_next_check(); -- fails, should succeed

Also, this patch should include updates to the regression tests.

-Neil

Needs cleaning up.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

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

Neil Conway wrote:
> On Sun, 2006-02-12 at 20:15 +0300, Sergey E. Koposov wrote:
> > I'm proposing the fix of this bug:
> > http://archives.postgresql.org/pgsql-hackers/2005-02/msg00498.php
>
> I think the suggested logic for compatible_tupdesc() is still wrong. For
> example, the patch rejects the following:
>
> create table usno (ra real, dec real, bmag real, rmag real, ipix int8);
> create function ret_next_check() returns setof usno as $$
> declare
> r record;
> begin
> for r in select * from usno loop
> return next r;
> end loop;
> return;
> end;
> $$ language plpgsql;
>
> insert into usno values (1.0, 2.0, 3.0, 4.0, 5);
> select * from ret_next_check();
> alter table usno drop column ipix;
> select * from ret_next_check(); -- fails, should succeed
>
> Also, this patch should include updates to the regression tests.
>
> -Neil
>
>
>
> ---------------------------(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
SRA OSS, Inc. http://www.sraoss.com

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

Added to TODO:

o Fix problems with RETURN NEXT on tables with
dropped/added columns after function creation

http://archives.postgresql.org/pgsql-patches/2006-02/msg00165$

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

Sergey E. Koposov wrote:
> Hello All,
>
> I'm proposing the fix of this bug:
> http://archives.postgresql.org/pgsql-hackers/2005-02/msg00498.php
>
> The exact SQL code exposing the error:
>
> ----------
>
> create table usno (ra real, dec real, bmag real, rmag real,ipix int8);
> CREATE OR REPLACE FUNCTION xxx(refcursor) RETURNS refcursor AS '
> DECLARE query varchar;
> BEGIN
> query = ''SELECT * FROM usno'';
> OPEN $1 FOR EXECUTE query;
> RETURN $1;
> END;
> ' LANGUAGE plpgsql;
>
> CREATE OR REPLACE FUNCTION yyy() RETURNS SETOF usno AS '
> DECLARE rec record;
> DECLARE cur refcursor;
> BEGIN
> cur=xxx(''curs_name'');
> LOOP
> FETCH cur into rec;
> EXIT WHEN NOT FOUND;
> RETURN NEXT rec;
> END LOOP;
> RETURN;
> END;
> ' LANGUAGE plpgsql;
>
> insert into usno values(1,2,3,4);
> select * from yyy();
> alter table usno add column errbox box;
> select * from yyy();
> alter table usno drop column errbox;
> select * from yyy();
>
> -------
>
> The problem with that is in fact in pl_exec.c in function
> compatible_tupdesc(), which do not check for the deleted attributes.
>
> The patch is attached.
>
> Regards,
> Sergey
>
> *****************************************************
> Sergey E. Koposov
> Max Planck Institute for Astronomy
> Web: http://lnfm1.sai.msu.ru/~math
> E-mail: math(at)sai(dot)msu(dot)ru
>

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

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