Re: a few small bugs in plpgsql

Hello,

today I found a few bugs:

a) parser allow a labels on invalid positions with strange runtime bug:

postgres=# CREATE OR REPLACE FUNCTION foo()
RETURNS void AS $$
BEGIN
FOR i IN 1..2
<<<invalidLabel>>
LOOP
RAISE NOTICE '%',i;
END LOOP;
END;
$$ LANGUAGE plpgsql;
CREATE FUNCTION

ERROR: column "invalidlabel" does not exist
LINE 2: <<<invalidLabel>>
^
QUERY: SELECT 2
<<<invalidLabel>>
CONTEXT: PL/pgSQL function "foo" line 3 at FOR with integer loop variable
postgres=#

b) SRF functions must not be finished by RETURN statement - I know, so
there is outer default block, but it looks like inconsistency for SRF
functions, because you can use a RETURN NEXT without RETURN. It maybe
isn't bug - but I am filling it as inconsistency.

postgres=# CREATE OR REPLACE FUNCTION fg(OUT i int)
RETURNS SETOF int AS $$
BEGIN
FOR i IN 1..3
LOOP fg.i := i;
RETURN NEXT;
END LOOP;
END;
$$ LANGUAGE plpgsql;
CREATE FUNCTION

postgres=# select fg();
fg
----
1
2
3
(3 rows)

Regards

Pavel Stehule

On Thu, Oct 7, 2010 at 2:53 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello,
>
> today I found a few bugs:
>
> a) parser allow a labels on invalid positions with strange runtime bug:
>
> postgres=# CREATE OR REPLACE FUNCTION foo()
> RETURNS void AS $$
> BEGIN
>  FOR i IN 1..2
>  <<<invalidLabel>>
>  LOOP
>    RAISE NOTICE '%',i;
>  END LOOP;
> END;
> $$ LANGUAGE plpgsql;
> CREATE FUNCTION
>
> ERROR:  column "invalidlabel" does not exist
> LINE 2:   <<<invalidLabel>>
>             ^
> QUERY:  SELECT 2
>  <<<invalidLabel>>
> CONTEXT:  PL/pgSQL function "foo" line 3 at FOR with integer loop variable
> postgres=#

I'm not sure if I'd call that a bug, but it does look like a somewhat
odd error message, at least at first glance.

> b) SRF functions must not be finished by RETURN statement - I know, so
> there is outer default block, but it looks like inconsistency for SRF
> functions, because you can use a RETURN NEXT without RETURN. It maybe
> isn't bug - but I am filling it as inconsistency.
>
> postgres=# CREATE OR REPLACE FUNCTION fg(OUT i int)
> RETURNS SETOF int AS $$
> BEGIN
>  FOR i IN 1..3
>  LOOP fg.i := i;
>    RETURN NEXT;
>  END LOOP;
> END;
> $$ LANGUAGE plpgsql;
> CREATE FUNCTION
>
> postgres=# select fg();
>  fg
> ----
>  1
>  2
>  3
> (3 rows)

I don't see what's wrong with this.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

>> b) SRF functions must not be finished by RETURN statement - I know, so
>> there is outer default block, but it looks like inconsistency for SRF
>> functions, because you can use a RETURN NEXT without RETURN. It maybe
>> isn't bug - but I am filling it as inconsistency.

Hmmm. Is there any likelyhood we'll go back to requiring the final
RETURN in the future?

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Oct 7, 2010 at 2:53 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> b) SRF functions must not be finished by RETURN statement - I know, so
>> there is outer default block, but it looks like inconsistency for SRF
>> functions, because you can use a RETURN NEXT without RETURN. It maybe
>> isn't bug - but I am filling it as inconsistency.

> I don't see what's wrong with this.

Back around 8.0 we intentionally changed plpgsql to not require a final
RETURN in cases where RETURN isn't used to supply the result value:
http://archives.postgresql.org/pgsql-hackers/2005-04/msg00152.php
http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=e00ee887612da0dab02f1a56e33d8ae821710e14

Even if there were a good argument for going back to the old way,
backwards-compatibility would win the day, I think. Being strict
about this --- in *either* direction --- would break a lot of code.

regards, tom lane

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> a) parser allow a labels on invalid positions with strange runtime bug:

> postgres=# CREATE OR REPLACE FUNCTION foo()
> RETURNS void AS $$
> BEGIN
> FOR i IN 1..2
> <<<invalidLabel>>
> LOOP
> RAISE NOTICE '%',i;
> END LOOP;
> END;
> $$ LANGUAGE plpgsql;
> CREATE FUNCTION

> ERROR: column "invalidlabel" does not exist
> LINE 2: <<<invalidLabel>>
> ^
> QUERY: SELECT 2
> <<<invalidLabel>>
> CONTEXT: PL/pgSQL function "foo" line 3 at FOR with integer loop variable

That isn't a bug, because the construct isn't a label, and wouldn't be
even if you'd gotten the number of <'s right ;-). What you have is an
expression "2 <<< invalidLabel >>", which given the right operator
definitions could be perfectly valid. plpgsql labels can't appear in
the middle of an SQL expression.

regards, tom lane

Hello

2010/10/8 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> a) parser allow a labels on invalid positions with strange runtime bug:
>
>> postgres=# CREATE OR REPLACE FUNCTION foo()
>> RETURNS void AS $$
>> BEGIN
>>   FOR i IN 1..2
>>   <<<invalidLabel>>
>>   LOOP
>>     RAISE NOTICE '%',i;
>>   END LOOP;
>> END;
>> $$ LANGUAGE plpgsql;
>> CREATE FUNCTION
>
>> ERROR:  column "invalidlabel" does not exist
>> LINE 2:   <<<invalidLabel>>
>>              ^
>> QUERY:  SELECT 2
>>   <<<invalidLabel>>
>> CONTEXT:  PL/pgSQL function "foo" line 3 at FOR with integer loop variable
>
> That isn't a bug, because the construct isn't a label, and wouldn't be
> even if you'd gotten the number of <'s right ;-).  What you have is an
> expression "2 <<< invalidLabel >>", which given the right operator
> definitions could be perfectly valid.  plpgsql labels can't appear in
> the middle of an SQL expression.
>

I see it now - I did a bug <<<some>>, but with correct text there is same behave

CREATE OR REPLACE FUNCTION foo()
RETURNS void AS $$
BEGIN
FOR i IN 1..10
<<label>>
LOOP
RAISE NOTICE '%', i;
END LOOP;
END;
$$ LANGUAGE plpgsql;
CREATE FUNCTION

Now I understand to interpretation. But there is little bit difficult
to understand to error message. Can be message enhanced to show a
complete expression? Some like

postgres=# select foo();
ERROR: column "label" does not exist
Detail: Expr 10 <<label>>
LINE 2: <<label>>

Regards

Pavel

>                        regards, tom lane
>

2010/10/8 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Oct 7, 2010 at 2:53 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> b) SRF functions must not be finished by RETURN statement - I know, so
>>> there is outer default block, but it looks like inconsistency for SRF
>>> functions, because you can use a RETURN NEXT without RETURN. It maybe
>>> isn't bug - but I am filling it as inconsistency.
>
>> I don't see what's wrong with this.
>
> Back around 8.0 we intentionally changed plpgsql to not require a final
> RETURN in cases where RETURN isn't used to supply the result value:
> http://archives.postgresql.org/pgsql-hackers/2005-04/msg00152.php
> http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=e00ee887612da0dab02f1a56e33d8ae821710e14
>
> Even if there were a good argument for going back to the old way,
> backwards-compatibility would win the day, I think.  Being strict
> about this --- in *either* direction --- would break a lot of code.
>
>                        regards, tom lane

ok, understand - thank you. I think so it was not a best decision -
the RETURN statement helps with higher verbosity, but I can accept so
there are not way to back.

Regards

Pavel

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> Now I understand to interpretation. But there is little bit difficult
> to understand to error message. Can be message enhanced to show a
> complete expression?

It does already:

regression=# select foo();
ERROR: column "label" does not exist
LINE 2: <<label>>
^
QUERY: SELECT 10
<<label>>
CONTEXT: PL/pgSQL function "foo" line 3 at FOR with integer loop variable

The "query" is SELECT followed by whatever plpgsql thought the
expression was.

regards, tom lane

2010/10/8 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> Now I understand to interpretation. But there is little bit difficult
>> to understand to error message. Can be message enhanced to show a
>> complete expression?
>
> It does already:
>
> regression=# select foo();
> ERROR:  column "label" does not exist
> LINE 2:   <<label>>
>            ^
> QUERY:  SELECT 10

the keyword QUERY is misleading :(

but you have a true - there are all necessary information.

Regards

Pavel Stehule

>  <<label>>
> CONTEXT:  PL/pgSQL function "foo" line 3 at FOR with integer loop variable
>
> The "query" is SELECT followed by whatever plpgsql thought the
> expression was.
>
>                        regards, tom lane
>