Extra warnings and errors for PL/pgSQL
authorSimon Riggs <simon@2ndQuadrant.com>
Sun, 6 Apr 2014 16:21:51 +0000 (12:21 -0400)
committerSimon Riggs <simon@2ndQuadrant.com>
Sun, 6 Apr 2014 16:21:51 +0000 (12:21 -0400)
Infrastructure to allow
 plpgsql.extra_warnings
 plpgsql.extra_errors

Initial extra checks only for shadowed_variables

Marko Tiikkaja and Petr Jelinek
Reviewed by Simon Riggs and Pavel StÄ›hule

doc/src/sgml/plpgsql.sgml
src/pl/plpgsql/src/pl_comp.c
src/pl/plpgsql/src/pl_gram.y
src/pl/plpgsql/src/pl_handler.c
src/pl/plpgsql/src/plpgsql.h
src/test/regress/expected/plpgsql.out
src/test/regress/sql/plpgsql.sql

index bddd4585283af6510dc52d1d384b7b0d7373e816..a549a24eaef244b6991c16b2065aba5f70d56062 100644 (file)
@@ -4711,6 +4711,56 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
   </variablelist>
 
   </sect2>
+  <sect2 id="plpgsql-extra-checks">
+   <title>Additional compile-time checks</title>
+
+   <para>
+    To aid the user in finding instances of simple but common problems before
+    they cause harm, <application>PL/PgSQL</> provides additional
+    <replaceable>checks</>. When enabled, depending on the configuration, they
+    can be used to emit either a <literal>WARNING</> or an <literal>ERROR</>
+    during the compilation of a function. A function which has received
+    a <literal>WARNING</> can be executed without producing further messages,
+    so you are advised to test in a separate development environment.
+   </para>
+
+ <para>
+  These additional checks are enabled through the configuration variables
+  <varname>plpgsql.extra_warnings</> for warnings and 
+  <varname>plpgsql.extra_errors</> for errors. Both can be set either to
+  a comma-separated list of checks, <literal>"none"</> or <literal>"all"</>.
+  The default is <literal>"none"</>. Currently the list of available checks
+  includes only one:
+  <variablelist>
+   <varlistentry>
+    <term><varname>shadowed_variables</varname></term>
+    <listitem>
+     <para>
+      Checks if a declaration shadows a previously defined variable. 
+     </para>
+    </listitem>
+   </varlistentry>
+  </variablelist>
+
+  The following example shows the effect of <varname>plpgsql.extra_warnings</>
+  set to <varname>shadowed_variables</>:
+<programlisting>
+SET plpgsql.extra_warnings TO 'shadowed_variables';
+
+CREATE FUNCTION foo(f1 int) RETURNS int AS $$
+DECLARE
+f1 int;
+BEGIN
+RETURN f1;
+END
+$$ LANGUAGE plpgsql;
+WARNING:  variable "f1" shadows a previously defined variable
+LINE 3: f1 int;
+        ^
+CREATE FUNCTION
+</programlisting>
+ </para>
+ </sect2>
  </sect1>
 
   <!-- **** Porting from Oracle PL/SQL **** -->
index 5afc2e500441c935e755b29bedb5b8e0271aed4d..12ac964d13190272d78e4326c97adaa1652e2d7b 100644 (file)
@@ -352,6 +352,9 @@ do_compile(FunctionCallInfo fcinfo,
        function->out_param_varno = -1;         /* set up for no OUT param */
        function->resolve_option = plpgsql_variable_conflict;
        function->print_strict_params = plpgsql_print_strict_params;
+       /* only promote extra warnings and errors at CREATE FUNCTION time */
+       function->extra_warnings = forValidator ? plpgsql_extra_warnings : 0;
+       function->extra_errors = forValidator ? plpgsql_extra_errors : 0;
 
        if (is_dml_trigger)
                function->fn_is_trigger = PLPGSQL_DML_TRIGGER;
@@ -849,6 +852,9 @@ plpgsql_compile_inline(char *proc_source)
        function->out_param_varno = -1;         /* set up for no OUT param */
        function->resolve_option = plpgsql_variable_conflict;
        function->print_strict_params = plpgsql_print_strict_params;
+       /* don't do extra validation for inline code as we don't want to add spam at runtime */
+       function->extra_warnings = 0;
+       function->extra_errors = 0;
 
        plpgsql_ns_init();
        plpgsql_ns_push(func_name);
index c0cb58530bea8d84095f475de63f087045251538..91186c6b9aefd47bb7b7059d460326ebc00eec1f 100644 (file)
@@ -727,6 +727,21 @@ decl_varname       : T_WORD
                                                                                          $1.ident, NULL, NULL,
                                                                                          NULL) != NULL)
                                                        yyerror("duplicate declaration");
+
+                                               if (plpgsql_curr_compile->extra_warnings & PLPGSQL_XCHECK_SHADOWVAR ||
+                                                       plpgsql_curr_compile->extra_errors & PLPGSQL_XCHECK_SHADOWVAR)
+                                               {
+                                                       PLpgSQL_nsitem *nsi;
+                                                       nsi = plpgsql_ns_lookup(plpgsql_ns_top(), false,
+                                                                                                       $1.ident, NULL, NULL, NULL);
+                                                       if (nsi != NULL)
+                                                               ereport(plpgsql_curr_compile->extra_errors & PLPGSQL_XCHECK_SHADOWVAR ? ERROR : WARNING,
+                                                                               (errcode(ERRCODE_DUPLICATE_ALIAS),
+                                                                                errmsg("variable \"%s\" shadows a previously defined variable",
+                                                                                               $1.ident),
+                                                                                parser_errposition(@1)));
+                                               }
+
                                        }
                                | unreserved_keyword
                                        {
@@ -740,6 +755,21 @@ decl_varname       : T_WORD
                                                                                          $1, NULL, NULL,
                                                                                          NULL) != NULL)
                                                        yyerror("duplicate declaration");
+
+                                               if (plpgsql_curr_compile->extra_warnings & PLPGSQL_XCHECK_SHADOWVAR ||
+                                                       plpgsql_curr_compile->extra_errors & PLPGSQL_XCHECK_SHADOWVAR)
+                                               {
+                                                       PLpgSQL_nsitem *nsi;
+                                                       nsi = plpgsql_ns_lookup(plpgsql_ns_top(), false,
+                                                                                                       $1, NULL, NULL, NULL);
+                                                       if (nsi != NULL)
+                                                               ereport(plpgsql_curr_compile->extra_errors & PLPGSQL_XCHECK_SHADOWVAR ? ERROR : WARNING,
+                                                                               (errcode(ERRCODE_DUPLICATE_ALIAS),
+                                                                                errmsg("variable \"%s\" shadows a previously defined variable",
+                                                                                               $1),
+                                                                                parser_errposition(@1)));
+                                               }
+
                                        }
                                ;
 
index f21393ae41d5fee706a06266086ff509f024c413..e659f8e289a1841715c69bab4298668a7f6f118a 100644 (file)
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
 
+
+static bool plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source);
+static void plpgsql_extra_warnings_assign_hook(const char *newvalue, void *extra);
+static void plpgsql_extra_errors_assign_hook(const char *newvalue, void *extra);
+
 PG_MODULE_MAGIC;
 
 /* Custom GUC variable */
@@ -39,10 +44,89 @@ int                 plpgsql_variable_conflict = PLPGSQL_RESOLVE_ERROR;
 
 bool           plpgsql_print_strict_params = false;
 
+char           *plpgsql_extra_warnings_string = NULL;
+char           *plpgsql_extra_errors_string = NULL;
+int                    plpgsql_extra_warnings;
+int                    plpgsql_extra_errors;
+
 /* Hook for plugins */
 PLpgSQL_plugin **plugin_ptr = NULL;
 
 
+static bool
+plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source)
+{
+       char       *rawstring;
+       List       *elemlist;
+       ListCell   *l;
+       int                 extrachecks = 0;
+       int                *myextra;
+
+       if (pg_strcasecmp(*newvalue, "all") == 0)
+               extrachecks = PLPGSQL_XCHECK_ALL;
+       else if (pg_strcasecmp(*newvalue, "none") == 0)
+               extrachecks = PLPGSQL_XCHECK_NONE;
+       else
+       {
+               /* Need a modifiable copy of string */
+               rawstring = pstrdup(*newvalue);
+
+               /* Parse string into list of identifiers */
+               if (!SplitIdentifierString(rawstring, ',', &elemlist))
+               {
+                       /* syntax error in list */
+                       GUC_check_errdetail("List syntax is invalid.");
+                       pfree(rawstring);
+                       list_free(elemlist);
+                       return false;
+               }
+
+               foreach(l, elemlist)
+               {
+                       char       *tok = (char *) lfirst(l);
+
+                       if (pg_strcasecmp(tok, "shadowed_variables") == 0)
+                               extrachecks |= PLPGSQL_XCHECK_SHADOWVAR;
+                       else if (pg_strcasecmp(tok, "all") == 0 || pg_strcasecmp(tok, "none") == 0)
+                       {
+                               GUC_check_errdetail("Key word \"%s\" cannot be combined with other key words.", tok);
+                               pfree(rawstring);
+                               list_free(elemlist);
+                               return false;
+                       }
+                       else
+                       {
+                               GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+                               pfree(rawstring);
+                               list_free(elemlist);
+                               return false;
+                       }
+               }
+
+               pfree(rawstring);
+               list_free(elemlist);
+       }
+
+       myextra = (int *) malloc(sizeof(int));
+       *myextra = extrachecks;
+       *extra = (void *) myextra;
+
+       return true;
+}
+
+static void
+plpgsql_extra_warnings_assign_hook(const char *newvalue, void *extra)
+{
+       plpgsql_extra_warnings = *((int *) extra);
+}
+
+static void
+plpgsql_extra_errors_assign_hook(const char *newvalue, void *extra)
+{
+       plpgsql_extra_errors = *((int *) extra);
+}
+
+
 /*
  * _PG_init()                  - library load-time initialization
  *
@@ -76,6 +160,26 @@ _PG_init(void)
                                                         PGC_USERSET, 0,
                                                         NULL, NULL, NULL);
 
+       DefineCustomStringVariable("plpgsql.extra_warnings",
+                                                          gettext_noop("List of programming constructs which should produce a warning."),
+                                                          NULL,
+                                                          &plpgsql_extra_warnings_string,
+                                                          "none",
+                                                          PGC_USERSET, GUC_LIST_INPUT,
+                                                          plpgsql_extra_checks_check_hook,
+                                                          plpgsql_extra_warnings_assign_hook,
+                                                          NULL);
+
+       DefineCustomStringVariable("plpgsql.extra_errors",
+                                                          gettext_noop("List of programming constructs which should produce an error."),
+                                                          NULL,
+                                                          &plpgsql_extra_errors_string,
+                                                          "none",
+                                                          PGC_USERSET, GUC_LIST_INPUT,
+                                                          plpgsql_extra_checks_check_hook,
+                                                          plpgsql_extra_errors_assign_hook,
+                                                          NULL);
+
        EmitWarningsOnPlaceholders("plpgsql");
 
        plpgsql_HashTableInit();
index f3d1283015d6853b270412101b0492ab613d849b..41fc9407a22c3144bc9435ed55b6245353279867 100644 (file)
@@ -739,6 +739,10 @@ typedef struct PLpgSQL_function
 
        bool            print_strict_params;
 
+       /* extra checks */
+       int             extra_warnings;
+       int             extra_errors;
+
        int                     ndatums;
        PLpgSQL_datum **datums;
        PLpgSQL_stmt_block *action;
@@ -881,6 +885,14 @@ extern int plpgsql_variable_conflict;
 
 extern bool plpgsql_print_strict_params;
 
+/* extra compile-time checks */
+#define PLPGSQL_XCHECK_NONE                    0
+#define PLPGSQL_XCHECK_SHADOWVAR       1
+#define PLPGSQL_XCHECK_ALL                     ((int) ~0)
+
+extern int plpgsql_extra_warnings;
+extern int plpgsql_extra_errors;
+
 extern bool plpgsql_check_syntax;
 extern bool plpgsql_DumpExecTree;
 
index 0405ef47dd87a0bb6287044120dda227bde10125..8892bb417d39a020c14b4f4d550f1bfa0c9597a3 100644 (file)
@@ -3208,6 +3208,128 @@ select footest();
 ERROR:  query returned more than one row
 DETAIL:  parameters: p1 = '2', p3 = 'foo'
 CONTEXT:  PL/pgSQL function footest() line 10 at SQL statement
+-- test warnings and errors
+set plpgsql.extra_warnings to 'all';
+set plpgsql.extra_warnings to 'none';
+set plpgsql.extra_errors to 'all';
+set plpgsql.extra_errors to 'none';
+-- test warnings when shadowing a variable
+set plpgsql.extra_warnings to 'shadowed_variables';
+-- simple shadowing of input and output parameters
+create or replace function shadowtest(in1 int)
+       returns table (out1 int) as $$
+declare
+in1 int;
+out1 int;
+begin
+end
+$$ language plpgsql;
+WARNING:  variable "in1" shadows a previously defined variable
+LINE 4: in1 int;
+        ^
+WARNING:  variable "out1" shadows a previously defined variable
+LINE 5: out1 int;
+        ^
+select shadowtest(1);
+ shadowtest 
+------------
+(0 rows)
+
+set plpgsql.extra_warnings to 'shadowed_variables';
+select shadowtest(1);
+ shadowtest 
+------------
+(0 rows)
+
+create or replace function shadowtest(in1 int)
+       returns table (out1 int) as $$
+declare
+in1 int;
+out1 int;
+begin
+end
+$$ language plpgsql;
+WARNING:  variable "in1" shadows a previously defined variable
+LINE 4: in1 int;
+        ^
+WARNING:  variable "out1" shadows a previously defined variable
+LINE 5: out1 int;
+        ^
+select shadowtest(1);
+ shadowtest 
+------------
+(0 rows)
+
+drop function shadowtest(int);
+-- shadowing in a second DECLARE block
+create or replace function shadowtest()
+       returns void as $$
+declare
+f1 int;
+begin
+       declare
+       f1 int;
+       begin
+       end;
+end$$ language plpgsql;
+WARNING:  variable "f1" shadows a previously defined variable
+LINE 7:  f1 int;
+         ^
+drop function shadowtest();
+-- several levels of shadowing
+create or replace function shadowtest(in1 int)
+       returns void as $$
+declare
+in1 int;
+begin
+       declare
+       in1 int;
+       begin
+       end;
+end$$ language plpgsql;
+WARNING:  variable "in1" shadows a previously defined variable
+LINE 4: in1 int;
+        ^
+WARNING:  variable "in1" shadows a previously defined variable
+LINE 7:  in1 int;
+         ^
+drop function shadowtest(int);
+-- shadowing in cursor definitions
+create or replace function shadowtest()
+       returns void as $$
+declare
+f1 int;
+c1 cursor (f1 int) for select 1;
+begin
+end$$ language plpgsql;
+WARNING:  variable "f1" shadows a previously defined variable
+LINE 5: c1 cursor (f1 int) for select 1;
+                   ^
+drop function shadowtest();
+-- test errors when shadowing a variable
+set plpgsql.extra_errors to 'shadowed_variables';
+create or replace function shadowtest(f1 int)
+       returns boolean as $$
+declare f1 int; begin return 1; end $$ language plpgsql;
+ERROR:  variable "f1" shadows a previously defined variable
+LINE 3: declare f1 int; begin return 1; end $$ language plpgsql;
+                ^
+select shadowtest(1);
+ERROR:  function shadowtest(integer) does not exist
+LINE 1: select shadowtest(1);
+               ^
+HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
+reset plpgsql.extra_errors;
+reset plpgsql.extra_warnings;
+create or replace function shadowtest(f1 int)
+       returns boolean as $$
+declare f1 int; begin return 1; end $$ language plpgsql;
+select shadowtest(1);
+ shadowtest 
+------------
+ t
+(1 row)
+
 -- test scrollable cursor support
 create function sc_test() returns setof integer as $$
 declare
index d01011a85a7082824306a7bbacd01803560165a0..c5616ee8fc638b68fa6bc60a4d9fcc6ac7cec11e 100644 (file)
@@ -2689,6 +2689,95 @@ end$$ language plpgsql;
 
 select footest();
 
+-- test warnings and errors
+set plpgsql.extra_warnings to 'all';
+set plpgsql.extra_warnings to 'none';
+set plpgsql.extra_errors to 'all';
+set plpgsql.extra_errors to 'none';
+
+-- test warnings when shadowing a variable
+
+set plpgsql.extra_warnings to 'shadowed_variables';
+
+-- simple shadowing of input and output parameters
+create or replace function shadowtest(in1 int)
+       returns table (out1 int) as $$
+declare
+in1 int;
+out1 int;
+begin
+end
+$$ language plpgsql;
+select shadowtest(1);
+
+set plpgsql.extra_warnings to 'shadowed_variables';
+select shadowtest(1);
+create or replace function shadowtest(in1 int)
+       returns table (out1 int) as $$
+declare
+in1 int;
+out1 int;
+begin
+end
+$$ language plpgsql;
+select shadowtest(1);
+drop function shadowtest(int);
+
+-- shadowing in a second DECLARE block
+create or replace function shadowtest()
+       returns void as $$
+declare
+f1 int;
+begin
+       declare
+       f1 int;
+       begin
+       end;
+end$$ language plpgsql;
+drop function shadowtest();
+
+-- several levels of shadowing
+create or replace function shadowtest(in1 int)
+       returns void as $$
+declare
+in1 int;
+begin
+       declare
+       in1 int;
+       begin
+       end;
+end$$ language plpgsql;
+drop function shadowtest(int);
+
+-- shadowing in cursor definitions
+create or replace function shadowtest()
+       returns void as $$
+declare
+f1 int;
+c1 cursor (f1 int) for select 1;
+begin
+end$$ language plpgsql;
+drop function shadowtest();
+
+-- test errors when shadowing a variable
+
+set plpgsql.extra_errors to 'shadowed_variables';
+
+create or replace function shadowtest(f1 int)
+       returns boolean as $$
+declare f1 int; begin return 1; end $$ language plpgsql;
+
+select shadowtest(1);
+
+reset plpgsql.extra_errors;
+reset plpgsql.extra_warnings;
+
+create or replace function shadowtest(f1 int)
+       returns boolean as $$
+declare f1 int; begin return 1; end $$ language plpgsql;
+
+select shadowtest(1);
+
 -- test scrollable cursor support
 
 create function sc_test() returns setof integer as $$