Re: [HACKERS] BUG #4516: FOUND variable does not work after RETURN QUERY

The following bug has been logged online:

Bug reference: 4516
Logged by: Michal szymanski
Email address: szymanskim(at)datera(dot)pl
PostgreSQL version: 8.3
Operating system: Windows
Description: FOUND variable does not work after RETURN QUERY
Details:

This short program display two rows instead one. If I use RETURN NEXT it
works.

CREATE TABLE test_table (
value VARCHAR
);
INSERT INTO test_table VALUES ('a');
INSERT INTO test_table VALUES ('b');

CREATE OR REPLACE FUNCTION test()
RETURNS SETOF test_table AS
$BODY$
DECLARE
BEGIN

RETURN QUERY
SELECT * FROM test_table WHERE value='a';
IF NOT FOUND THEN
RETURN QUERY
SELECT * FROM test_table WHERE value='b';
END IF;

RETURN;
END;
$BODY$
LANGUAGE 'plpgsql' VOLATILE;

select * from test()

"Michal szymanski" <szymanskim(at)datera(dot)pl> writes:
> Description: FOUND variable does not work after RETURN QUERY

The list of statements that set FOUND is here:
http://www.postgresql.org/docs/8.3/static/plpgsql-statements.html#PLPGSQL-STATEMENTS-DIAGNOSTICS

RETURN isn't one of them.

regards, tom lane

Hello

2008/11/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> "Michal szymanski" <szymanskim(at)datera(dot)pl> writes:
>> Description: FOUND variable does not work after RETURN QUERY
>
> The list of statements that set FOUND is here:
> http://www.postgresql.org/docs/8.3/static/plpgsql-statements.html#PLPGSQL-STATEMENTS-DIAGNOSTICS
>
> RETURN isn't one of them.
>
> regards, tom lane
>

It should be enhanced - my initial proposal of return query expected
so return query is last statement, that isn't now. So we could add
this check there.

regards
Pavel Stehule

> --
> Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
>

"Pavel Stehule" <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2008/11/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> RETURN isn't one of them.

> It should be enhanced - my initial proposal of return query expected
> so return query is last statement, that isn't now. So we could add
> this check there.

Well, changing the semantics of an already-released statement carries a
risk of breaking existing apps that aren't expecting it to change FOUND.
So I'd want to see a pretty strong case why this is important --- not
just that it didn't meet someone's didn't-read-the-manual expectation.

regards, tom lane

2008/11/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> "Pavel Stehule" <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2008/11/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> RETURN isn't one of them.
>
>> It should be enhanced - my initial proposal of return query expected
>> so return query is last statement, that isn't now. So we could add
>> this check there.
>
> Well, changing the semantics of an already-released statement carries a
> risk of breaking existing apps that aren't expecting it to change FOUND.
> So I'd want to see a pretty strong case why this is important --- not
> just that it didn't meet someone's didn't-read-the-manual expectation.
>

It's should do some problems, but I belive much less than change of
casting or tsearch2 integration. And actually it's not ortogonal.
Every not dynamic statement change FOUND variable.

regards
Pavel Stehule

> regards, tom lane
>

>>>>> "Pavel" == "Pavel Stehule" <pavel(dot)stehule(at)gmail(dot)com> writes:

>> Well, changing the semantics of an already-released statement
>> carries a risk of breaking existing apps that aren't expecting it
>> to change FOUND. So I'd want to see a pretty strong case why this
>> is important --- not just that it didn't meet someone's
>> didn't-read-the-manual expectation.

Pavel> It's should do some problems, but I belive much less than
Pavel> change of casting or tsearch2 integration. And actually it's
Pavel> not ortogonal. Every not dynamic statement change FOUND
Pavel> variable.

Regardless of what you think of FOUND, a more serious problem is this:

postgres=# create function test(n integer) returns setof integer language plpgsql
as $f$
declare
rc bigint;
begin
return query (select i from generate_series(1,n) i);
get diagnostics rc = row_count;
raise notice 'rc = %',rc;
end;
$f$;
CREATE FUNCTION
postgres=# select test(3);
NOTICE: rc = 0
test
------
1
2
3
(3 rows)

Since GET DIAGNOSTICS is documented as working for every SQL query
executed in the function, rather than for a specific list of
constructs, this is clearly a bug.

--
Andrew (irc:RhodiumToad)

I am sending patch, that adds FOUND and GET DIAGNOSTICS support for
RETURN QUERY statement

Regards
Pavel Stehule

2008/11/10 Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>:
>>>>>> "Pavel" == "Pavel Stehule" <pavel(dot)stehule(at)gmail(dot)com> writes:
>
> >> Well, changing the semantics of an already-released statement
> >> carries a risk of breaking existing apps that aren't expecting it
> >> to change FOUND. So I'd want to see a pretty strong case why this
> >> is important --- not just that it didn't meet someone's
> >> didn't-read-the-manual expectation.
>
> Pavel> It's should do some problems, but I belive much less than
> Pavel> change of casting or tsearch2 integration. And actually it's
> Pavel> not ortogonal. Every not dynamic statement change FOUND
> Pavel> variable.
>
> Regardless of what you think of FOUND, a more serious problem is this:
>
> postgres=# create function test(n integer) returns setof integer language plpgsql
> as $f$
> declare
> rc bigint;
> begin
> return query (select i from generate_series(1,n) i);
> get diagnostics rc = row_count;
> raise notice 'rc = %',rc;
> end;
> $f$;
> CREATE FUNCTION
> postgres=# select test(3);
> NOTICE: rc = 0
> test
> ------
> 1
> 2
> 3
> (3 rows)
>
> Since GET DIAGNOSTICS is documented as working for every SQL query
> executed in the function, rather than for a specific list of
> constructs, this is clearly a bug.
>
> --
> Andrew (irc:RhodiumToad)
>
> --
> Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
>

Uh, is this ready to be applied?

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

Pavel Stehule wrote:
> I am sending patch, that adds FOUND and GET DIAGNOSTICS support for
> RETURN QUERY statement
>
> Regards
> Pavel Stehule
>
>
>
> 2008/11/10 Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>:
> >>>>>> "Pavel" == "Pavel Stehule" <pavel(dot)stehule(at)gmail(dot)com> writes:
> >
> > >> Well, changing the semantics of an already-released statement
> > >> carries a risk of breaking existing apps that aren't expecting it
> > >> to change FOUND. So I'd want to see a pretty strong case why this
> > >> is important --- not just that it didn't meet someone's
> > >> didn't-read-the-manual expectation.
> >
> > Pavel> It's should do some problems, but I belive much less than
> > Pavel> change of casting or tsearch2 integration. And actually it's
> > Pavel> not ortogonal. Every not dynamic statement change FOUND
> > Pavel> variable.
> >
> > Regardless of what you think of FOUND, a more serious problem is this:
> >
> > postgres=# create function test(n integer) returns setof integer language plpgsql
> > as $f$
> > declare
> > rc bigint;
> > begin
> > return query (select i from generate_series(1,n) i);
> > get diagnostics rc = row_count;
> > raise notice 'rc = %',rc;
> > end;
> > $f$;
> > CREATE FUNCTION
> > postgres=# select test(3);
> > NOTICE: rc = 0
> > test
> > ------
> > 1
> > 2
> > 3
> > (3 rows)
> >
> > Since GET DIAGNOSTICS is documented as working for every SQL query
> > executed in the function, rather than for a specific list of
> > constructs, this is clearly a bug.
> >
> > --
> > Andrew (irc:RhodiumToad)
> >
> > --
> > Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-bugs
> >

[ Attachment, skipping... ]

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Uh, is this ready to be applied?

I don't think any consensus has been reached on changing this behavior.

regards, tom lane

Hello

2009/1/10 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> Uh, is this ready to be applied?
>
> I don't think any consensus has been reached on changing this behavior.
>
> regards, tom lane
>

I thing, so this is bug - RETURN QUERY has to supply FOR SELECT LOOP
RETURN NEXT pattern.

My first patch expected so RETURN QUERY is final statement, so I don't
solve FOUND variable, but Heikki changed this behave.

Without correct FOUND behave we can't to use RETURN QUERY for following pattern

RETURN QUERY some;
IF FOUND THEN RETURN; END IF;
RETURN QUERY some_other;
RETURN;

regards
Pavel Stehule

> 2009/1/10 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>>> Uh, is this ready to be applied?
>>
>> I don't think any consensus has been reached on changing this behavior.
>
> I thing, so this is bug - RETURN QUERY has to supply FOR SELECT LOOP
> RETURN NEXT pattern.
>
> My first patch expected so RETURN QUERY is final statement, so I don't
> solve FOUND variable, but Heikki changed this behave.
>
> Without correct FOUND behave we can't to use RETURN QUERY for following pattern
>
> RETURN QUERY some;
> IF FOUND THEN RETURN; END IF;
> RETURN QUERY some_other;
> RETURN;

+1. I can't imagine it's good for this to be randomly inconsistent.

...Robert

Pavel Stehule wrote:
> My first patch expected so RETURN QUERY is final statement, so I don't
> solve FOUND variable, but Heikki changed this behave.

Me? I don't recall doing anything related to this.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

2009/1/10 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> Pavel Stehule wrote:
>>
>> My first patch expected so RETURN QUERY is final statement, so I don't
>> solve FOUND variable, but Heikki changed this behave.
>
> Me? I don't recall doing anything related to this.
>

I have to look to archive, maybe Peter.

Pavel

> --
> Heikki Linnakangas
> EnterpriseDB http://www.enterprisedb.com
>

Robert Haas wrote:
> > 2009/1/10 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> >> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> >>> Uh, is this ready to be applied?
> >>
> >> I don't think any consensus has been reached on changing this behavior.
> >
> > I thing, so this is bug - RETURN QUERY has to supply FOR SELECT LOOP
> > RETURN NEXT pattern.
> >
> > My first patch expected so RETURN QUERY is final statement, so I don't
> > solve FOUND variable, but Heikki changed this behave.
> >
> > Without correct FOUND behave we can't to use RETURN QUERY for following pattern
> >
> > RETURN QUERY some;
> > IF FOUND THEN RETURN; END IF;
> > RETURN QUERY some_other;
> > RETURN;
>
> +1. I can't imagine it's good for this to be randomly inconsistent.

So should this be applied or just kept for 8.5?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

On Wed, Feb 4, 2009 at 1:32 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> > 2009/1/10 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> >> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> >>> Uh, is this ready to be applied?
>> >>
>> >> I don't think any consensus has been reached on changing this behavior.
>> >
>> > I thing, so this is bug - RETURN QUERY has to supply FOR SELECT LOOP
>> > RETURN NEXT pattern.
>> >
>> > My first patch expected so RETURN QUERY is final statement, so I don't
>> > solve FOUND variable, but Heikki changed this behave.
>> >
>> > Without correct FOUND behave we can't to use RETURN QUERY for following pattern
>> >
>> > RETURN QUERY some;
>> > IF FOUND THEN RETURN; END IF;
>> > RETURN QUERY some_other;
>> > RETURN;
>>
>> +1. I can't imagine it's good for this to be randomly inconsistent.
>
> So should this be applied or just kept for 8.5?

Well, *I* think it should be applied. But YMMV.

...Robert

Pavel Stehule wrote:
> I am sending patch, that adds FOUND and GET DIAGNOSTICS support for
> RETURN QUERY statement

Updated patch attached and applied. Thanks.

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

>
> Regards
> Pavel Stehule
>
>
>
> 2008/11/10 Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>:
> >>>>>> "Pavel" == "Pavel Stehule" <pavel(dot)stehule(at)gmail(dot)com> writes:
> >
> > >> Well, changing the semantics of an already-released statement
> > >> carries a risk of breaking existing apps that aren't expecting it
> > >> to change FOUND. So I'd want to see a pretty strong case why this
> > >> is important --- not just that it didn't meet someone's
> > >> didn't-read-the-manual expectation.
> >
> > Pavel> It's should do some problems, but I belive much less than
> > Pavel> change of casting or tsearch2 integration. And actually it's
> > Pavel> not ortogonal. Every not dynamic statement change FOUND
> > Pavel> variable.
> >
> > Regardless of what you think of FOUND, a more serious problem is this:
> >
> > postgres=# create function test(n integer) returns setof integer language plpgsql
> > as $f$
> > declare
> > rc bigint;
> > begin
> > return query (select i from generate_series(1,n) i);
> > get diagnostics rc = row_count;
> > raise notice 'rc = %',rc;
> > end;
> > $f$;
> > CREATE FUNCTION
> > postgres=# select test(3);
> > NOTICE: rc = 0
> > test
> > ------
> > 1
> > 2
> > 3
> > (3 rows)
> >
> > Since GET DIAGNOSTICS is documented as working for every SQL query
> > executed in the function, rather than for a specific list of
> > constructs, this is clearly a bug.
> >
> > --
> > Andrew (irc:RhodiumToad)
> >
> > --
> > Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-bugs
> >

[ Attachment, skipping... ]

>
> --
> Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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