Thread: Re: [BUGS] BUG #4516: FOUND variable does not work after RETURN QUERY
I am sending patch, that adds FOUND and GET DIAGNOSTICS support for RETURN QUERY statement Regards Pavel Stehule 2008/11/10 Andrew Gierth <andrew@tao11.riddles.org.uk>: >>>>>> "Pavel" == "Pavel Stehule" <pavel.stehule@gmail.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-bugs >
Attachment
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@tao11.riddles.org.uk>: > >>>>>> "Pavel" == "Pavel Stehule" <pavel.stehule@gmail.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@postgresql.org) > > To make changes to your subscription: > > http://www.postgresql.org/mailpref/pgsql-bugs > > [ Attachment, skipping... ] > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.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@sss.pgh.pa.us>: > Bruce Momjian <bruce@momjian.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@sss.pgh.pa.us>: >> Bruce Momjian <bruce@momjian.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.linnakangas@enterprisedb.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@sss.pgh.pa.us>: > >> Bruce Momjian <bruce@momjian.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@momjian.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@momjian.us> wrote: > Robert Haas wrote: >> > 2009/1/10 Tom Lane <tgl@sss.pgh.pa.us>: >> >> Bruce Momjian <bruce@momjian.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@tao11.riddles.org.uk>: > >>>>>> "Pavel" == "Pavel Stehule" <pavel.stehule@gmail.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@postgresql.org) > > To make changes to your subscription: > > http://www.postgresql.org/mailpref/pgsql-bugs > > [ Attachment, skipping... ] > > -- > Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-bugs -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: doc/src/sgml/plpgsql.sgml =================================================================== RCS file: /cvsroot/pgsql/doc/src/sgml/plpgsql.sgml,v retrieving revision 1.137 diff -c -c -r1.137 plpgsql.sgml *** doc/src/sgml/plpgsql.sgml 4 Feb 2009 21:30:41 -0000 1.137 --- doc/src/sgml/plpgsql.sgml 5 Feb 2009 15:15:03 -0000 *************** *** 1356,1361 **** --- 1356,1369 ---- execution of other statements within the loop body. </para> </listitem> + <listitem> + <para> + A <command>RETURN QUERY</command> and <command>RETURN QUERY + EXECUTE</command> statements set <literal>FOUND</literal> + true if the query returns at least one row, false if no row + is returned. + </para> + </listitem> </itemizedlist> <literal>FOUND</literal> is a local variable within each Index: src/pl/plpgsql/src/pl_exec.c =================================================================== RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v retrieving revision 1.231 diff -c -c -r1.231 pl_exec.c *** src/pl/plpgsql/src/pl_exec.c 21 Jan 2009 11:13:14 -0000 1.231 --- src/pl/plpgsql/src/pl_exec.c 5 Feb 2009 15:15:03 -0000 *************** *** 2286,2291 **** --- 2286,2292 ---- PLpgSQL_stmt_return_query *stmt) { Portal portal; + uint32 processed = 0; if (!estate->retisset) ereport(ERROR, *************** *** 2327,2332 **** --- 2328,2334 ---- HeapTuple tuple = SPI_tuptable->vals[i]; tuplestore_puttuple(estate->tuple_store, tuple); + processed++; } MemoryContextSwitchTo(old_cxt); *************** *** 2336,2341 **** --- 2338,2346 ---- SPI_freetuptable(SPI_tuptable); SPI_cursor_close(portal); + estate->eval_processed = processed; + exec_set_found(estate, processed != 0); + return PLPGSQL_RC_OK; } Index: src/test/regress/expected/plpgsql.out =================================================================== RCS file: /cvsroot/pgsql/src/test/regress/expected/plpgsql.out,v retrieving revision 1.66 diff -c -c -r1.66 plpgsql.out *** src/test/regress/expected/plpgsql.out 18 Jul 2008 03:32:53 -0000 1.66 --- src/test/regress/expected/plpgsql.out 5 Feb 2009 15:15:03 -0000 *************** *** 3666,3668 **** --- 3666,3700 ---- (2 rows) drop function tftest(int); + create or replace function rttest() + returns setof int as $$ + declare rc int; + begin + return query values(10),(20); + get diagnostics rc = row_count; + raise notice '% %', found, rc; + return query select * from (values(10),(20)) f(a) where false; + get diagnostics rc = row_count; + raise notice '% %', found, rc; + return query execute 'values(10),(20)'; + get diagnostics rc = row_count; + raise notice '% %', found, rc; + return query execute 'select * from (values(10),(20)) f(a) where false'; + get diagnostics rc = row_count; + raise notice '% %', found, rc; + end; + $$ language plpgsql; + select * from rttest(); + NOTICE: t 2 + NOTICE: f 0 + NOTICE: t 2 + NOTICE: f 0 + rttest + -------- + 10 + 20 + 10 + 20 + (4 rows) + + drop function rttest(); Index: src/test/regress/sql/plpgsql.sql =================================================================== RCS file: /cvsroot/pgsql/src/test/regress/sql/plpgsql.sql,v retrieving revision 1.56 diff -c -c -r1.56 plpgsql.sql *** src/test/regress/sql/plpgsql.sql 18 Jul 2008 03:32:53 -0000 1.56 --- src/test/regress/sql/plpgsql.sql 5 Feb 2009 15:15:03 -0000 *************** *** 2948,2950 **** --- 2948,2974 ---- select * from tftest(10); drop function tftest(int); + + create or replace function rttest() + returns setof int as $$ + declare rc int; + begin + return query values(10),(20); + get diagnostics rc = row_count; + raise notice '% %', found, rc; + return query select * from (values(10),(20)) f(a) where false; + get diagnostics rc = row_count; + raise notice '% %', found, rc; + return query execute 'values(10),(20)'; + get diagnostics rc = row_count; + raise notice '% %', found, rc; + return query execute 'select * from (values(10),(20)) f(a) where false'; + get diagnostics rc = row_count; + raise notice '% %', found, rc; + end; + $$ language plpgsql; + + select * from rttest(); + + drop function rttest(); +