On Sat, 2004-11-27 at 12:55 -0500, Tom Lane wrote:
> 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.
> 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
#
# 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_con
