On 19/03/14 09:45, Pavel Stehule wrote:
Hello
This patch introduce a possibility to implement some new checks without
impact to current code.
1. there is a common agreement about this functionality, syntax, naming
2. patching is clean, compilation is without error and warning
3. all regress tests passed
4. feature is well documented
5. code is clean, documented and respect out codding standards
Note: please, replace "shadowed-variables" by "shadowed_variables"
This patch is ready for commit
Thanks! Attached new version of the patch with the above change.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index bddd458..0582c91 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4711,6 +4711,51 @@ 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.
+ </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. For
+ example (with <varname>plpgsql.extra_warnings</> set to
+ <varname>shadowed_variables</varname>):
+<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 **** -->
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 5afc2e5..8732efc 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -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 = plpgsql_extra_warnings && forValidator;
+ function->extra_errors = plpgsql_extra_errors && forValidator;
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 = false;
+ function->extra_errors = false;
plpgsql_ns_init();
plpgsql_ns_push(func_name);
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index c0cb585..98f7ddd 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -727,6 +727,20 @@ decl_varname : T_WORD
$1.ident, NULL, NULL,
NULL) != NULL)
yyerror("duplicate declaration");
+
+ if (plpgsql_curr_compile->extra_warnings || plpgsql_curr_compile->extra_errors)
+ {
+ 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 ? ERROR : WARNING,
+ (errcode(ERRCODE_DUPLICATE_ALIAS),
+ errmsg("variable \"%s\" shadows a previously defined variable",
+ $1.ident),
+ parser_errposition(@1)));
+ }
+
}
| unreserved_keyword
{
@@ -740,6 +754,20 @@ decl_varname : T_WORD
$1, NULL, NULL,
NULL) != NULL)
yyerror("duplicate declaration");
+
+ if (plpgsql_curr_compile->extra_warnings || plpgsql_curr_compile->extra_errors)
+ {
+ PLpgSQL_nsitem *nsi;
+ nsi = plpgsql_ns_lookup(plpgsql_ns_top(), false,
+ $1, NULL, NULL, NULL);
+ if (nsi != NULL)
+ ereport(plpgsql_curr_compile->extra_errors ? ERROR : WARNING,
+ (errcode(ERRCODE_DUPLICATE_ALIAS),
+ errmsg("variable \"%s\" shadows a previously defined variable",
+ $1),
+ parser_errposition(@1)));
+ }
+
}
;
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index f21393a..1ce6554 100644
--- a/src/pl/plpgsql/src/pl_handler.c
+++ b/src/pl/plpgsql/src/pl_handler.c
@@ -25,6 +25,11 @@
#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,45 @@ int plpgsql_variable_conflict = PLPGSQL_RESOLVE_ERROR;
bool plpgsql_print_strict_params = false;
+char *plpgsql_extra_warnings_list = NULL;
+char *plpgsql_extra_errors_list = NULL;
+bool plpgsql_extra_warnings;
+bool plpgsql_extra_errors;
+
/* Hook for plugins */
PLpgSQL_plugin **plugin_ptr = NULL;
+static bool
+plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source)
+{
+ if (strcmp(*newvalue, "all") == 0 ||
+ strcmp(*newvalue, "shadowed_variables") == 0 ||
+ strcmp(*newvalue, "none") == 0)
+ return true;
+ return false;
+}
+
+static void
+plpgsql_extra_warnings_assign_hook(const char *newvalue, void *extra)
+{
+ if (strcmp(newvalue, "all") == 0 ||
+ strcmp(newvalue, "shadowed_variables") == 0)
+ plpgsql_extra_warnings = true;
+ else
+ plpgsql_extra_warnings = false;
+}
+
+static void
+plpgsql_extra_errors_assign_hook(const char *newvalue, void *extra)
+{
+ if (strcmp(newvalue, "all") == 0 ||
+ strcmp(newvalue, "shadowed_variables") == 0)
+ plpgsql_extra_errors = true;
+ else
+ plpgsql_extra_errors = false;
+}
+
/*
* _PG_init() - library load-time initialization
*
@@ -76,6 +116,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_list,
+ "none",
+ PGC_USERSET, 0,
+ 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_list,
+ "none",
+ PGC_USERSET, 0,
+ plpgsql_extra_checks_check_hook,
+ plpgsql_extra_errors_assign_hook,
+ NULL);
+
EmitWarningsOnPlaceholders("plpgsql");
plpgsql_HashTableInit();
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index f3d1283..93956fd 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -739,6 +739,10 @@ typedef struct PLpgSQL_function
bool print_strict_params;
+ /* extra checks */
+ bool extra_warnings;
+ bool extra_errors;
+
int ndatums;
PLpgSQL_datum **datums;
PLpgSQL_stmt_block *action;
@@ -881,6 +885,9 @@ extern int plpgsql_variable_conflict;
extern bool plpgsql_print_strict_params;
+extern bool plpgsql_extra_warnings;
+extern bool plpgsql_extra_errors;
+
extern bool plpgsql_check_syntax;
extern bool plpgsql_DumpExecTree;
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 0405ef4..987c417 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3208,6 +3208,79 @@ 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 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;
+ ^
+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 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.extra_errors;
+reset plpgsql.extra_warnings;
-- test scrollable cursor support
create function sc_test() returns setof integer as $$
declare
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index d01011a..717d4fa 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2689,6 +2689,68 @@ end$$ language plpgsql;
select footest();
+-- 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;
+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 void as $$
+declare f1 int; begin end $$ language plpgsql;
+
+reset plpgsql.extra_errors;
+reset plpgsql.extra_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
