Hi everyone,

Here's a new version of the patch. Added new tests and docs and changed the GUCs per discussion.


plpgsql.warnings_as_errors only affects compilation at CREATE FUNCTION time:

=# set plpgsql.warnings to 'all';
SET
=#* set plpgsql.warnings_as_errors to true;
SET
=#* select foof(1); -- compiled since it's the first call in this session
WARNING:  variable "f1" shadows a previously defined variable
LINE 2: declare f1 int;
                ^
 foof
------

(1 row)

=#* create or replace function foof(f1 int) returns void as
$$
declare f1 int;
begin
end $$ language plpgsql;
ERROR:  variable "f1" shadows a previously defined variable
LINE 3: declare f1 int;


Currently, plpgsql_warnings is a boolean since there's only one warning we implement. The idea is to make it a bit field of some kind in the future when we add more warnings. Starting that work for 9.4 seemed like overkill, though. I tried to keep things simple.


Regards,
Marko Tiikkaja
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
***************
*** 4712,4717 **** a_output := a_output || $$ if v_$$ || referrer_keys.kind || 
$$ like '$$
--- 4712,4762 ----
    </variablelist>
  
    </sect2>
+   <sect2 id="plpgsql-warnings">
+    <title>Warnings</title>
+ 
+    <para>
+     To aid the user in finding instances of simple but common problems before
+     they cause harm, <application>PL/PgSQL</> provides a number of
+     <replaceable>warnings</>.  When enabled, a <literal>WARNING</> is emitted
+     during the compilation of a function.  Optionally, if
+     <varname>plpgsql.warnings_as_errors</> is set, an <literal>ERROR</> is
+     raised when attempting to create a function which would otherwise result 
in
+     a warning.
+    </para>
+ 
+  <para>
+   Warnings are enabled through the configuration variable
+   <varname>plpgsql.warnings</>.  It can be set either to a comma-separated 
list
+   of warnings, <literal>"none"</> or <literal>"all"</>.  The default is
+   <literal>"none"</>.  Currently the list of available warnings includes only
+   one:
+   <variablelist>
+    <varlistentry>
+     <term><varname>shadow</varname></term>
+     <listitem>
+      <para>
+       Warns when a declaration shadows a previously defined variable.  For
+       example:
+ <programlisting>
+ 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>
+     </listitem>
+    </varlistentry>
+   </variablelist>
+  </para>
+  </sect2>
   </sect1>
  
    <!-- **** Porting from Oracle PL/SQL **** -->
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
***************
*** 352,357 **** do_compile(FunctionCallInfo fcinfo,
--- 352,360 ----
        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;
+       function->warnings = plpgsql_warnings;
+       /* only promote warnings to errors at CREATE FUNCTION time */
+       function->warnings_as_errors = plpgsql_warnings_as_errors && 
forValidator;
  
        if (is_dml_trigger)
                function->fn_is_trigger = PLPGSQL_DML_TRIGGER;
***************
*** 849,854 **** plpgsql_compile_inline(char *proc_source)
--- 852,860 ----
        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;
+       function->warnings = plpgsql_warnings;
+       /* never promote warnings to errors */
+       function->warnings_as_errors = false;
  
        plpgsql_ns_init();
        plpgsql_ns_push(func_name);
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
***************
*** 727,732 **** decl_varname   : T_WORD
--- 727,746 ----
                                                                                
          $1.ident, NULL, NULL,
                                                                                
          NULL) != NULL)
                                                        yyerror("duplicate 
declaration");
+ 
+                                               if 
(plpgsql_curr_compile->warnings)
+                                               {
+                                                       PLpgSQL_nsitem *nsi;
+                                                       nsi = 
plpgsql_ns_lookup(plpgsql_ns_top(), false,
+                                                                               
                        $1.ident, NULL, NULL, NULL);
+                                                       if (nsi != NULL)
+                                                               
ereport(plpgsql_curr_compile->warnings_as_errors ? ERROR : WARNING,
+                                                                               
(errcode(ERRCODE_DUPLICATE_ALIAS),
+                                                                               
 errmsg("variable \"%s\" shadows a previously defined variable",
+                                                                               
                $1.ident),
+                                                                               
 parser_errposition(@1)));
+                                               }
+ 
                                        }
                                | unreserved_keyword
                                        {
***************
*** 740,745 **** decl_varname   : T_WORD
--- 754,773 ----
                                                                                
          $1, NULL, NULL,
                                                                                
          NULL) != NULL)
                                                        yyerror("duplicate 
declaration");
+ 
+                                               if 
(plpgsql_curr_compile->warnings)
+                                               {
+                                                       PLpgSQL_nsitem *nsi;
+                                                       nsi = 
plpgsql_ns_lookup(plpgsql_ns_top(), false,
+                                                                               
                        $1, NULL, NULL, NULL);
+                                                       if (nsi != NULL)
+                                                               
ereport(plpgsql_curr_compile->warnings_as_errors ? ERROR : WARNING,
+                                                                               
(errcode(ERRCODE_DUPLICATE_ALIAS),
+                                                                               
 errmsg("variable \"%s\" shadows a previously defined variable",
+                                                                               
                $1),
+                                                                               
 parser_errposition(@1)));
+                                               }
+ 
                                        }
                                ;
  
*** a/src/pl/plpgsql/src/pl_handler.c
--- b/src/pl/plpgsql/src/pl_handler.c
***************
*** 25,30 ****
--- 25,34 ----
  #include "utils/lsyscache.h"
  #include "utils/syscache.h"
  
+ 
+ static bool plpgsql_warnings_check_hook(char **newvalue, void **extra, 
GucSource source);
+ static void plpgsql_warnings_assign_hook(const char *newvalue, void *extra);
+ 
  PG_MODULE_MAGIC;
  
  /* Custom GUC variable */
***************
*** 39,48 **** int                      plpgsql_variable_conflict = 
PLPGSQL_RESOLVE_ERROR;
--- 43,76 ----
  
  bool          plpgsql_print_strict_params = false;
  
+ char     *plpgsql_warnings_list = NULL;
+ bool          plpgsql_warnings;
+ bool          plpgsql_warnings_as_errors;
+ 
  /* Hook for plugins */
  PLpgSQL_plugin **plugin_ptr = NULL;
  
  
+ static bool
+ plpgsql_warnings_check_hook(char **newvalue, void **extra, GucSource source)
+ {
+       if (strcmp(*newvalue, "all") == 0 ||
+               strcmp(*newvalue, "shadow") == 0 ||
+               strcmp(*newvalue, "none") == 0)
+               return true;
+       return false;
+ }
+ 
+ static void
+ plpgsql_warnings_assign_hook(const char *newvalue, void *extra)
+ {
+       if (strcmp(newvalue, "all") == 0 ||
+               strcmp(newvalue, "shadow") == 0)
+               plpgsql_warnings = true;
+       else
+               plpgsql_warnings = false;
+ }
+ 
  /*
   * _PG_init()                 - library load-time initialization
   *
***************
*** 76,81 **** _PG_init(void)
--- 104,127 ----
                                                         PGC_USERSET, 0,
                                                         NULL, NULL, NULL);
  
+       DefineCustomStringVariable("plpgsql.warnings",
+                                                          gettext_noop("List 
of programming constructs which should produce a warning."),
+                                                          NULL,
+                                                          
&plpgsql_warnings_list,
+                                                          "none",
+                                                          PGC_USERSET, 0,
+                                                          
plpgsql_warnings_check_hook,
+                                                          
plpgsql_warnings_assign_hook,
+                                                          NULL);
+ 
+       DefineCustomBoolVariable("plpgsql.warnings_as_errors",
+                                                        gettext_noop("If set, 
attempting to compile a function which would emit a warning will raise an error 
instead."),
+                                                        NULL,
+                                                        
&plpgsql_warnings_as_errors,
+                                                        false,
+                                                        PGC_USERSET, 0,
+                                                        NULL, NULL, NULL);
+ 
        EmitWarningsOnPlaceholders("plpgsql");
  
        plpgsql_HashTableInit();
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
***************
*** 739,744 **** typedef struct PLpgSQL_function
--- 739,748 ----
  
        bool            print_strict_params;
  
+       /* warnings */
+       bool            warnings;
+       bool            warnings_as_errors;
+ 
        int                     ndatums;
        PLpgSQL_datum **datums;
        PLpgSQL_stmt_block *action;
***************
*** 881,886 **** extern int     plpgsql_variable_conflict;
--- 885,893 ----
  
  extern bool plpgsql_print_strict_params;
  
+ extern bool plpgsql_warnings;
+ extern bool plpgsql_warnings_as_errors;
+ 
  extern bool plpgsql_check_syntax;
  extern bool plpgsql_DumpExecTree;
  
*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
***************
*** 3208,3213 **** select footest();
--- 3208,3286 ----
  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 when shadowing a variable
+ set plpgsql.warnings to 'shadow';
+ -- 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;
+         ^
+ 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 warnings_as_errors
+ set plpgsql.warnings_as_errors to 'on';
+ create or replace function shadowtest(f1 int)
+       returns void as $$
+ declare f1 int; begin end $$ language plpgsql;
+ ERROR:  variable "f1" shadows a previously defined variable
+ LINE 3: declare f1 int; begin end $$ language plpgsql;
+                 ^
+ reset plpgsql.warnings_as_errors;
+ reset plpgsql.warnings;
  -- test scrollable cursor support
  create function sc_test() returns setof integer as $$
  declare
*** a/src/test/regress/sql/plpgsql.sql
--- b/src/test/regress/sql/plpgsql.sql
***************
*** 2689,2694 **** end$$ language plpgsql;
--- 2689,2756 ----
  
  select footest();
  
+ -- test warnings when shadowing a variable
+ 
+ set plpgsql.warnings to 'shadow';
+ 
+ -- 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;
+ 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 warnings_as_errors
+ 
+ set plpgsql.warnings_as_errors to 'on';
+ 
+ create or replace function shadowtest(f1 int)
+       returns void as $$
+ declare f1 int; begin end $$ language plpgsql;
+ 
+ reset plpgsql.warnings_as_errors;
+ reset plpgsql.warnings;
+ 
  -- test scrollable cursor support
  
  create function sc_test() returns setof integer as $$
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to