--=-+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--