Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 23/03/14 19:38, Pavel Stehule wrote:
doc should be enhanced by:
snip
Docs updated.
--
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..2a9b327 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4711,6 +4711,54 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
/variablelist
/sect2
+ sect2 id=plpgsql-extra-checks
+ titleAdditional compile-time checks/title
+
+ para
+To aid the user in finding instances of simple but common problems before
+they cause harm, applicationPL/PgSQL/ provides additional
+replaceablechecks/. When enabled, depending on the configuration, they
+can be used to emit either a literalWARNING/ or an literalERROR/
+during the compilation of a function.
+ /para
+
+ para
+ These additional checks are enabled through the configuration variables
+ varnameplpgsql.extra_warnings/ for warnings and
+ varnameplpgsql.extra_errors/ for errors. Both can be set either to
+ a comma-separated list of checks, literalnone/ or literalall/.
+ The default is literalnone/. Currently the list of available checks
+ includes only one:
+ variablelist
+ varlistentry
+termvarnameshadowed_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 varnameplpgsql.extra_warnings/
+ set to varnameshadowed_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 --
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 5afc2e5..12ac964 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 = 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);
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index c0cb585..91186c6 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -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)));
+ }
+
}
;
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index f21393a..e659f8e 100644
---
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 24 March 2014 10:58, Petr Jelinek p...@2ndquadrant.com wrote: Docs updated. OK, it looks to me that all outstanding comments have been resolved. I'll be looking to commit this later today, so last call for comments. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
Review shadow_v6 patch Hello I did a recheck a newest version of this patch: 1. There is a wide agreement on implemented feature - nothing changed from previous review - it is not necessary comment it again. 2. v6 patch: patching cleanly, compilation without errors and warnings, all regress tests passed Tom's objections was related to GUC part. It is redesigned as Tom proposed. The code is good - and I don't see any problem there. I have only one objection - What I remember - more usual is using a list instead a bitmap for these purposes - typical is DefElem struct. Isn't it better? Regards Pavel 2014-03-20 12:39 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: On 20/03/14 00:32, Tom Lane wrote: TBH, if I thought this specific warning was the only one that would ever be there, I'd probably be arguing to reject this patch altogether. Of course, nobody assumes that it will be the only one. Also, adding GUC_LIST_INPUT later is not really cool since it changes the parsing behavior for the GUC. If it's going to be a list, it should be one from day zero. Actually it does not since it all has to be handled in check/assign hook anyway. But nevertheless, I made V6 with doc change suggested by Alvaro and also added this list handling framework for the GUC params. In the end it is probably less confusing now that the implementation uses bitmask instead of bool when the user facing functionality talks about list... This obviously needs code review again (I haven't changed tests since nothing changed from user perspective). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
2014-03-23 15:14 GMT+01:00 Pavel Stehule pavel.steh...@gmail.com: Review shadow_v6 patch Hello I did a recheck a newest version of this patch: 1. There is a wide agreement on implemented feature - nothing changed from previous review - it is not necessary comment it again. 2. v6 patch: patching cleanly, compilation without errors and warnings, all regress tests passed Tom's objections was related to GUC part. It is redesigned as Tom proposed. The code is good - and I don't see any problem there. I have only one objection - What I remember - more usual is using a list instead a bitmap for these purposes - typical is DefElem struct. Isn't it better? A using DefElem will be longer, but it is typical pattern for this case in Postgres. What is opinion of other hackers? Pavel Regards Pavel 2014-03-20 12:39 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: On 20/03/14 00:32, Tom Lane wrote: TBH, if I thought this specific warning was the only one that would ever be there, I'd probably be arguing to reject this patch altogether. Of course, nobody assumes that it will be the only one. Also, adding GUC_LIST_INPUT later is not really cool since it changes the parsing behavior for the GUC. If it's going to be a list, it should be one from day zero. Actually it does not since it all has to be handled in check/assign hook anyway. But nevertheless, I made V6 with doc change suggested by Alvaro and also added this list handling framework for the GUC params. In the end it is probably less confusing now that the implementation uses bitmask instead of bool when the user facing functionality talks about list... This obviously needs code review again (I haven't changed tests since nothing changed from user perspective). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 23/03/14 15:14, Pavel Stehule wrote: Review shadow_v6 patch I have only one objection - What I remember - more usual is using a list instead a bitmap for these purposes - typical is DefElem struct. Isn't it better? To me it seemed that for similar use cases (list of boolean options) the bitmap is more common in the existing code, question might be if we go over the 32 bits any time soon which does not seem likely to me for the checks. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
2014-03-23 15:53 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: On 23/03/14 15:14, Pavel Stehule wrote: Review shadow_v6 patch I have only one objection - What I remember - more usual is using a list instead a bitmap for these purposes - typical is DefElem struct. Isn't it better? To me it seemed that for similar use cases (list of boolean options) the bitmap is more common in the existing code, question might be if we go over the 32 bits any time soon which does not seem likely to me for the checks. I don't afraid so 32 bits it is too low - in this case, list can be used without compatibility issues. if others has no problem with it, I have not a problem too. doc should be enhanced by: SET plpgsql.extra_warnings TO 'shadowed_variables'; CREATE FUNCTION foo(f1 int) RETURNS int AS $$ DECLARE f1 int; BEGIN RETURN f1; END $$ LANGUAGE plpgsql; Regards Pavel -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
+myextra = (int *) malloc(sizeof(int)); Please consider not casting malloc(). See http://c-faq.com/malloc/mallocnocast.html -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
Piotr Stefaniak postg...@piotr-stefaniak.me writes: +myextra = (int *) malloc(sizeof(int)); Please consider not casting malloc(). See That code is per project style, and should stay that way. http://c-faq.com/malloc/mallocnocast.html That argument is entirely bogus, as it considers only one possible way in which the call could be wrong; a way that is of very low probability in PG usage, since we include stdlib.h in our core headers. Besides which, as noted in the page itself, most modern compilers would warn anyway if you forgot the inclusion. On the other side, coding with the explicit cast helps guard against far more dangerous coding errors, which the compiler will *not* help you with. What if myextra is actually of type int64 *? In that case you probably meant to make enough space for an int64 not an int. But without the cast, you won't be told you did anything wrong. This is a particular hazard if you change your mind later on about the type of myextra. (A colleague at Salesforce got burnt in exactly that way, just a couple days ago.) So, general policy around here is that malloc and palloc calls should look like ptr = (foo *) malloc(n * sizeof(foo)); so that there's a direct, visible connection between the size calculation and the type of the resulting pointer. I'm aware that there are some places in the code that fail to do this, but they are not models to emulate. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 03/22/2014 04:00 PM, Tom Lane wrote: That argument is entirely bogus, as it considers only one possible way in which the call could be wrong; a way that is of very low probability in PG usage, since we include stdlib.h in our core headers. Besides which, as noted in the page itself, most modern compilers would warn anyway if you forgot the inclusion. Apart from what the page says, I also think of casting malloc() as bad style and felt the need to bring this up. But since you pointed out why you don't want to remove the cast, I withdraw my previous suggestion. On the other side, coding with the explicit cast helps guard against far more dangerous coding errors, which the compiler will *not* help you with. What if myextra is actually of type int64 *? In that case you probably meant to make enough space for an int64 not an int. But without the cast, you won't be told you did anything wrong. This is a particular hazard if you change your mind later on about the type of myextra. (A colleague at Salesforce got burnt in exactly that way, just a couple days ago.) So perhaps this alternative: myextra = malloc(sizeof *myextra); PS. Coding style matters to me, but I was and still am far from insisting on anything. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
Piotr Stefaniak postg...@piotr-stefaniak.me writes: Apart from what the page says, I also think of casting malloc() as bad style and felt the need to bring this up. Well, that's a value judgement I don't happen to agree with. Yeah, it'd be better if the language design were such that we could avoid explicit casting everywhere, but in this context casting is less risky than not casting. So perhaps this alternative: myextra = malloc(sizeof *myextra); [ shrug... ] That's about a wash for this exact use case, but it gets messy as soon as the lefthand side is anything more complicated than a simple variable name. And it doesn't scale to cases where the malloc result isn't directly assigned to anything --- for example, what if you want to pass the result of palloc() directly to some other function, or return it from the current function? The bigger picture though is that the style with the explicit cast is already extremely widely used in Postgres. That being the case, conforming to project style is better than using some inconsistent convention, regardless of your personal views about whether there's a better way to do it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 03/22/2014 04:00 PM, Tom Lane wrote:
On the other side, coding with the explicit cast helps guard against far
more dangerous coding errors, which the compiler will*not* help you with.
What if myextra is actually of type int64 *?
Indeed, neither gcc -Wall -Wextra -std=c89 -pedantic nor clang
-Weverything -Wno-shadow -std=c89 -pedantic issues a warning in such
case. clang --analyze, however, does. Perhaps TenDRA would, if it ever
worked.
This message is meant to be merely informative, since I've put some
effort into this test. I'm not trying to argue.
#include stdlib.h
typedef long int int64;
int main(void)
{
int *myextra;
/* with explicit casting */
myextra = (int *) malloc(sizeof (int));
free(myextra);
/* with no explicit casting */
myextra = malloc(sizeof (int));
free(myextra);
/* myextra now becomes int64 */
{
int64 *myextra;
/* with explicit casting */
myextra = (int *) malloc(sizeof (int)); /* [1], [2]. and [3] warn here */
free(myextra);
/* with no explicit casting */
myextra = malloc(sizeof (int)); /* Only [3] warns here */
free(myextra);
}
return 0;
}
/*
1: gcc 4.8.2: gcc -Wall -Wextra -std=c89 -pedantic /tmp/test.c
2: clang 3.5.0: clang -Weverything -Wno-shadow -std=c89 -pedantic /tmp/test.c
3: clang 3.5.0: clang --analyze /tmp/test.c
*/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 3/20/14, 12:32 AM, Tom Lane wrote: Isn't the entire point to create a framework in which more tests will be added later? Also, adding GUC_LIST_INPUT later is not really cool since it changes the parsing behavior for the GUC. If it's going to be a list, it should be one from day zero. I'm not sure what exactly you mean by this. If the only allowed values are none, variable_shadowing and all, how is the behaviour for those going to change if we make it a list for 9.5? Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 20/03/14 00:32, Tom Lane wrote:
TBH, if I thought this specific warning was the only one that would ever
be there, I'd probably be arguing to reject this patch altogether.
Of course, nobody assumes that it will be the only one.
Also, adding GUC_LIST_INPUT later is not really cool since it changes
the parsing behavior for the GUC. If it's going to be a list, it should
be one from day zero.
Actually it does not since it all has to be handled in check/assign hook
anyway.
But nevertheless, I made V6 with doc change suggested by Alvaro and also
added this list handling framework for the GUC params.
In the end it is probably less confusing now that the implementation
uses bitmask instead of bool when the user facing functionality talks
about list...
This obviously needs code review again (I haven't changed tests since
nothing changed from user perspective).
--
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..d1e6c9f 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4711,6 +4711,52 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
/variablelist
/sect2
+ sect2 id=plpgsql-extra-checks
+ titleAdditional compile-time checks/title
+
+ para
+To aid the user in finding instances of simple but common problems before
+they cause harm, applicationPL/PgSQL/ provides additional
+replaceablechecks/. When enabled, depending on the configuration, they
+can be used to emit either a literalWARNING/ or an literalERROR/
+during the compilation of a function.
+ /para
+
+ para
+ These additional checks are enabled through the configuration variables
+ varnameplpgsql.extra_warnings/ for warnings and
+ varnameplpgsql.extra_errors/ for errors. Both can be set either to
+ a comma-separated list of checks, literalnone/ or literalall/.
+ The default is literalnone/. Currently the list of available checks
+ includes only one:
+ variablelist
+ varlistentry
+termvarnameshadowed_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 varnameplpgsql.extra_warnings/
+ set to varnameshadowed_variables/:
+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
+ /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..12ac964 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 = 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);
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index c0cb585..91186c6 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -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
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
Marko Tiikkaja ma...@joh.to writes: On 3/20/14, 12:32 AM, Tom Lane wrote: Also, adding GUC_LIST_INPUT later is not really cool since it changes the parsing behavior for the GUC. If it's going to be a list, it should be one from day zero. I'm not sure what exactly you mean by this. If the only allowed values are none, variable_shadowing and all, how is the behaviour for those going to change if we make it a list for 9.5? If we switch to using SplitIdentifierString later, which is the typical implementation of parsing list GUCs, that will do things like case-fold, remove double quotes, remove white space. It's possible that that's completely upward-compatible with what happens if you don't do that ... but I'm not sure about it. In any case, if the point of this patch is to provide a framework for extra error detection, I'm not sure why we'd arbitrarily say we're going to leave the framework unfinished in the GUC department. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
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 Regards Pavel Stehule
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
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
+ titleAdditional compile-time checks/title
+
+ para
+To aid the user in finding instances of simple but common problems before
+they cause harm, applicationPL/PgSQL/ provides additional
+replaceablechecks/. When enabled, depending on the configuration, they
+can be used to emit either a literalWARNING/ or an literalERROR/
+during the compilation of a function.
+ /para
+
+ para
+ These additional checks are enabled through the configuration variables
+ varnameplpgsql.extra_warnings/ for warnings and
+ varnameplpgsql.extra_errors/ for errors. Both can be set either to
+ a comma-separated list of checks, literalnone/ or literalall/.
+ The default is literalnone/. Currently the list of available checks
+ includes only one:
+ variablelist
+ varlistentry
+termvarnameshadowed_variables/varname/term
+listitem
+ para
+ Checks if a declaration shadows a previously defined variable. For
+ example (with varnameplpgsql.extra_warnings/ set to
+ varnameshadowed_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
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
Hello all is pk Pavel 2014-03-19 11:28 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: 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
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
Why start a new thread for this review? It seems to me it'd be more comfortable to keep the review as a reply on the original thread. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
2014-03-19 13:51 GMT+01:00 Alvaro Herrera alvhe...@2ndquadrant.com: Why start a new thread for this review? It seems to me it'd be more comfortable to keep the review as a reply on the original thread. I am sorry, I though so review should to start in separate thread Pavel -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
Petr Jelinek escribió:
+ para
+ These additional checks are enabled through the configuration variables
+ varnameplpgsql.extra_warnings/ for warnings and
+ varnameplpgsql.extra_errors/ for errors. Both can be set either to
+ a comma-separated list of checks, literalnone/ or literalall/.
+ The default is literalnone/. Currently the list of available checks
+ includes only one:
+ variablelist
+ varlistentry
+termvarnameshadowed_variables/varname/term
+listitem
+ para
+ Checks if a declaration shadows a previously defined variable. For
+ example (with varnameplpgsql.extra_warnings/ set to
+ varnameshadowed_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
As a matter of style, I think the example should go after the
variablelist is closed. Perhaps in the future, when we invent more
extra warnings/errors, we might want to show more than one in a single
example, for compactness.
+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;
+}
I'm not too clear on how this works when there is more than one possible
value.
+ 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);
I think this should have the GUC_LIST_INPUT flag, and ensure that when
multiple values are passed, we can process them all in a sane fashion.
Other than this, the patch looks sane to me in a quick skim.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 19/03/14 19:26, Alvaro Herrera wrote: Petr Jelinek escribió: + para + These additional checks are enabled through the configuration variables + varnameplpgsql.extra_warnings/ for warnings and + varnameplpgsql.extra_errors/ for errors. Both can be set either to + a comma-separated list of checks, literalnone/ or literalall/. + The default is literalnone/. Currently the list of available checks + includes only one: + variablelist + varlistentry +termvarnameshadowed_variables/varname/term +listitem + para + Checks if a declaration shadows a previously defined variable. For + example (with varnameplpgsql.extra_warnings/ set to + varnameshadowed_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 As a matter of style, I think the example should go after the variablelist is closed. Perhaps in the future, when we invent more extra warnings/errors, we might want to show more than one in a single example, for compactness. Ok I can change that. +static bool +plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source) I'm not too clear on how this works when there is more than one possible value. + 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); I think this should have the GUC_LIST_INPUT flag, and ensure that when multiple values are passed, we can process them all in a sane fashion. Well, as we said with Marko in the original thread, the proper handling is left for whoever wants to add additional parameters, for the current implementation proper list handling is not really needed and it will only server to increase complexity of this simple patch quite late in the release cycle. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
Petr Jelinek p...@2ndquadrant.com writes: On 19/03/14 19:26, Alvaro Herrera wrote: I think this should have the GUC_LIST_INPUT flag, and ensure that when multiple values are passed, we can process them all in a sane fashion. Well, as we said with Marko in the original thread, the proper handling is left for whoever wants to add additional parameters, for the current implementation proper list handling is not really needed and it will only server to increase complexity of this simple patch quite late in the release cycle. TBH, if I thought this specific warning was the only one that would ever be there, I'd probably be arguing to reject this patch altogether. Isn't the entire point to create a framework in which more tests will be added later? Also, adding GUC_LIST_INPUT later is not really cool since it changes the parsing behavior for the GUC. If it's going to be a list, it should be one from day zero. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
2014-03-20 0:32 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Petr Jelinek p...@2ndquadrant.com writes: On 19/03/14 19:26, Alvaro Herrera wrote: I think this should have the GUC_LIST_INPUT flag, and ensure that when multiple values are passed, we can process them all in a sane fashion. Well, as we said with Marko in the original thread, the proper handling is left for whoever wants to add additional parameters, for the current implementation proper list handling is not really needed and it will only server to increase complexity of this simple patch quite late in the release cycle. TBH, if I thought this specific warning was the only one that would ever be there, I'd probably be arguing to reject this patch altogether. Isn't the entire point to create a framework in which more tests will be added later? I plan to work on plpgsql redesign this summer, so plpgsql check with same functionality can be on next release, but should not be too. This functionality doesn't change anything - and when we will have a better tools, we can replace it without any cost, so I am for integration. It can helps - plpgsql_check is next level, but it is next level complexity and now it is not simply to integrate it. Proposed feature can server lot of users and it is good API when we integrate some more sophisticated tool. I like this interface - it is simple and good for almost all use cases that I can to see. Regards Pavel Also, adding GUC_LIST_INPUT later is not really cool since it changes the parsing behavior for the GUC. If it's going to be a list, it should be one from day zero. regards, tom lane
