a few small bugs in plpgsql

On Thu, Oct 7, 2010 at 2:53 PM, Pavel Stehule <pavel.stehule@gmail.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@gmail.com> writes:

On Thu, Oct 7, 2010 at 2:53 PM, Pavel Stehule <pavel.stehule@gmail.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.stehule@gmail.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@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.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

2010/10/8 Tom Lane <tgl@sss.pgh.pa.us>:

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Oct 7, 2010 at 2:53 PM, Pavel Stehule <pavel.stehule@gmail.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.stehule@gmail.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@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.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