Skip site navigation (1) Skip section navigation (2)

Re: WIP: pl/pgsql cleanup

From: Neil Conway <neilc(at)samurai(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: WIP: pl/pgsql cleanup
Date: 2005-02-21 06:24:31
Message-ID: 42197E9F.9010906@samurai.com (view raw or flat)
Thread:
Lists: pgsql-patches
Tom Lane wrote:
> Still a few bricks shy of a load I fear [...]

My apologies; thanks for the code review.

> regression=# create or replace function foo() returns int language plpgsql as $$
> regression$# begin
> regression$#   return ;
> regression$# end$$;
> CREATE FUNCTION
> regression=# select foo();
> server closed the connection unexpectedly
> 
> If you're going to stick a NULL into the return's expression then you'd
> better check for same when it's used.

Right. Better I think is to only allow a missing RETURN expression for 
functions declared to return void anyway; that catches the mistake at 
compile-time. I've implemented this behavior in the revised patch. The 
error message in this scenario is still a little poor:

create function missing_return_expr() returns int as $$
begin
     return ;
end;$$ language plpgsql;
ERROR:  syntax error at end of input at character 8
QUERY:  SELECT
CONTEXT:  SQL statement in PL/PgSQL function "missing_return_expr" near 
line 2
LINE 1: SELECT
                ^

Is it worth putting in a special-case to look for an unexpected ';' in 
either the RETURN production or read_sql_construct() ?

> What I was actually intending to check when I ran into that is why some
> of the error paths in your FOR-loop rewrite use
> 	plpgsql_error_lineno = $1;
> while others have
> 	plpgsql_error_lineno = plpgsql_scanner_lineno();
> I suspect the former is more appropriate, at least for errors that
> reference the FOR variable and not some other part of the statement.
> Also why the inconsistent use of yyerror() vs ereport()?

Sorry, both of these result from sloppiness on my part: I think the code 
was originally correct, but I tried to refactor some of the more complex 
parts of the FOR statement production into a separate function, and 
eventually wasn't able to (because of the `forvariable' union member). 
When I gave up and reverted the code, I obviously wasn't careful enough.

Attached is a revised patch.

-Neil

Attachment: plpgsql_cleanup-34.patch.gz
Description: application/gzip (25.0 KB)

In response to

Responses

pgsql-patches by date

Next:From: Andreas PflugDate: 2005-02-21 10:18:19
Subject: was: BUG #1466: syslogger issues
Previous:From: Neil ConwayDate: 2005-02-21 02:20:51
Subject: Re: Bibliography updates

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group