On 18/03/14 13:43, Pavel Stehule wrote:
2014-03-18 13:23 GMT+01:00 Petr Jelinek <p...@2ndquadrant.com
<mailto:p...@2ndquadrant.com>>
Agree that compile_errors dos not make sense, additional_errors
and additional_warnings seems better, maybe plpgsql.extra_warnings
and plpgsql.extra_errors?
extra* sounds better
Ok, so I took the liberty of rewriting the patch so that it uses
plpgsql.extra_warnings and plpgsql.extra_errors configuration variables
with possible values "none", "all" and "shadow" ("none" being the default).
Updated doc and regression tests to reflect the code changes, everything
builds and tests pass just fine.
I did one small change (that I think was agreed anyway) from Marko's
original patch in that warnings are only emitted during function
creation, no runtime warnings and no warnings for inline (DO) plpgsql
code either as I really don't think these optional warnings/errors
during runtime are a good idea.
Note that the patch does not really handle the list of values as list,
basically "all" and "shadow" are translated to true and proper handling
of this is left to whoever will want to implement additional checks. I
think this approach is fine as I don't see the need to build frameworks
here and it was same in the original patch.
--
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..56ee2b3 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 a number of additional
+ <replaceable>checks</>. When enabled, depending on the configuration, they
+ can be used to emit 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>shadow</varname></term>
+ <listitem>
+ <para>
+ Checks if a declaration shadows a previously defined variable. For
+ example (with <varname>plpgsql.extra_warnings</> set to
+ <varname>shadow</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..020c601 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, "shadow") == 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, "shadow") == 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, "shadow") == 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..5d21b15 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 '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 errors when shadowing a variable
+set plpgsql.extra_errors to 'shadow';
+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..744c796 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 '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 errors when shadowing a variable
+
+set plpgsql.extra_errors to 'shadow';
+
+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
