Skip to content

Commit 7d8f1de

Browse files
simonat2ndQuadrantsimonat2ndQuadrant
committed
Extra warnings and errors for PL/pgSQL
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
1 parent f14a6bb commit 7d8f1de

File tree

7 files changed

+413
-0
lines changed

7 files changed

+413
-0
lines changed

‎doc/src/sgml/plpgsql.sgml‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4711,6 +4711,56 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
47114711
</variablelist>
47124712

47134713
</sect2>
4714+
<sect2 id="plpgsql-extra-checks">
4715+
<title>Additional compile-time checks</title>
4716+
4717+
<para>
4718+
To aid the user in finding instances of simple but common problems before
4719+
they cause harm, <application>PL/PgSQL</> provides additional
4720+
<replaceable>checks</>. When enabled, depending on the configuration, they
4721+
can be used to emit either a <literal>WARNING</> or an <literal>ERROR</>
4722+
during the compilation of a function. A function which has received
4723+
a <literal>WARNING</> can be executed without producing further messages,
4724+
so you are advised to test in a separate development environment.
4725+
</para>
4726+
4727+
<para>
4728+
These additional checks are enabled through the configuration variables
4729+
<varname>plpgsql.extra_warnings</> for warnings and
4730+
<varname>plpgsql.extra_errors</> for errors. Both can be set either to
4731+
a comma-separated list of checks, <literal>"none"</> or <literal>"all"</>.
4732+
The default is <literal>"none"</>. Currently the list of available checks
4733+
includes only one:
4734+
<variablelist>
4735+
<varlistentry>
4736+
<term><varname>shadowed_variables</varname></term>
4737+
<listitem>
4738+
<para>
4739+
Checks if a declaration shadows a previously defined variable.
4740+
</para>
4741+
</listitem>
4742+
</varlistentry>
4743+
</variablelist>
4744+
4745+
The following example shows the effect of <varname>plpgsql.extra_warnings</>
4746+
set to <varname>shadowed_variables</>:
4747+
<programlisting>
4748+
SET plpgsql.extra_warnings TO 'shadowed_variables';
4749+
4750+
CREATE FUNCTION foo(f1 int) RETURNS int AS $$
4751+
DECLARE
4752+
f1 int;
4753+
BEGIN
4754+
RETURN f1;
4755+
END
4756+
$$ LANGUAGE plpgsql;
4757+
WARNING: variable "f1" shadows a previously defined variable
4758+
LINE 3: f1 int;
4759+
^
4760+
CREATE FUNCTION
4761+
</programlisting>
4762+
</para>
4763+
</sect2>
47144764
</sect1>
47154765

47164766
<!-- **** Porting from Oracle PL/SQL **** -->

‎src/pl/plpgsql/src/pl_comp.c‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,9 @@ do_compile(FunctionCallInfo fcinfo,
352352
function->out_param_varno = -1; /* set up for no OUT param */
353353
function->resolve_option = plpgsql_variable_conflict;
354354
function->print_strict_params = plpgsql_print_strict_params;
355+
/* only promote extra warnings and errors at CREATE FUNCTION time */
356+
function->extra_warnings = forValidator ? plpgsql_extra_warnings : 0;
357+
function->extra_errors = forValidator ? plpgsql_extra_errors : 0;
355358

356359
if (is_dml_trigger)
357360
function->fn_is_trigger = PLPGSQL_DML_TRIGGER;
@@ -849,6 +852,9 @@ plpgsql_compile_inline(char *proc_source)
849852
function->out_param_varno = -1; /* set up for no OUT param */
850853
function->resolve_option = plpgsql_variable_conflict;
851854
function->print_strict_params = plpgsql_print_strict_params;
855+
/* don't do extra validation for inline code as we don't want to add spam at runtime */
856+
function->extra_warnings = 0;
857+
function->extra_errors = 0;
852858

853859
plpgsql_ns_init();
854860
plpgsql_ns_push(func_name);

‎src/pl/plpgsql/src/pl_gram.y‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -727,6 +727,21 @@ decl_varname : T_WORD
727727
$1.ident, NULL, NULL,
728728
NULL) != NULL)
729729
yyerror("duplicate declaration");
730+
731+
if (plpgsql_curr_compile->extra_warnings & PLPGSQL_XCHECK_SHADOWVAR ||
732+
plpgsql_curr_compile->extra_errors & PLPGSQL_XCHECK_SHADOWVAR)
733+
{
734+
PLpgSQL_nsitem *nsi;
735+
nsi = plpgsql_ns_lookup(plpgsql_ns_top(), false,
736+
$1.ident, NULL, NULL, NULL);
737+
if (nsi != NULL)
738+
ereport(plpgsql_curr_compile->extra_errors & PLPGSQL_XCHECK_SHADOWVAR ? ERROR : WARNING,
739+
(errcode(ERRCODE_DUPLICATE_ALIAS),
740+
errmsg("variable \"%s\" shadows a previously defined variable",
741+
$1.ident),
742+
parser_errposition(@1)));
743+
}
744+
730745
}
731746
| unreserved_keyword
732747
{
@@ -740,6 +755,21 @@ decl_varname : T_WORD
740755
$1, NULL, NULL,
741756
NULL) != NULL)
742757
yyerror("duplicate declaration");
758+
759+
if (plpgsql_curr_compile->extra_warnings & PLPGSQL_XCHECK_SHADOWVAR ||
760+
plpgsql_curr_compile->extra_errors & PLPGSQL_XCHECK_SHADOWVAR)
761+
{
762+
PLpgSQL_nsitem *nsi;
763+
nsi = plpgsql_ns_lookup(plpgsql_ns_top(), false,
764+
$1, NULL, NULL, NULL);
765+
if (nsi != NULL)
766+
ereport(plpgsql_curr_compile->extra_errors & PLPGSQL_XCHECK_SHADOWVAR ? ERROR : WARNING,
767+
(errcode(ERRCODE_DUPLICATE_ALIAS),
768+
errmsg("variable \"%s\" shadows a previously defined variable",
769+
$1),
770+
parser_errposition(@1)));
771+
}
772+
743773
}
744774
;
745775

‎src/pl/plpgsql/src/pl_handler.c‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@
2525
#include "utils/lsyscache.h"
2626
#include "utils/syscache.h"
2727

28+
29+
static bool plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source);
30+
static void plpgsql_extra_warnings_assign_hook(const char *newvalue, void *extra);
31+
static void plpgsql_extra_errors_assign_hook(const char *newvalue, void *extra);
32+
2833
PG_MODULE_MAGIC;
2934

3035
/* Custom GUC variable */
@@ -39,10 +44,89 @@ int plpgsql_variable_conflict = PLPGSQL_RESOLVE_ERROR;
3944

4045
bool plpgsql_print_strict_params = false;
4146

47+
char *plpgsql_extra_warnings_string = NULL;
48+
char *plpgsql_extra_errors_string = NULL;
49+
int plpgsql_extra_warnings;
50+
int plpgsql_extra_errors;
51+
4252
/* Hook for plugins */
4353
PLpgSQL_plugin **plugin_ptr = NULL;
4454

4555

56+
static bool
57+
plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source)
58+
{
59+
char *rawstring;
60+
List *elemlist;
61+
ListCell *l;
62+
int extrachecks = 0;
63+
int *myextra;
64+
65+
if (pg_strcasecmp(*newvalue, "all") == 0)
66+
extrachecks = PLPGSQL_XCHECK_ALL;
67+
else if (pg_strcasecmp(*newvalue, "none") == 0)
68+
extrachecks = PLPGSQL_XCHECK_NONE;
69+
else
70+
{
71+
/* Need a modifiable copy of string */
72+
rawstring = pstrdup(*newvalue);
73+
74+
/* Parse string into list of identifiers */
75+
if (!SplitIdentifierString(rawstring, ',', &elemlist))
76+
{
77+
/* syntax error in list */
78+
GUC_check_errdetail("List syntax is invalid.");
79+
pfree(rawstring);
80+
list_free(elemlist);
81+
return false;
82+
}
83+
84+
foreach(l, elemlist)
85+
{
86+
char *tok = (char *) lfirst(l);
87+
88+
if (pg_strcasecmp(tok, "shadowed_variables") == 0)
89+
extrachecks |= PLPGSQL_XCHECK_SHADOWVAR;
90+
else if (pg_strcasecmp(tok, "all") == 0 || pg_strcasecmp(tok, "none") == 0)
91+
{
92+
GUC_check_errdetail("Key word \"%s\" cannot be combined with other key words.", tok);
93+
pfree(rawstring);
94+
list_free(elemlist);
95+
return false;
96+
}
97+
else
98+
{
99+
GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
100+
pfree(rawstring);
101+
list_free(elemlist);
102+
return false;
103+
}
104+
}
105+
106+
pfree(rawstring);
107+
list_free(elemlist);
108+
}
109+
110+
myextra = (int *) malloc(sizeof(int));
111+
*myextra = extrachecks;
112+
*extra = (void *) myextra;
113+
114+
return true;
115+
}
116+
117+
static void
118+
plpgsql_extra_warnings_assign_hook(const char *newvalue, void *extra)
119+
{
120+
plpgsql_extra_warnings = *((int *) extra);
121+
}
122+
123+
static void
124+
plpgsql_extra_errors_assign_hook(const char *newvalue, void *extra)
125+
{
126+
plpgsql_extra_errors = *((int *) extra);
127+
}
128+
129+
46130
/*
47131
* _PG_init() - library load-time initialization
48132
*
@@ -76,6 +160,26 @@ _PG_init(void)
76160
PGC_USERSET, 0,
77161
NULL, NULL, NULL);
78162

163+
DefineCustomStringVariable("plpgsql.extra_warnings",
164+
gettext_noop("List of programming constructs which should produce a warning."),
165+
NULL,
166+
&plpgsql_extra_warnings_string,
167+
"none",
168+
PGC_USERSET, GUC_LIST_INPUT,
169+
plpgsql_extra_checks_check_hook,
170+
plpgsql_extra_warnings_assign_hook,
171+
NULL);
172+
173+
DefineCustomStringVariable("plpgsql.extra_errors",
174+
gettext_noop("List of programming constructs which should produce an error."),
175+
NULL,
176+
&plpgsql_extra_errors_string,
177+
"none",
178+
PGC_USERSET, GUC_LIST_INPUT,
179+
plpgsql_extra_checks_check_hook,
180+
plpgsql_extra_errors_assign_hook,
181+
NULL);
182+
79183
EmitWarningsOnPlaceholders("plpgsql");
80184

81185
plpgsql_HashTableInit();

‎src/pl/plpgsql/src/plpgsql.h‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -739,6 +739,10 @@ typedef struct PLpgSQL_function
739739

740740
bool print_strict_params;
741741

742+
/* extra checks */
743+
int extra_warnings;
744+
int extra_errors;
745+
742746
int ndatums;
743747
PLpgSQL_datum **datums;
744748
PLpgSQL_stmt_block *action;
@@ -881,6 +885,14 @@ extern int plpgsql_variable_conflict;
881885

882886
extern bool plpgsql_print_strict_params;
883887

888+
/* extra compile-time checks */
889+
#define PLPGSQL_XCHECK_NONE 0
890+
#define PLPGSQL_XCHECK_SHADOWVAR 1
891+
#define PLPGSQL_XCHECK_ALL ((int) ~0)
892+
893+
extern int plpgsql_extra_warnings;
894+
extern int plpgsql_extra_errors;
895+
884896
extern bool plpgsql_check_syntax;
885897
extern bool plpgsql_DumpExecTree;
886898

0 commit comments

Comments
 (0)