dbTalk Databases Forums  

[BUGS] BUG #1329: Bug in IF-ELSEIF-ELSE construct

mailing.database.pgsql-bugs mailing.database.pgsql-bugs


Discuss [BUGS] BUG #1329: Bug in IF-ELSEIF-ELSE construct in the mailing.database.pgsql-bugs forum.



Reply
 
Thread Tools Display Modes
  #1  
Old   
PostgreSQL Bugs List
 
Posts: n/a

Default [BUGS] BUG #1329: Bug in IF-ELSEIF-ELSE construct - 11-26-2004 , 05:19 AM







The following bug has been logged online:

Bug reference: 1329
Logged by: Rico Wind

Email address: rw (AT) rico-wind (DOT) dk

PostgreSQL version: 8.0 Beta

Operating system: Windows XP, SP2

Description: Bug in IF-ELSEIF-ELSE construct

Details:

Beta 1.
The following always returns 4:

IF from_date_param=period_begin AND until_date_param=period_end
THEN
return 1;
ELSEIF from_date_param=period_begin
THEN
return 2;
ELSEIF until_date_param=period_end
THEN
return 3;
ELSE
return 4;
END IF;

Whereas the following returns the right answer(not 4 each time). They should
be the same.
IF from_date_param=period_begin AND until_date_param=period_end
THEN
return 1;
ELSE
IF from_date_param = period_begin
THEN
return 2;
END IF;

IF until_date_param=period_end
THEN
return 3;
END IF;
END IF;
RETURN 4;


---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

Reply With Quote
  #2  
Old   
Tom Lane
 
Posts: n/a

Default Re: [BUGS] BUG #1329: Bug in IF-ELSEIF-ELSE construct - 11-26-2004 , 11:57 AM






"PostgreSQL Bugs List" <pgsql-bugs (AT) postgresql (DOT) org> writes:
Quote:
Description: Bug in IF-ELSEIF-ELSE construct
There is no ELSEIF construct. Try ELSIF.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster


Reply With Quote
  #3  
Old   
Neil Conway
 
Posts: n/a

Default Re: [BUGS] BUG #1329: Bug in IF-ELSEIF-ELSE construct - 11-27-2004 , 12:20 AM



Tom Lane wrote:
Quote:
There is no ELSEIF construct.
Sure, but it would be nice to throw a syntax error rather than silently
accepting the function. Unfortunately the way PL/PgSQL's parser works
doesn't make this very easy. (BTW, I think that fixing how we do parsing
would be one of the prime motivations for rewriting PL/PgSQL. One
possibility would be to integrate the PL/PgSQL parser into the main SQL
parser, although there may be a cleaner way to improve PL/PgSQL parsing.)

In any case, given this function:

create or replace function foo() returns int as
'
#option dump
begin
if 5 > 5 then
return 10;
elseif 5 > 6 then
return 15;
else
return 20;
end if;
end;' language 'plpgsql';

We produce this parsetree: (helpfully dumped via the undocumented
"#option dump" feature)

Functions statements:
2:BLOCK <<*unnamed*>>
3: IF 'SELECT 5 > 5' THEN
4: RETURN 'SELECT 10'
5: EXECSQL 'elseif 5 > 6 then 15 15'
ELSE
8: RETURN 'SELECT 20'
ENDIF
END -- *unnamed*

One way to fix the specific bug reported here would be to add K_ELSEIF
to the PL/PgSQL lexer, and then throw a syntax error in the stmt_else
production. But that is a very limited fix: if the user specifies any
other word in the place of 'elseif', we will not throw a syntax error.

Another solution would be to teach the PL/PgSQL lexer to recognize the
initial tokens of every SQL statement (SELECT, UPDATE, and so forth).
Right now we just assume an unrecognized word must be the beginning of a
SQL statement; if we taught the lexer about the initial tokens of all
legal SQL statements, we could reject unrecognized words. But this is
kind of ugly as well, as it means duplicating the knowledge about what
constitutes a legal SQL statement in multiple places.

Alternatively, we could arrange to have the PL/PgSQL parser pass a block
of text it has identified as a possible SQL statement to the main SQL
parser; if it produces a syntax error, we can pass that syntax error
back to the user. I'm not sure if this would have any negative
ramifications, though.

Comments?

(BTW, another thing this example exposes is that we don't issue warnings
about trivially-dead-code, such as statements in a basic block that
follow a RETURN. This would probably be also worth doing.)

-Neil

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faqs/FAQ.html


Reply With Quote
  #4  
Old   
Tom Lane
 
Posts: n/a

Default Re: [BUGS] BUG #1329: Bug in IF-ELSEIF-ELSE construct - 11-27-2004 , 12:09 PM



Neil Conway <neilc (AT) samurai (DOT) com> writes:
Quote:
Tom Lane wrote:
There is no ELSEIF construct.

Sure, but it would be nice to throw a syntax error rather than silently
accepting the function. Unfortunately the way PL/PgSQL's parser works
doesn't make this very easy.
Actually, the simplest solution would be to just *allow* ELSEIF as a
variant spelling of ELSIF. I cannot see any real good argument for
not doing that, and considering that we've seen two people make the
same mistake in the past month, my interest in doing it is increasing.

Quote:
(BTW, I think that fixing how we do parsing
would be one of the prime motivations for rewriting PL/PgSQL.
Yeah, if we could think of a way. Copying the main grammar a la ecpg is
definitely not the way :-(

Quote:
Alternatively, we could arrange to have the PL/PgSQL parser pass a block
of text it has identified as a possible SQL statement to the main SQL
parser; if it produces a syntax error, we can pass that syntax error
back to the user. I'm not sure if this would have any negative
ramifications, though.
This seems like the most appropriate answer to me; I was thinking of
doing that earlier when Fabien and I were fooling with plpgsql error
reporting, but didn't get around to it.

As long as you only do this in check_function_bodies mode it seems
safe enough. One possible problem (if it's done towards the end of
parsing as you've suggested for dead-code checking) is that a syntax
error in a SQL statement might confuse the plpgsql parser into
misparsing statement boundaries, which could lead to a plpgsql parse
error much further down, such as a "missing END" at the end of the
function. The error would be more useful if reported immediately
after the putative SQL statement is parsed. Not sure if that's
hard or not. (The same remark applies to dead code checking, now
that I think about it.)

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faqs/FAQ.html


Reply With Quote
  #5  
Old   
Neil Conway
 
Posts: n/a

Default Re: [BUGS] BUG #1329: Bug in IF-ELSEIF-ELSE construct - 11-28-2004 , 09:39 PM



--=-+nCK3IF5DcEUvPFvVfzt
Content-Type: text/plain
Content-Transfer-Encoding: 7bit

On Sat, 2004-11-27 at 12:55 -0500, Tom Lane wrote:
Quote:
This seems like the most appropriate answer to me; I was thinking of
doing that earlier when Fabien and I were fooling with plpgsql error
reporting, but didn't get around to it.
Attached is a patch that implements a rough draft of this (it also
includes an improved version of the "unreachable code" patch that
includes your suggested fixes). Two questions about the patch:

(1) It doesn't report syntax errors in unreachable code. I suppose this
ought to be done, right?

(2) The syntax error message is wrong (we print a character offset and
query context that is relative to the CREATE FUNCTION statement, not the
individual SQL statement we're executing). I fooled around a bit with
defining a custom ereport() callback to print the right line number and
query context, but I couldn't get it right. Do you have any guidance on
the proper way to do this.

Quote:
As long as you only do this in check_function_bodies mode it seems
safe enough. One possible problem (if it's done towards the end of
parsing as you've suggested for dead-code checking) is that a syntax
error in a SQL statement might confuse the plpgsql parser into
misparsing statement boundaries, which could lead to a plpgsql parse
error much further down, such as a "missing END" at the end of the
function. The error would be more useful if reported immediately
after the putative SQL statement is parsed. Not sure if that's
hard or not. (The same remark applies to dead code checking, now
that I think about it.)
In the case of dead code checking, I don't think it matters. Doing the
syntax check in gram.y might be a better idea, I'll take a look doing
that...

-Neil


--=-+nCK3IF5DcEUvPFvVfzt
Content-Disposition: attachment; filename=plpgsql-unreachable-3.patch
Content-Type: text/x-patch; name=plpgsql-unreachable-3.patch; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

#
# patch "src/pl/plpgsql/src/pl_comp.c"
# from [94dec38ffccf5fceed00c2b1007cf2b0c238af4b]
# to [3223e07f0046277dbea155774132975d6e636fa7]
#
# patch "src/test/regress/expected/plpgsql.out"
# from [1b0591090144d66281066d54d81ef9a238809b0d]
# to [320d79127d4f067eea8c09f0e17174721e96a9df]
#
# patch "src/test/regress/sql/plpgsql.sql"
# from [4ca1c085eda5ca4fa0397354d887a28d10236594]
# to [261d4fa85aef9eb40c2922e433af025a5b19f38a]
#
--- src/pl/plpgsql/src/pl_comp.c
+++ src/pl/plpgsql/src/pl_comp.c
@@ -52,6 +52,7 @@
#include "nodes/makefuncs.h"
#include "parser/gramparse.h"
#include "parser/parse_type.h"
+#include "parser/parser.h"
#include "tcop/tcopprot.h"
#include "utils/array.h"
#include "utils/builtins.h"
@@ -130,6 +131,7 @@
static void plpgsql_HashTableInsert(PLpgSQL_function *function,
PLpgSQL_func_hashkey *func_key);
static void plpgsql_HashTableDelete(PLpgSQL_function *function);
+static void check_function(PLpgSQL_function *function);

/*
* This routine is a crock, and so is everyplace that calls it. The problem
@@ -152,8 +154,10 @@
/* ----------
* plpgsql_compile Make an execution tree for a PL/pgSQL function.
*
- * If forValidator is true, we're only compiling for validation purposes,
- * and so some checks are skipped.
+ * If forValidator is true, we're only compiling for validation
+ * purposes; this means we skip some checks, as well as making some
+ * additional compile-time checks that we only want to do once (at
+ * function definition time), not very time the function is compiled.
*
* Note: it's important for this to fall through quickly if the function
* has already been compiled.
@@ -293,7 +297,7 @@
* Setup error traceback support for ereport()
*/
plerrcontext.callback = plpgsql_compile_error_callback;
- plerrcontext.arg = forValidator ? proc_source : (char *) NULL;
+ plerrcontext.arg = forValidator ? proc_source : NULL;
plerrcontext.previous = error_context_stack;
error_context_stack = &plerrcontext;

@@ -595,17 +599,16 @@
plpgsql_add_initdatums(NULL);

/*
- * Now parse the functions text
+ * Now parse the function's text
*/
parse_rc = plpgsql_yyparse();
if (parse_rc != 0)
elog(ERROR, "plpgsql parser returned %d", parse_rc);

plpgsql_scanner_finish();
- pfree(proc_source);

/*
- * If that was successful, complete the functions info.
+ * If that was successful, complete the function's info.
*/
function->fn_nargs = procStruct->pronargs;
for (i = 0; i < function->fn_nargs; i++)
@@ -616,12 +619,22 @@
function->datums[i] = plpgsql_Datums[i];
function->action = plpgsql_yylval.program;

+ /*
+ * Perform whatever additional compile-time checks we can. Note
+ * that we only do this when validating the function; this is so
+ * that (a) we don't bother the user with warnings when the
+ * function is invoked (b) we don't take the performance hit of
+ * doing the analysis more than once per function definition.
+ */
+ if (forValidator)
+ check_function(function);
+
/* Debug dump for completed functions */
if (plpgsql_DumpExecTree)
plpgsql_dumptree(function);

/*
- * add it to the hash table
+ * Add it to the hash table
*/
plpgsql_HashTableInsert(function, hashkey);

@@ -631,6 +644,7 @@
error_context_stack = plerrcontext.previous;
plpgsql_error_funcname = NULL;
plpgsql_error_lineno = 0;
+ pfree(proc_source);

return function;
}
@@ -664,8 +678,198 @@
plpgsql_error_funcname, plpgsql_error_lineno);
}

+/*
+ * The PL/PgSQL parser will often assume that an unrecognized keyword
+ * indicates the beginning of a SQL statement. That avoids the need to
+ * duplicate parts of the SQL grammar in the PL/PgSQL parser, but it
+ * also means we are very liberal in the input we accept. To try and
+ * catch some of the more obviously invalid input, we run strings that
+ * we've guessed to be SQL statements through the backend's parser.
+ *
+ * We only invoke the "raw" parser (not the analyzer); this doesn't do
+ * any database access, it just checks that the query is (mostly)
+ * syntactically correct.
+ *
+ * XXX: do we want to check that certain SQL statements cannot be
+ * used?
+ *
+ * XXX: fix line number in syntax error message
+ *
+ * XXX: fix the context that is printed on error
+ */
+static void
+check_sql_statement(char *stmt_string)
+{
+ MemoryContext tmpCxt;
+ MemoryContext oldCxt;

+ /* XXX: reset a passed-in temporary context, don't create/destroy */
+ tmpCxt = AllocSetContextCreate(CurrentMemoryContext,
+ "PL/PgSQL syntax check",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+ oldCxt = MemoryContextSwitchTo(tmpCxt);
+
+ raw_parser(stmt_string);
+
+ MemoryContextSwitchTo(oldCxt);
+ MemoryContextDelete(tmpCxt);
+}
+
/*
+ * Emit a warning that the statement 'unreach' is unreachable, due to
+ * the effect of the preceding statement 'cause'.
+ */
+static void
+report_unreachable_stmt(PLpgSQL_stmt *unreach, PLpgSQL_stmt *cause)
+{
+ /*
+ * XXX: adjust the line number that is emitted along with the
+ * warning message. This is a kludge.
+ */
+ int old_lineno = plpgsql_error_lineno;
+ plpgsql_error_lineno = unreach->lineno;
+
+ /*
+ * XXX: error code?
+ */
+ ereport(WARNING,
+ (errmsg("%s is unreachable, due to %s near line %d",
+ plpgsql_stmt_typename(unreach),
+ plpgsql_stmt_typename(cause),
+ cause->lineno)));
+
+ plpgsql_error_lineno = old_lineno;
+}
+
+/*
+ * Given a list of PL/PgSQL statements, perform some compile-time
+ * checks on them.
+ *
+ * Note when checking for unreachable code, we return as soon as we
+ * have emitted "unreachable" warnings for a given sequence of
+ * statements. So that given:
+ *
+ * EXIT; EXIT; EXIT
+ *
+ * we will see the first "EXIT", issue warnings for the second and
+ * third unreachable EXIT statements, and then return, so that we
+ * don't issue bogus "unreachable" warnings when we see the second
+ * EXIT.
+ *
+ * XXX: currently we walk the PL/PgSQL execution tree by hand. It
+ * would probably be worth refactoring this to use something akin to
+ * the tree walker infrastructure in the backend.
+ */
+static void
+check_stmts(PLpgSQL_stmts *stmts)
+{
+ int i;
+
+ for (i = 0; i < stmts->stmts_used; i++)
+ {
+ PLpgSQL_stmt *stmt = stmts->stmts[i];
+
+ switch (stmt->cmd_type)
+ {
+ case PLPGSQL_STMT_RETURN:
+ if (i + 1 < stmts->stmts_used)
+ {
+ report_unreachable_stmt(stmts->stmts[i+1], stmt);
+ return;
+ }
+ case PLPGSQL_STMT_EXIT:
+ {
+ /*
+ * If the EXIT statement has a conditional, it is
+ * not guaranteed to exit the loop, so don't issue
+ * a warning.
+ */
+ PLpgSQL_stmt_exit *exit_stmt = (PLpgSQL_stmt_exit *) stmt;
+ if (exit_stmt->cond == NULL && i + 1 < stmts->stmts_used)
+ {
+ report_unreachable_stmt(stmts->stmts[i+1], stmt);
+ return;
+ }
+ }
+ break;
+ case PLPGSQL_STMT_RAISE:
+ {
+ PLpgSQL_stmt_raise *raise_stmt = (PLpgSQL_stmt_raise *) stmt;
+ /*
+ * Only RAISE EXCEPTION (converted to elog_level =
+ * ERROR by the parser) will exit the current
+ * block.
+ */
+ if (raise_stmt->elog_level == ERROR && i + 1 < stmts->stmts_used)
+ {
+ report_unreachable_stmt(stmts->stmts[i+1], stmt);
+ return;
+ }
+ }
+ break;
+ case PLPGSQL_STMT_BLOCK:
+ {
+ PLpgSQL_stmt_block *block_stmt = (PLpgSQL_stmt_block *) stmt;
+ check_stmts(block_stmt->body);
+
+ if (block_stmt->exceptions)
+ {
+ PLpgSQL_exceptions *exceptions;
+ int j;
+
+ exceptions = block_stmt->exceptions;
+ for (j = 0; j < exceptions->exceptions_used; j++)
+ check_stmts(exceptions->exceptions[j]->action);
+ }
+ }
+ break;
+ case PLPGSQL_STMT_IF:
+ check_stmts(((PLpgSQL_stmt_if *) stmt)->true_body);
+ check_stmts(((PLpgSQL_stmt_if *) stmt)->false_body);
+ break;
+ case PLPGSQL_STMT_LOOP:
+ check_stmts(((PLpgSQL_stmt_loop *) stmt)->body);
+ break;
+ case PLPGSQL_STMT_WHILE:
+ check_stmts(((PLpgSQL_stmt_while *) stmt)->body);
+ break;
+ case PLPGSQL_STMT_FORI:
+ check_stmts(((PLpgSQL_stmt_fori *) stmt)->body);
+ break;
+ case PLPGSQL_STMT_FORS:
+ {
+ PLpgSQL_stmt_fors *fors_stmt = (PLpgSQL_stmt_fors *) stmt;
+ check_sql_statement(fors_stmt->query->query);
+ check_stmts(fors_stmt->body);
+ }
+ break;
+ case PLPGSQL_STMT_DYNFORS:
+ check_stmts(((PLpgSQL_stmt_dynfors *) stmt)->body);
+ break;
+ case PLPGSQL_STMT_EXECSQL:
+ check_sql_statement(((PLpgSQL_stmt_execsql *) stmt)->sqlstmt->query);
+ break;
+ default:
+ /* do nothing */
+ break;
+ }
+ }
+}
+
+/*
+ * Issue warnings about various ill-advised constructs in the function
+ * body. We don't do very many compile-time checks at the moment, but
+ * a few is better than none...
+ */
+static void
+check_function(PLpgSQL_function *function)
+{
+ check_stmts(function->action->body);
+}
+
+/*
* Fetch the argument names, if any, from the proargnames field of the
* pg_proc tuple. Results are palloc'd.
*/
--- src/test/regress/expected/plpgsql.out
+++ src/test/regress/expected/plpgsql.out
@@ -2032,3 +2032,153 @@
DETAIL: Key (f1)=(2) is not present in table "master".
drop function trap_foreign_key(int);
drop function trap_foreign_key_2();
+--
+-- issue warnings about unreachable code
+--
+create function exit_warn() returns int as $$
+declare x int;
+begin
+ x := 5;
+ loop
+ x := x + 1;
+ exit;
+ x := x + 2;
+ end loop;
+ x := x + 3;
+ return x;
+end;$$ language 'plpgsql';
+WARNING: assignment is unreachable, due to exit near line 6
+CONTEXT: compile of PL/pgSQL function "exit_warn" near line 7
+create function exit_no_warn() returns int as $$
+declare x int;
+begin
+ x := 5;
+ loop
+ x := x + 5;
+ exit when x > 20;
+ x := x - 1;
+ end loop;
+ x := x + 3;
+ return x;
+end;$$ language 'plpgsql';
+create function return_warn() returns int as $$
+declare
+ a int;
+ b int;
+begin
+ a := 3;
+ b := 2;
+ if a > b then
+ return 5;
+ begin
+ return 10;
+ end;
+ end if;
+ return 15;
+end;$$ language 'plpgsql';
+WARNING: block variables initialization is unreachable, due to return near line 8
+CONTEXT: compile of PL/pgSQL function "return_warn" near line 9
+create function return_warn2() returns int as $$
+begin
+ if 5 > 5 then
+ return 5;
+ return 10;
+ end if;
+ return 10;
+ return 15;
+ return 20;
+end;$$ language 'plpgsql';
+WARNING: return is unreachable, due to return near line 3
+CONTEXT: compile of PL/pgSQL function "return_warn2" near line 4
+WARNING: return is unreachable, due to return near line 6
+CONTEXT: compile of PL/pgSQL function "return_warn2" near line 7
+create function raise_warn() returns int as $$
+declare x int;
+begin
+ x := 10;
+ begin
+ raise exception 'some random exception';
+ return x;
+ exception
+ when RAISE_EXCEPTION then NULL;
+ end;
+
+ begin
+ raise exception 'foo';
+ exception
+ when RAISE_EXCEPTION then
+ return x;
+ x := x + 1;
+ end;
+
+ begin
+ raise notice 'not an exception';
+ return x;
+ end;
+end;$$ language 'plpgsql';
+WARNING: return is unreachable, due to raise near line 5
+CONTEXT: compile of PL/pgSQL function "raise_warn" near line 6
+WARNING: assignment is unreachable, due to return near line 15
+CONTEXT: compile of PL/pgSQL function "raise_warn" near line 16
+-- note that we only want to emit warnings at CREATE FUNCTION time, not
+-- when the function is invoked.
+SELECT exit_warn();
+ exit_warn
+-----------
+ 9
+(1 row)
+
+SELECT return_warn();
+ return_warn
+-------------
+ 5
+(1 row)
+
+SELECT return_warn2();
+ return_warn2
+--------------
+ 10
+(1 row)
+
+SELECT raise_warn();
+ raise_warn
+------------
+ 10
+(1 row)
+
+--
+-- reject function definitions that contain malformed SQL queries at
+-- compile-time, where possible
+--
+create function bad_sql1() returns int as $$
+declare a int;
+begin
+ a := 5;
+ Johnny Yuma;
+ a := 10;
+ return a;
+end$$ language 'plpgsql';
+ERROR: syntax error at or near "Johnny" at character 45
+LINE 1: create function bad_sql1() returns int as $$
+ ^
+create function bad_sql2() returns int as $$
+declare r record;
+begin
+ for r in I fought the law, the law won LOOP
+ raise notice 'in loop';
+ end loop;
+ return 5;
+end;$$ language 'plpgsql';
+ERROR: syntax error at or near "I" at character 46
+LINE 2: declare r record;
+ ^
+create function bad_sql3() returns int as $$
+declare x int;
+begin
+ return 10;
+ -- report syntax errors even in unreachable code
+ ekgekrgkjge djngrg;
+ return 15;
+end;$$ language 'plpgsql';
+WARNING: SQL statement is unreachable, due to return near line 3
+CONTEXT: compile of PL/pgSQL function "bad_sql3" near line 5
--- src/test/regress/sql/plpgsql.sql
+++ src/test/regress/sql/plpgsql.sql
@@ -1764,3 +1764,123 @@

drop function trap_foreign_key(int);
drop function trap_foreign_key_2();
+
+--
+-- issue warnings about unreachable code
+--
+create function exit_warn() returns int as $$
+declare x int;
+begin
+ x := 5;
+ loop
+ x := x + 1;
+ exit;
+ x := x + 2;
+ end loop;
+ x := x + 3;
+ return x;
+end;$$ language 'plpgsql';
+
+create function exit_no_warn() returns int as $$
+declare x int;
+begin
+ x := 5;
+ loop
+ x := x + 5;
+ exit when x > 20;
+ x := x - 1;
+ end loop;
+ x := x + 3;
+ return x;
+end;$$ language 'plpgsql';
+
+create function return_warn() returns int as $$
+declare
+ a int;
+ b int;
+begin
+ a := 3;
+ b := 2;
+ if a > b then
+ return 5;
+ begin
+ return 10;
+ end;
+ end if;
+ return 15;
+end;$$ language 'plpgsql';
+
+create function return_warn2() returns int as $$
+begin
+ if 5 > 5 then
+ return 5;
+ return 10;
+ end if;
+ return 10;
+ return 15;
+ return 20;
+end;$$ language 'plpgsql';
+
+create function raise_warn() returns int as $$
+declare x int;
+begin
+ x := 10;
+ begin
+ raise exception 'some random exception';
+ return x;
+ exception
+ when RAISE_EXCEPTION then NULL;
+ end;
+
+ begin
+ raise exception 'foo';
+ exception
+ when RAISE_EXCEPTION then
+ return x;
+ x := x + 1;
+ end;
+
+ begin
+ raise notice 'not an exception';
+ return x;
+ end;
+end;$$ language 'plpgsql';
+
+-- note that we only want to emit warnings at CREATE FUNCTION time, not
+-- when the function is invoked.
+SELECT exit_warn();
+SELECT return_warn();
+SELECT return_warn2();
+SELECT raise_warn();
+
+--
+-- reject function definitions that contain malformed SQL queries at
+-- compile-time, where possible
+--
+
+create function bad_sql1() returns int as $$
+declare a int;
+begin
+ a := 5;
+ Johnny Yuma;
+ a := 10;
+ return a;
+end$$ language 'plpgsql';
+
+create function bad_sql2() returns int as $$
+declare r record;
+begin
+ for r in I fought the law, the law won LOOP
+ raise notice 'in loop';
+ end loop;
+ return 5;
+end;$$ language 'plpgsql';
+
+create function bad_sql3() returns int as $$
+declare x int;
+begin
+ return 10;
+ -- report syntax errors even in unreachable code
+ ekgekrgkjge djngrg;
+ return 15;
+end;$$ language 'plpgsql';

--=-+nCK3IF5DcEUvPFvVfzt
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faqs/FAQ.html

--=-+nCK3IF5DcEUvPFvVfzt--



Reply With Quote
  #6  
Old   
Tom Lane
 
Posts: n/a

Default Re: [BUGS] BUG #1329: Bug in IF-ELSEIF-ELSE construct - 11-29-2004 , 11:05 AM



Neil Conway <neilc (AT) samurai (DOT) com> writes:
Quote:
(2) The syntax error message is wrong (we print a character offset and
query context that is relative to the CREATE FUNCTION statement, not the
individual SQL statement we're executing). I fooled around a bit with
defining a custom ereport() callback to print the right line number and
query context, but I couldn't get it right. Do you have any guidance on
the proper way to do this.
Hmm ... I was about to say the SQL function validator already does this
(see sql_function_parse_error_callback in pg_proc.c), but it has the
advantage that there's a one-to-one correspondence between the string it
sends to the main parser and a substring of the original function text.
In plpgsql that's not true because of (a) substitution of parameter
symbols for names and (b) the liberties that the plpgsql lexer takes
with whitespace and eliding comments.

You might be best off just to strive for output like this:

ERROR: syntax error at or near...
QUERY: select ...
CONTEXT: compile of plpgsql function "frob" at SQL statement line 12

which ought to be relatively easy to get.

BTW, don't forget to check SQL expressions (eg, the condition of an IF)
as well as SQL statements. In the case of EXECUTE, you can check
the expression that gives rise to the text string.

Quote:
The error would be more useful if reported immediately
after the putative SQL statement is parsed. Not sure if that's
hard or not. (The same remark applies to dead code checking, now
that I think about it.)

In the case of dead code checking, I don't think it matters.
My thought was that a dead-code error could well indicate a problem
along the lines of a missing begin or end, and that flagging the
dead-code error would probably provide a much closer pointer to the
true problem than barfing at the end of the input when we realize
we have unmatched begin/end structure. (Especially since the barf
will likely take the form of a nonspecific "syntax error" message.
Anytime you can do better than that, you're ahead.) Similarly, checking
expressions immediately will help report mismatched-parenthesis problems
in a more useful way than we do now.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster


Reply With Quote
  #7  
Old   
Tom Lane
 
Posts: n/a

Default Re: [BUGS] BUG #1329: Bug in IF-ELSEIF-ELSE construct - 12-21-2004 , 12:59 PM



Awhile back I wrote:
Quote:
As long as you only do this in check_function_bodies mode it seems
safe enough. One possible problem (if it's done towards the end of
parsing as you've suggested for dead-code checking) is that a syntax
error in a SQL statement might confuse the plpgsql parser into
misparsing statement boundaries, which could lead to a plpgsql parse
error much further down, such as a "missing END" at the end of the
function. The error would be more useful if reported immediately
after the putative SQL statement is parsed. Not sure if that's
hard or not. (The same remark applies to dead code checking, now
that I think about it.)
A real-world example of why this would be useful can be seen at
http://archives.postgresql.org/pgsql...2/msg00223.php

The problem is a missing semicolon just before an IF construct. If
the putative PERFORM were SQL-parsed right away, the user could see
what had been taken as the body of the PERFORM and would be able to
figure out his mistake. If we continue plpgsql-parsing it's very
hard to see how we avoid generating an error that leads the user
to look in the wrong place.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faqs/FAQ.html


Reply With Quote
Reply




Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off



Powered by vBulletin Version 3.5.3
Copyright ©2000 - 2012, Jelsoft Enterprises Ltd.