Re: [HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

Hi Petr,

On 3/18/14, 8:38 PM, I wrote:

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.


Not super excited, but I can live with that.


I'm sorry, that came out wrong.

As far as I'm concerned, I believe we have a consensus that 
*runtime-only* warnings are not a terribly good idea.  The warnings in 
this patch were emitted originally all the time because I wanted to 
maximize their visibility.  But I think that has a bit of the same 
problems as run-time warnings do; who's gonna notice them?


In any case, I think you guys have the situation under control and if 
this patch gets committed like this, it solves my issues.  Thanks for 
your work here.



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] plpgsql.warn_shadow

[Simon Riggs]

On 18 March 2014 20:52, Marti Raudsepp  wrote:
> On Tue, Mar 18, 2014 at 9:29 PM, Petr Jelinek  wrote:
>> Attached V4 uses "shadowed-variables" instead of "shadow".
>
> I think that should be "shadowed_variables" for consistency; GUC
> values usually have underscores, not dashes. (e.g.
> intervalstyle=sql_standard, backslash_quote=safe_encoding,
> synchronous_commit=remote_write etc)

Definitely. Sorry for not noticing that earlier; don't want dashes.

-- 
 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] plpgsql.warn_shadow

[Marti Raudsepp]

On Tue, Mar 18, 2014 at 9:29 PM, Petr Jelinek  wrote:
> Attached V4 uses "shadowed-variables" instead of "shadow".

I think that should be "shadowed_variables" for consistency; GUC
values usually have underscores, not dashes. (e.g.
intervalstyle=sql_standard, backslash_quote=safe_encoding,
synchronous_commit=remote_write etc)

Regards,
Marti


-- 
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] plpgsql.warn_shadow

[Pavel Stehule]

2014-03-18 20:49 GMT+01:00 Petr Jelinek :

>
> On 18/03/14 20:45, Pavel Stehule wrote:
>
>
>
> 2014-03-18 20:44 GMT+01:00 Petr Jelinek :
>
>>
>> On 18/03/14 20:36, Pavel Stehule wrote:
>>
>>>
>>>
>>> +To aid the user in finding instances of simple but common problems
>>> before
>>> +they cause harm, PL/PgSQL provides a number of
>>> additional
>>> +checks. When enabled, depending on the
>>> configuration, they
>>> +can be used to emit either a WARNING or an
>>> ERROR
>>> +during the compilation of a function.
>>> +   
>>>
>>> >>>provides a number of additional<<<
>>>
>>> There will be only one check in 9.4, so this sentence is confusing and
>>> should be reformulated.
>>>
>>
>>  Thanks, yeah I left that sentence unchanged from original patch, maybe I
>> should remove the word "number" there as it implies that there are a lot of
>> them, but I don't really want to change everything to singular when the
>> input is specified as list.
>
>
>  What about add sentence: in this moment only "shadowed-variables" is
> available?
>
>
> There is something like that said 2 paragraphs down the line:
>
> +  The default is "none". Currently the list of available checks
> +  includes only one:
> +  
> +   
> +shadowed-variables
>
>
ok


>  --
>  Petr Jelinek  http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>
>

Re: [HACKERS] plpgsql.warn_shadow

[Petr Jelinek]

On 18/03/14 20:45, Pavel Stehule wrote:



2014-03-18 20:44 GMT+01:00 Petr Jelinek >:



On 18/03/14 20:36, Pavel Stehule wrote:


   
+To aid the user in finding instances of simple but common
problems before
+they cause harm, PL/PgSQL provides a
number of additional
+checks. When enabled, depending on the
configuration, they
+can be used to emit either a WARNING or an
ERROR
+during the compilation of a function.
+   

>>>provides a number of additional<<<

There will be only one check in 9.4, so this sentence is
confusing and should be reformulated.


Thanks, yeah I left that sentence unchanged from original patch,
maybe I should remove the word "number" there as it implies that
there are a lot of them, but I don't really want to change
everything to singular when the input is specified as list.


What about add sentence: in this moment only "shadowed-variables" is 
available?


There is something like that said 2 paragraphs down the line:

+  The default is "none". Currently the list of available checks
+  includes only one:
+  
+   
+shadowed-variables

--
 Petr Jelinek  http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] plpgsql.warn_shadow

[Pavel Stehule]

2014-03-18 20:44 GMT+01:00 Petr Jelinek :

>
> On 18/03/14 20:36, Pavel Stehule wrote:
>
>>
>>
>> +To aid the user in finding instances of simple but common problems
>> before
>> +they cause harm, PL/PgSQL provides a number of
>> additional
>> +checks. When enabled, depending on the
>> configuration, they
>> +can be used to emit either a WARNING or an
>> ERROR
>> +during the compilation of a function.
>> +   
>>
>> >>>provides a number of additional<<<
>>
>> There will be only one check in 9.4, so this sentence is confusing and
>> should be reformulated.
>>
>
> Thanks, yeah I left that sentence unchanged from original patch, maybe I
> should remove the word "number" there as it implies that there are a lot of
> them, but I don't really want to change everything to singular when the
> input is specified as list.


What about add sentence: in this moment only "shadowed-variables" is
available?

Pavel


>
>
> --
>  Petr Jelinek  http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>
>

Re: [HACKERS] plpgsql.warn_shadow

[Petr Jelinek]

On 18/03/14 20:36, Pavel Stehule wrote:


   
+To aid the user in finding instances of simple but common 
problems before
+they cause harm, PL/PgSQL provides a number of 
additional
+checks. When enabled, depending on the 
configuration, they
+can be used to emit either a WARNING or an 
ERROR

+during the compilation of a function.
+   

>>>provides a number of additional<<<

There will be only one check in 9.4, so this sentence is confusing and 
should be reformulated.


Thanks, yeah I left that sentence unchanged from original patch, maybe I 
should remove the word "number" there as it implies that there are a lot 
of them, but I don't really want to change everything to singular when 
the input is specified as list.


--
 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] plpgsql.warn_shadow

[Marko Tiikkaja]

On 3/18/14, 7:56 PM, Petr Jelinek wrote:

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.


Cool, thanks!


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.


Not super excited, but I can live with that.


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.


Yeah, I don't think rushing all that logic into 9.4 would be such a good 
idea.  Especially since it's not necessary at all.



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] plpgsql.warn_shadow

[Pavel Stehule]

Hello


Tomorrow I'll do a review

fast check



   
+To aid the user in finding instances of simple but common problems
before
+they cause harm, PL/PgSQL provides a number of
additional
+checks. When enabled, depending on the configuration,
they
+can be used to emit either a WARNING or an
ERROR
+during the compilation of a function.
+   

>>>provides a number of additional<<<

There will be only one check in 9.4, so this sentence is confusing and
should be reformulated.

Regards

Pavel



2014-03-18 20:29 GMT+01:00 Petr Jelinek :

>
> On 18/03/14 20:15, Pavel Stehule wrote:
>
>
> 2014-03-18 20:14 GMT+01:00 Simon Riggs :
>
>> On 18 March 2014 19:04, Pavel Stehule  wrote:
>>
>> > I don't think so only "shadow" is good name for some check. Maybe
>> > "shadow-variables-check"
>>
>>  "shadowed-variables" would be a better name.
>>
>
> +1
>
>
> Attached V4 uses "shadowed-variables" instead of "shadow".
>
> --
>  Petr Jelinek  http://www.2ndQuadrant.com/
>
>  PostgreSQL Development, 24x7 Support, Training & Services
>
>

Re: [HACKERS] plpgsql.warn_shadow

[Petr Jelinek]

On 18/03/14 20:15, Pavel Stehule wrote:


2014-03-18 20:14 GMT+01:00 Simon Riggs >:


On 18 March 2014 19:04, Pavel Stehule mailto:pavel.steh...@gmail.com>> wrote:

> I don't think so only "shadow" is good name for some check. Maybe
> "shadow-variables-check"

"shadowed-variables" would be a better name.


+1


Attached V4 uses "shadowed-variables" instead of "shadow".

--
 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..d6709fd 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 '$$
   
 
   
+  
+   Additional compile-time checks
+
+   
+To aid the user in finding instances of simple but common problems before
+they cause harm, PL/PgSQL provides a number of additional
+checks. When enabled, depending on the configuration, they
+can be used to emit either a WARNING or an ERROR
+during the compilation of a function.
+   
+
+ 
+  These additional checks are enabled through the configuration variables
+  plpgsql.extra_warnings for warnings and 
+  plpgsql.extra_errors for errors. Both can be set either to
+  a comma-separated list of checks, "none" or "all".
+  The default is "none". Currently the list of available checks
+  includes only one:
+  
+   
+shadowed-variables
+
+ 
+  Checks if a declaration shadows a previously defined variable. For
+  example (with plpgsql.extra_warnings set 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
+
+ 
+
+   
+  
+ 
+ 
  
 
   
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..a8e9210 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, GucSou

Re: [HACKERS] plpgsql.warn_shadow

[Pavel Stehule]

2014-03-18 20:14 GMT+01:00 Simon Riggs :

> On 18 March 2014 19:04, Pavel Stehule  wrote:
>
> > I don't think so only "shadow" is good name for some check. Maybe
> > "shadow-variables-check"
>
> "shadowed-variables" would be a better name.
>

+1


>
> --
>  Simon Riggs   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Re: [HACKERS] plpgsql.warn_shadow

[Simon Riggs]

On 18 March 2014 19:04, Pavel Stehule  wrote:

> I don't think so only "shadow" is good name for some check. Maybe
> "shadow-variables-check"

"shadowed-variables" would be a better name.

-- 
 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] plpgsql.warn_shadow

[Pavel Stehule]

2014-03-18 19:56 GMT+01:00 Petr Jelinek :

>
> On 18/03/14 13:43, Pavel Stehule wrote:
>
> 2014-03-18 13:23 GMT+01:00 Petr Jelinek 
>
>>
>>  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 don't think so only "shadow" is good name for some check. Maybe
"shadow-variables-check"

??


>
> 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
>
>

Re: [HACKERS] plpgsql.warn_shadow

[Petr Jelinek]

On 18/03/14 13:43, Pavel Stehule wrote:
2014-03-18 13:23 GMT+01:00 Petr Jelinek >



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 '$$
   
 
   
+  
+   Additional compile-time checks
+
+   
+To aid the user in finding instances of simple but common problems before
+they cause harm, PL/PgSQL provides a number of additional
+checks. When enabled, depending on the configuration, they
+can be used to emit a WARNING or an ERROR
+during the compilation of a function.
+   
+
+ 
+  These additional checks are enabled through the configuration variables
+  plpgsql.extra_warnings for warnings and 
+  plpgsql.extra_errors for errors. Both can be set either to
+  a comma-separated list of checks, "none" or "all".
+  The default is "none". Currently the list of available checks
+  includes only one:
+  
+   
+shadow
+
+ 
+  Checks if a declaration shadows a previously defined variable. For
+  example (with plpgsql.extra_warnings set to
+  shadow):
+
+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
+
+ 
+
+   
+  
+ 
+ 
  
 
   
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_warn

Re: [HACKERS] plpgsql.warn_shadow

[Simon Riggs]

On 18 March 2014 12:23, Petr Jelinek  wrote:

> Agree that compile_errors dos not make sense, additional_errors and
> additional_warnings seems better, maybe plpgsql.extra_warnings and
> plpgsql.extra_errors?

That sems better to me also.

-- 
 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] plpgsql.warn_shadow

[Pavel Stehule]

2014-03-18 13:23 GMT+01:00 Petr Jelinek :

>
> On 14/03/14 13:12, Simon Riggs wrote:
>
>> On 14 March 2014 11:10, Pavel Stehule  wrote:
>>
>>>
>>>
>>> 2014-03-14 12:02 GMT+01:00 Marko Tiikkaja :
>>>
>>>  On 3/14/14 10:56 AM, Simon Riggs wrote:

> The patch looks fine, apart from some non-guideline code formatting
> issues.
>

 I'm not sure what you're referring to.  I thought it looked fine.


  Having looked at gcc and clang, I have a proposal for naming/API
>
> We just have two variables
>
> plpgsql.compile_warnings = 'list'default = 'none'
>

>>> +1
>>>
>>>  plpgsql.compile_errors = 'list'default = 'none'
>
> Only possible values in 9.4 are 'shadow', 'all', 'none'
>

>>> what is compile_errors ? We don't allow to ignore any error now.
>>>
>> How about
>>
>> plpgsql.additional_warnings = 'list'
>> plpgsql.additional_errors = 'list'
>>
>>
> 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

Pavel


>
> --
>  Petr Jelinek   http://www.2ndQuadrant.com/
>
>  PostgreSQL Development, 24x7 Support, Training & Services
>
>

Re: [HACKERS] plpgsql.warn_shadow

[Petr Jelinek]

On 14/03/14 13:12, Simon Riggs wrote:

On 14 March 2014 11:10, Pavel Stehule  wrote:



2014-03-14 12:02 GMT+01:00 Marko Tiikkaja :


On 3/14/14 10:56 AM, Simon Riggs wrote:

The patch looks fine, apart from some non-guideline code formatting
issues.


I'm not sure what you're referring to.  I thought it looked fine.



Having looked at gcc and clang, I have a proposal for naming/API

We just have two variables

plpgsql.compile_warnings = 'list'default = 'none'


+1


plpgsql.compile_errors = 'list'default = 'none'

Only possible values in 9.4 are 'shadow', 'all', 'none'


what is compile_errors ? We don't allow to ignore any error now.

How about

plpgsql.additional_warnings = 'list'
plpgsql.additional_errors = 'list'



Agree that compile_errors dos not make sense, additional_errors and 
additional_warnings seems better, maybe plpgsql.extra_warnings and 
plpgsql.extra_errors?


--
 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] plpgsql.warn_shadow

[Pavel Stehule]

2014-03-14 13:12 GMT+01:00 Simon Riggs :

> On 14 March 2014 11:10, Pavel Stehule  wrote:
> >
> >
> >
> > 2014-03-14 12:02 GMT+01:00 Marko Tiikkaja :
> >
> >> On 3/14/14 10:56 AM, Simon Riggs wrote:
> >>>
> >>> The patch looks fine, apart from some non-guideline code formatting
> >>> issues.
> >>
> >>
> >> I'm not sure what you're referring to.  I thought it looked fine.
> >>
> >>
> >>> Having looked at gcc and clang, I have a proposal for naming/API
> >>>
> >>> We just have two variables
> >>>
> >>>plpgsql.compile_warnings = 'list'default = 'none'
> >
> >
> > +1
> >
> >>>
> >>>plpgsql.compile_errors = 'list'default = 'none'
> >>>
> >>> Only possible values in 9.4 are 'shadow', 'all', 'none'
> >
> >
> > what is compile_errors ? We don't allow to ignore any error now.
>
> How about
>
> plpgsql.additional_warnings = 'list'
> plpgsql.additional_errors = 'list'
>

I understand .

+1

Pavel


>
> --
>  Simon Riggs   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Re: [HACKERS] plpgsql.warn_shadow

[Simon Riggs]

On 14 March 2014 11:10, Pavel Stehule  wrote:
>
>
>
> 2014-03-14 12:02 GMT+01:00 Marko Tiikkaja :
>
>> On 3/14/14 10:56 AM, Simon Riggs wrote:
>>>
>>> The patch looks fine, apart from some non-guideline code formatting
>>> issues.
>>
>>
>> I'm not sure what you're referring to.  I thought it looked fine.
>>
>>
>>> Having looked at gcc and clang, I have a proposal for naming/API
>>>
>>> We just have two variables
>>>
>>>plpgsql.compile_warnings = 'list'default = 'none'
>
>
> +1
>
>>>
>>>plpgsql.compile_errors = 'list'default = 'none'
>>>
>>> Only possible values in 9.4 are 'shadow', 'all', 'none'
>
>
> what is compile_errors ? We don't allow to ignore any error now.

How about

plpgsql.additional_warnings = 'list'
plpgsql.additional_errors = 'list'

-- 
 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] plpgsql.warn_shadow

[Pavel Stehule]

2014-03-14 12:02 GMT+01:00 Marko Tiikkaja :

> On 3/14/14 10:56 AM, Simon Riggs wrote:
>
>> The patch looks fine, apart from some non-guideline code formatting
>> issues.
>>
>
> I'm not sure what you're referring to.  I thought it looked fine.
>
>
>  Having looked at gcc and clang, I have a proposal for naming/API
>>
>> We just have two variables
>>
>>plpgsql.compile_warnings = 'list'default = 'none'
>>
>
+1


> plpgsql.compile_errors = 'list'default = 'none'
>>
>> Only possible values in 9.4 are 'shadow', 'all', 'none'
>>
>
what is compile_errors ? We don't allow to ignore any error now.



>
> I'm fine with this.  I'm starting to think that runtime warnings are a bad
> idea anyway.
>

+1

Pavel


>
>
> Regards,
> Marko Tiikkaja
>

Re: [HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

On 3/14/14 10:56 AM, Simon Riggs wrote:

The patch looks fine, apart from some non-guideline code formatting issues.


I'm not sure what you're referring to.  I thought it looked fine.


Having looked at gcc and clang, I have a proposal for naming/API

We just have two variables

   plpgsql.compile_warnings = 'list'default = 'none'
   plpgsql.compile_errors = 'list'default = 'none'

Only possible values in 9.4 are 'shadow', 'all', 'none'


I'm fine with this.  I'm starting to think that runtime warnings are a 
bad idea anyway.



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] plpgsql.warn_shadow

[Simon Riggs]

On 3 February 2014 20:17, Pavel Stehule  wrote:
> Hello
>
> I am not happy from "warnings_as_error"
>
> what about "stop_on_warning" instead?
>
> second question: should be these errors catchable or uncatchable?
>
> When I work on large project, where I had to use some error handler of
> "EXCEPTION WHEN OTHERS" I found very strange and not useful so all syntax
> errors was catched by this handler. Any debugging was terribly difficult and
> I had to write plpgsql_lint as solution.

The patch looks fine, apart from some non-guideline code formatting issues.

Having looked at gcc and clang, I have a proposal for naming/API

We just have two variables

  plpgsql.compile_warnings = 'list'default = 'none'
  plpgsql.compile_errors = 'list'default = 'none'

Only possible values in 9.4 are 'shadow', 'all', 'none'

If we can agree something quickly then we can commit this for 9.4

-- 
 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] plpgsql.warn_shadow

[Andrew Dunstan]

On 03/04/2014 03:40 PM, Joel Jacobson wrote:

On Tue, Mar 4, 2014 at 8:04 PM, Andrew Dunstan  wrote:

Lots of code quite correctly relies on this,
including some I have written.

I really cannot see when it would be a good coding practise to do so,
there must be something I don't understand, I would greatly appreciate
if you can give a real-life example of such a PL/pgSQL function.




I can't give you one because it's not mine to share. But I can tell you 
a couple of ways I have seen it come about.


One is when a piece if code is re-used in another function which happens 
to have a parameter name which is the same. Another is when translating 
some code and this is the simplest way to do it, with the least effort 
involved.


If I am writing a piece of green fields code, than like you I avoid 
this. But the vast majority of what I do for people is not green fields 
code.


In any case, it's not our responsibility to enforce a coding standard. 
That's a management issue, in the end, not a technological issue.


cheers

andrew


--
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] plpgsql.warn_shadow

[Joel Jacobson]

On Tue, Mar 4, 2014 at 8:04 PM, Andrew Dunstan  wrote:
> Lots of code quite correctly relies on this,
> including some I have written.

I really cannot see when it would be a good coding practise to do so,
there must be something I don't understand, I would greatly appreciate
if you can give a real-life example of such a PL/pgSQL function.


-- 
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] plpgsql.warn_shadow

[Andrew Dunstan]

On 03/04/2014 11:23 AM, Joel Jacobson wrote:


I understand that from a technical perspective, the mandatory
BEGIN...END you always need in a PL/pgSQL function, is a new block,
and the variables declared are perhaps technically in a new block, at
a deeper level than the IN/OUT variables. But I would still argue the
expected behaviour of PL/pgSQL for a new user would be to consider the
IN/OUT variables to be in the same block as the variables declared in
the function's first block.




No they are not. Teaching a new user to consider them as the same is 
simply wrong.


The parameters belong to a block that matches the function name. The 
outermost block has a different name if supplied (I usually use <>), 
or is otherwise anonymous. Lots of code quite correctly relies on this, 
including some I have written.


This isn't a mere technical difference, and there is surely zero chance 
that we will label use of it an error under any circumstances.


cheers

andrew



--
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] plpgsql.warn_shadow

[Joel Jacobson]

On Tue, Mar 4, 2014 at 4:06 PM, Tom Lane  wrote:
> Joel Jacobson  writes:
>> On Tue, Mar 4, 2014 at 12:55 AM, Tom Lane  wrote:
>>> You're reasoning from a false premise: it's *not* necessarily an error.
>
>> Isn't this almost exactly the same situation as we had in 9.0?
>> "PL/pgSQL now throws an error if a variable name conflicts with a
>> column name used in a query (Tom Lane)"
>
> No; the reason why the old behavior was problematic was precisely that
> it failed to conform to normal block-structured language design rules
> (namely that the most closely nested definition should win).  If it
> had been like that to start with we'd probably have just left it that
> way.  The complexity of behavior that you see there today is there to
> help people with debugging issues created by that change of behavior.
>
> While I don't necessarily have an objection to creating a way to help
> debug variable-name-shadowing issues, the idea that they're broken and
> we can just start throwing errors is *wrong*.  The whole point of block
> structure in a language is that a block of code can be understood
> independently of what surrounds it.

I agree it should be possible to reuse a variable in a new block,
but I think the IN/OUT variable should be considered to be at the
*same* block-level
as the first block of code, thus an error should be thrown.

Consider the same scenario in for instance Perl:

# Example 1
# Prints "1" and doesn't throw an error, which is perfectly OK.
use warnings;
my $foo = 1;
{
my $foo = 2;
}
print $foo;

# Example 2
# "my" variable $foo masks earlier declaration in same scope at
warn_shadow.pl line 3.
use warnings;
my $foo = 1;
my $foo = 2;
print $foo;

Or maybe this is a better example, since we are talking about functions:

# Example 3
# "my" variable $bar masks earlier declaration in same scope at
warn_shadow.pl line 7.
use warnings;
sub foo
{
# IN-variables:
my ($bar) = @_;
# DECLARE:
my $bar;
# BEGIN:
$bar = 1;
return $bar;
}
foo(2);


I understand that from a technical perspective, the mandatory
BEGIN...END you always need in a PL/pgSQL function, is a new block,
and the variables declared are perhaps technically in a new block, at
a deeper level than the IN/OUT variables. But I would still argue the
expected behaviour of PL/pgSQL for a new user would be to consider the
IN/OUT variables to be in the same block as the variables declared in
the function's first block.


-- 
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] plpgsql.warn_shadow

[Tom Lane]

Joel Jacobson  writes:
> On Tue, Mar 4, 2014 at 12:55 AM, Tom Lane  wrote:
>> You're reasoning from a false premise: it's *not* necessarily an error.

> Isn't this almost exactly the same situation as we had in 9.0?
> "PL/pgSQL now throws an error if a variable name conflicts with a
> column name used in a query (Tom Lane)"

No; the reason why the old behavior was problematic was precisely that
it failed to conform to normal block-structured language design rules
(namely that the most closely nested definition should win).  If it
had been like that to start with we'd probably have just left it that
way.  The complexity of behavior that you see there today is there to
help people with debugging issues created by that change of behavior.

While I don't necessarily have an objection to creating a way to help
debug variable-name-shadowing issues, the idea that they're broken and
we can just start throwing errors is *wrong*.  The whole point of block
structure in a language is that a block of code can be understood
independently of what surrounds 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] plpgsql.warn_shadow

[Joel Jacobson]

On Tue, Mar 4, 2014 at 12:55 AM, Tom Lane  wrote:
> You're reasoning from a false premise: it's *not* necessarily an error.

When wouldn't it be an error? Can you give a real-life example of when
it would be a good idea to use the same name of an input parameter as
a declared variable?

Isn't this almost exactly the same situation as we had in 9.0?
"PL/pgSQL now throws an error if a variable name conflicts with a
column name used in a query (Tom Lane)"
(http://www.postgresql.org/docs/9.1/static/release-9-0.html)

Making variables in conflict with column names an error was a very good thing.
Making variables in conflict with in/out parameters would also be a
very good thing, by default. And for the ones who are unable to fix
their code, let them turn the error off by a setting. Maybe we don't
even need a new setting, maybe the existing plpgsql.variable_conflict
could be reused?


-- 
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] plpgsql.warn_shadow

[Tom Lane]

Joel Jacobson  writes:
> I strongly think it should be made an error, because it is most
> certainly an error, and even if it's not, it's at least bad coding
> style and the code should be fixed anyway, or if one is lazy, turn
> this off in the config file and make it a warning instead.

You're reasoning from a false premise: it's *not* necessarily an error.
If this were such a bad idea as you claim, generations of programming
language designers wouldn't have made their languages work like this.

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] plpgsql.warn_shadow

[Joel Jacobson]

On Wed, Jan 15, 2014 at 1:34 AM, Marko Tiikkaja  wrote:
> Hi all!
>
> It's me again, trying to find a solution to the most common mistakes I make.
> This time it's accidental shadowing of variables, especially input
> variables.  I've wasted several hours banging my head against the wall while
> shouting "HOW CAN THIS VARIABLE ALWAYS BE NULL?".  I can't believe I'm the
> only one.  To give you a rough idea on how it works:

+1

I've made the same mistake countless of times.
For me, it always happens when I have a declared variable in a
function which I later on make an input parameter instead and forget
to remove it under the declare section of the function.

Regarding the warning vs error discussion, I think it depends on if we
want to maximize short-term or long-term user-friendliness.
In the short-term it's more user-friendly to lie to the user and
pretend everything is fine by not crashing the PL/pgSQL-application
and instead writing warning messages in the log file which might not
be seen. But in the long-term the user will run into problems caused
by the bugs.
With MySQL, invalid dates are accepted unless special settings are
turned on, but yes, warnings are emitted which most implementations
ignore. This is bad.

I strongly think it should be made an error, because it is most
certainly an error, and even if it's not, it's at least bad coding
style and the code should be fixed anyway, or if one is lazy, turn
this off in the config file and make it a warning instead.

I don't think we should be too afraid to break backward compability if
it only affects a theoretical percentage of the users in a new major
release.


-- 
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] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/27/14, 12:04 PM, Simon Riggs wrote:

Florian's point was well made and we must come up with something that
allows warning/errors at compile time and once accepted, nothing at
run time.


I've been thinking about this, and I'm not sure I like this idea all 
that much.  For compile-time warnings this would probably be the ideal 
behaviour, but if we were to add any run-time warnings (e.g. 
consistent_into) I'm not sure how I would use such a feature.


Say I run my test suite with all warnings enabled, and now I want to 
know that it didn't produce any warnings.  How would I do that?  I guess 
I could grep the log for warning messages, but then I'd have to maintain 
a list of all warnings somewhere, which seems quite ugly.  A special 
SQLSTATE for PL/PgSQL warnings doesn't seem that much better.


But maybe I'm overthinking this and we'd only ever have one runtime 
warning (or maybe none at all, even) and this would not be an issue in 
practice.



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] plpgsql.warn_shadow

[Pavel Stehule]

Dne 3.2.2014 21:48 "Marko Tiikkaja"  napsal(a):
>
> On 2014-02-03 9:17 PM, Pavel Stehule wrote:
>>
>> I am not happy from "warnings_as_error"
>>
>> what about "stop_on_warning" instead?
>
>
> abort_compilation_on_warning?  I think if we're not going to make it more
clear that warnings are only promoted to errors at CREATE FUNCTION time, we
can't do much better than warnings_as_errors.
>
>
>> second question: should be these errors catchable or uncatchable?
>>
>> When I work on large project, where I had to use some error handler of
>> "EXCEPTION WHEN OTHERS" I found very strange and not useful so all syntax
>> errors was catched by this handler. Any debugging was terribly difficult
>> and I had to write plpgsql_lint as solution.
>
>
> Is this really an issue considering these errors can only happen when
someone runs CREATE FUNCTION?

Can you look, pls, what terminology is used in gcc, clang, perl or python.

I agree with idea, but proposed names I have not associated with something.

Regards

Pavel
>
>
> Regards,
> Marko Tiikkaja

Re: [HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

On 2014-02-03 9:17 PM, Pavel Stehule wrote:

I am not happy from "warnings_as_error"

what about "stop_on_warning" instead?


abort_compilation_on_warning?  I think if we're not going to make it 
more clear that warnings are only promoted to errors at CREATE FUNCTION 
time, we can't do much better than warnings_as_errors.



second question: should be these errors catchable or uncatchable?

When I work on large project, where I had to use some error handler of
"EXCEPTION WHEN OTHERS" I found very strange and not useful so all syntax
errors was catched by this handler. Any debugging was terribly difficult
and I had to write plpgsql_lint as solution.


Is this really an issue considering these errors can only happen when 
someone runs CREATE FUNCTION?



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] plpgsql.warn_shadow

[Pavel Stehule]

Hello

I am not happy from "warnings_as_error"

what about "stop_on_warning" instead?

second question: should be these errors catchable or uncatchable?

When I work on large project, where I had to use some error handler of
"EXCEPTION WHEN OTHERS" I found very strange and not useful so all syntax
errors was catched by this handler. Any debugging was terribly difficult
and I had to write plpgsql_lint as solution.

Regards

Pavel


2014-02-03 Marko Tiikkaja :

> 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
>

Re: [HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

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 

  

+   
+Warnings
+ 
+
+ To aid the user in finding instances of simple but common problems before
+ they cause harm, PL/PgSQL provides a number of
+ warnings.  When enabled, a WARNING is emitted
+ during the compilation of a function.  Optionally, if
+ plpgsql.warnings_as_errors is set, an ERROR is
+ raised when attempting to create a function which would otherwise result 
in
+ a warning.
+
+ 
+  
+   Warnings are enabled through the configuration variable
+   plpgsql.warnings.  It can be set either to a comma-separated 
list
+   of warnings, "none" or "all".  The default is
+   "none".  Currently the list of available warnings includes only
+   one:
+   
+
+ shadow
+ 
+  
+   Warns when a declaration shadows a previously defined variable.  For
+   example:
+ 
+ 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
+ 
+  
+ 
+
+   
+  
+  
   
  

*** 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),
+ 

Re: [HACKERS] plpgsql.warn_shadow

[Simon Riggs]

On 27 January 2014 10:40, Marti Raudsepp  wrote:
> On Sun, Jan 26, 2014 at 11:19 AM, Simon Riggs  wrote:
>> For 9.4, we should cut down the patch so it has
>>   plpgsql.warnings = none (default) | all | [individual item list]
>
>>   plpgsql.warnings_as_errors = off (default) | on
>
> I hope I'm not late for the bikeshedding :)
>
> Why not have 2 similar options:
> plpgsql.warnings = none (default) | all | [individual item list]
> plpgsql.errors = none (default) | all | [individual item list]
>
> That would be cleaner, more flexible, and one less option to
> set if you just want errors and no warnings.

That would allow you to mis-set the parameters and then cause a
runtime error for something that was only a warning at compile time.

Florian's point was well made and we must come up with something that
allows warning/errors at compile time and once accepted, nothing at
run time.

-- 
 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] plpgsql.warn_shadow

[Pavel Stehule]

2014-01-27 Marti Raudsepp 

> On Sun, Jan 26, 2014 at 11:19 AM, Simon Riggs 
> wrote:
> > For 9.4, we should cut down the patch so it has
> >   plpgsql.warnings = none (default) | all | [individual item list]
>
> >   plpgsql.warnings_as_errors = off (default) | on
>
> I hope I'm not late for the bikeshedding :)
>
> Why not have 2 similar options:
> plpgsql.warnings = none (default) | all | [individual item list]
> plpgsql.errors = none (default) | all | [individual item list]
>
> That would be cleaner, more flexible, and one less option to
> set if you just want errors and no warnings.
>

what means plpgsql.errors = none ???

I strongly disagree so this design is clean

Regards

Pavel


>
> Regards,
> Marti
>
>
> --
> 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] plpgsql.warn_shadow

[Marti Raudsepp]

On Sun, Jan 26, 2014 at 11:19 AM, Simon Riggs  wrote:
> For 9.4, we should cut down the patch so it has
>   plpgsql.warnings = none (default) | all | [individual item list]

>   plpgsql.warnings_as_errors = off (default) | on

I hope I'm not late for the bikeshedding :)

Why not have 2 similar options:
plpgsql.warnings = none (default) | all | [individual item list]
plpgsql.errors = none (default) | all | [individual item list]

That would be cleaner, more flexible, and one less option to
set if you just want errors and no warnings.

Regards,
Marti


-- 
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] plpgsql.warn_shadow

[Pavel Stehule]

> > Putting this and all future options as keywords seems like a poor
> > choice. Indeed, the # syntax proposed isn't even fully described on
> > list here, nor are examples given in tests. My feeling is that nobody
> > even knows that is being proposed and would likely cause more
> > discussion if they did. So I wish to push back the # syntax to a later
> > release when it has had more discussion. It would be good if you could
> > lead that discussion in later releases.
>

I am returning back to #option syntax

a) it should not be plpgsql keywords
b) it can be nice if validity can be verified by plpgsql plugins and used
by plpgsql plugins much more. Now we can use only GUC for plugin
parametrization, but it is not readable as #option it is.

Regards

Pavel

Re: [HACKERS] plpgsql.warn_shadow

[Simon Riggs]

On 26 January 2014 22:44, Pavel Stehule  wrote:
>
> Dne 26. 1. 2014 23:24 "Simon Riggs"  napsal(a):
>
>
>>
>> On 26 January 2014 15:53, Pavel Stehule  wrote:
>> >
>> >
>> >
>> > 2014-01-26 Florian Pflug 
>> >
>> >> On Jan26, 2014, at 10:19 , Simon Riggs  wrote:
>> >> > Also, having
>> >> >  plpgsql.warnings_as_errors = off (default) | on
>> >> > makes sense and should be included in 9.4
>> >>
>> >> I still think this is a bad idea, for the same reasons I don't like
>> >> consistent_into (discussed in a separate thread).
>> >>
>> >> But these objections would go away if restricted this to function
>> >> creation time only. So even with warnings_as_errors=on, you
>> >> could still *call* a function that produces a warning, but not
>> >> *create* one.
>> >
>> >
>> > +1 behave - and please, better name
>>
>> +1 to that.
>>
>> I guess I only saw that way of working because I was thinking of it as
>> a "compiler warning".
>>
>> So perhaps we should call it
>>   plpgsql.compiler_warnings_as_errors
>>
>> to make that behaviour more clear.
>>
>>   plpgsql.error_on_create_warnings
>
> I have a problém with joining word error and warning together.
>
> Some like stop_on_compilation_warning or strict_compiler sound me more
> logical

Not bothered by the name, so lets wait for Marko to produce a patch
and then finalise the naming.

-- 
 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] plpgsql.warn_shadow

[Pavel Stehule]

Dne 26. 1. 2014 23:24 "Simon Riggs"  napsal(a):
>
> On 26 January 2014 15:53, Pavel Stehule  wrote:
> >
> >
> >
> > 2014-01-26 Florian Pflug 
> >
> >> On Jan26, 2014, at 10:19 , Simon Riggs  wrote:
> >> > Also, having
> >> >  plpgsql.warnings_as_errors = off (default) | on
> >> > makes sense and should be included in 9.4
> >>
> >> I still think this is a bad idea, for the same reasons I don't like
> >> consistent_into (discussed in a separate thread).
> >>
> >> But these objections would go away if restricted this to function
> >> creation time only. So even with warnings_as_errors=on, you
> >> could still *call* a function that produces a warning, but not
> >> *create* one.
> >
> >
> > +1 behave - and please, better name
>
> +1 to that.
>
> I guess I only saw that way of working because I was thinking of it as
> a "compiler warning".
>
> So perhaps we should call it
>   plpgsql.compiler_warnings_as_errors
>
> to make that behaviour more clear.
>
>   plpgsql.error_on_create_warnings

I have a problém with joining word error and warning together.

Some like stop_on_compilation_warning or strict_compiler sound me more
logical

Regards

Pavel
>
> --
>  Simon Riggs   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] plpgsql.warn_shadow

[Simon Riggs]

On 26 January 2014 15:53, Pavel Stehule  wrote:
>
>
>
> 2014-01-26 Florian Pflug 
>
>> On Jan26, 2014, at 10:19 , Simon Riggs  wrote:
>> > Also, having
>> >  plpgsql.warnings_as_errors = off (default) | on
>> > makes sense and should be included in 9.4
>>
>> I still think this is a bad idea, for the same reasons I don't like
>> consistent_into (discussed in a separate thread).
>>
>> But these objections would go away if restricted this to function
>> creation time only. So even with warnings_as_errors=on, you
>> could still *call* a function that produces a warning, but not
>> *create* one.
>
>
> +1 behave - and please, better name

+1 to that.

I guess I only saw that way of working because I was thinking of it as
a "compiler warning".

So perhaps we should call it
  plpgsql.compiler_warnings_as_errors

to make that behaviour more clear.

  plpgsql.error_on_create_warnings

-- 
 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] plpgsql.warn_shadow

[Pavel Stehule]

2014-01-26 Florian Pflug 

> On Jan26, 2014, at 10:19 , Simon Riggs  wrote:
> > Also, having
> >  plpgsql.warnings_as_errors = off (default) | on
> > makes sense and should be included in 9.4
>
> I still think this is a bad idea, for the same reasons I don't like
> consistent_into (discussed in a separate thread).
>
> But these objections would go away if restricted this to function
> creation time only. So even with warnings_as_errors=on, you
> could still *call* a function that produces a warning, but not
> *create* one.
>

+1 behave - and please, better name

Regards

Pavel




>
> We could then integrate this with check_function_bodies, i.e. add a
> third possible value "error_on_warnings" to that GUC, instead of
> having a separate GUC for this.
>
> > Putting this and all future options as keywords seems like a poor
> > choice. Indeed, the # syntax proposed isn't even fully described on
> > list here, nor are examples given in tests. My feeling is that nobody
> > even knows that is being proposed and would likely cause more
> > discussion if they did. So I wish to push back the # syntax to a later
> > release when it has had more discussion. It would be good if you could
> > lead that discussion in later releases.
>
> +1
>
> best regards,
> Florian Pflug
>
>
>
> --
> 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] plpgsql.warn_shadow

[Florian Pflug]

On Jan26, 2014, at 10:19 , Simon Riggs  wrote:
> Also, having
>  plpgsql.warnings_as_errors = off (default) | on
> makes sense and should be included in 9.4

I still think this is a bad idea, for the same reasons I don't like
consistent_into (discussed in a separate thread).

But these objections would go away if restricted this to function
creation time only. So even with warnings_as_errors=on, you
could still *call* a function that produces a warning, but not
*create* one.

We could then integrate this with check_function_bodies, i.e. add a
third possible value "error_on_warnings" to that GUC, instead of
having a separate GUC for this.

> Putting this and all future options as keywords seems like a poor
> choice. Indeed, the # syntax proposed isn't even fully described on
> list here, nor are examples given in tests. My feeling is that nobody
> even knows that is being proposed and would likely cause more
> discussion if they did. So I wish to push back the # syntax to a later
> release when it has had more discussion. It would be good if you could
> lead that discussion in later releases.

+1

best regards,
Florian Pflug



-- 
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] plpgsql.warn_shadow

[Simon Riggs]

On 15 January 2014 00:34, Marko Tiikkaja  wrote:
> Hi all!
>
> It's me again, trying to find a solution to the most common mistakes I make.
> This time it's accidental shadowing of variables, especially input
> variables.  I've wasted several hours banging my head against the wall while
> shouting "HOW CAN THIS VARIABLE ALWAYS BE NULL?".  I can't believe I'm the
> only one.  To give you a rough idea on how it works:
>
> =# set plpgsql.warn_shadow to true;
> SET
> =#* create function test(in1 int, in2 int)
> -#* returns table(out1 int, out2 int) as $$
> $#* declare
> $#* IN1 text;
> $#* IN2 text;
> $#* out1 text;
> $#* begin
> $#*
> $#* declare
> $#* out2 text;
> $#* in1 text;
> $#* begin
> $#* end;
> $#* end$$ language plpgsql;
> WARNING:  variable "in1" shadows a previously defined variable
> LINE 4: IN1 text;
> ^
> WARNING:  variable "in2" shadows a previously defined variable
> LINE 5: IN2 text;
> ^
> WARNING:  variable "out1" shadows a previously defined variable
> LINE 6: out1 text;
> ^
> WARNING:  variable "out2" shadows a previously defined variable
> LINE 10: out2 text;
>  ^
> WARNING:  variable "in1" shadows a previously defined variable
> LINE 11: in1 text;
>  ^
> CREATE FUNCTION
>
>
> No behaviour change by default.  Even setting the GUC doesn't really change
> behaviour, just emits some warnings when a potentially faulty function is
> compiled.
>
> Let me know what you think so I can either cry or clean the patch up.

Looking at the patch and reading comments there is something here of
value. Some aspects need further consideration and I would like to
include some in 9.4 and push back others to later releases. This is
clearly a very useful contribution and the right direction for further
work. Suggesting fixes to your own problems is a very good way to
proceed...

For 9.4, we should cut down the patch so it has
  plpgsql.warnings = none (default) | all | [individual item list]
This syntax can then be extended in later releases to include further
individual items, without requiring they be listed individually -
which then becomes release dependent behaviour.

Also, having
  plpgsql.warnings_as_errors = off (default) | on
makes sense and should be included in 9.4

Putting this and all future options as keywords seems like a poor
choice. Indeed, the # syntax proposed isn't even fully described on
list here, nor are examples given in tests. My feeling is that nobody
even knows that is being proposed and would likely cause more
discussion if they did. So I wish to push back the # syntax to a later
release when it has had more discussion. It would be good if you could
lead that discussion in later releases.

Please add further tests, with comments that explain why the WARNING
is given. Those should include complex situations like double
shadowing, not just the basics. And docs, of course.

Last CF, so do this soon so we can commit. Thanks.

-- 
 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] plpgsql.warn_shadow

[Florian Pflug]

On Jan20, 2014, at 14:05 , Marko Tiikkaja  wrote:
> On 1/20/14 1:55 PM, Robert Haas wrote:
>> On Mon, Jan 20, 2014 at 7:16 AM, Marko Tiikkaja  wrote:
>>> What's so hard about plpgsql.warnings='all'?  Or if the fact that it's a
>>> list is your concern, I'm not going to oppose to making it a boolean.
>> 
>> Sure, that'd be fine.  What I don't want is to have to start each function 
>> with:
>> 
>> #option warn_this
>> #option warn_that
>> #option warn_theotherthing
>> #option warn_somethingelse
>> #option warn_yetanotherthing
>> #option warn_whatdoesthisdoagain
> 
> Right.  Completely agreed.  The only reason I had them in the patch is to 
> have the
> ability to turn *off* a specific warning for a particular function.  But even
> that's of a bit dubious a value.

I think as long as there's an easy way to avoid a warning - in the case of
warn_shadow e.g. by renaming the shadowing variable - there's no real 
requirement
to be able to disable the warning on a per-function basis.

And if there isn't a simple way to avoid a particular warning then we
ought not warn about it anyway, I guess, because that would indicate that there
are genuine reasons for doing whatever it is the warning complains about.

best regards,
Florian Pflug




-- 
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] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/20/14 1:55 PM, Robert Haas wrote:

On Mon, Jan 20, 2014 at 7:16 AM, Marko Tiikkaja  wrote:

What's so hard about plpgsql.warnings='all'?  Or if the fact that it's a
list is your concern, I'm not going to oppose to making it a boolean.


Sure, that'd be fine.  What I don't want is to have to start each function with:

#option warn_this
#option warn_that
#option warn_theotherthing
#option warn_somethingelse
#option warn_yetanotherthing
#option warn_whatdoesthisdoagain


Right.  Completely agreed.  The only reason I had them in the patch is 
to have the ability to turn *off* a specific warning for a particular 
function.  But even that's of a bit dubious a value.



Also, I think that the way we've been doing it, each of those needs to
become a PL/pgsql keyword.  That's going to become a problem at some
point.


Yeah, probably. :-(


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] plpgsql.warn_shadow

[Robert Haas]

On Mon, Jan 20, 2014 at 7:16 AM, Marko Tiikkaja  wrote:
> What's so hard about plpgsql.warnings='all'?  Or if the fact that it's a
> list is your concern, I'm not going to oppose to making it a boolean.

Sure, that'd be fine.  What I don't want is to have to start each function with:

#option warn_this
#option warn_that
#option warn_theotherthing
#option warn_somethingelse
#option warn_yetanotherthing
#option warn_whatdoesthisdoagain

Also, I think that the way we've been doing it, each of those needs to
become a PL/pgsql keyword.  That's going to become a problem at some
point.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/20/14 2:25 AM, Robert Haas wrote:

On Fri, Jan 17, 2014 at 5:45 AM, Marko Tiikkaja  wrote:

I see these as being two are different things.  There *is* a need for a
full-blown static analyzer for PL/PgSQL, but I don't think it needs to be in
core.  However, there seems to be a number of pitfalls we could warn the
user about with little effort in core, and I think those are worth doing.


I don't want to be overly negative, but that sounds sort of like
you're saying that it's not worth having a good static analyzer in
core, but that you are in favor of having a bad one.


Sort of, yeah.

My experience of static analyzers is that it's not really feasible to 
try and fix all code they think might be faulty, and I don't expect a 
PL/PgSQL one to be any different.  The idea behind these warnings (to 
me, at least) has been that they're simple and uncontroversial enough 
that it's feasible to say "never commit code which produces warnings 
upon compilation".  I realize that where to draw that line is a bit 
arbitrary and subjective, and I don't expect everyone to agree with me 
on the exact list of these "warnings".



I personally tend to think a static analyzer is a better fit than
adding a whole laundry list of pragmas.  Most people won't remember to
turn them all on anyway> , and those who do will find that it gets
pretty tedious after we have more than about two of them.


What's so hard about plpgsql.warnings='all'?  Or if the fact that it's a 
list is your concern, I'm not going to oppose to making it a boolean.



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] plpgsql.warn_shadow

[Robert Haas]

On Fri, Jan 17, 2014 at 5:45 AM, Marko Tiikkaja  wrote:
> On 1/17/14 11:26 AM, Pavel Stehule wrote:
>>
>> After some thinking I don't think so this design is not good. It  changing
>> a working with exception (error) levels - and it is not consistent with
>> other PostgreSQL parts.
>>
>> A benefit is less than not clean configuration. Better to solve similar
>> issues via specialized plpgsql extensions or try to help me push
>> plpgsql_check_function to core. It can be a best holder for this and
>> similar checks.
>
>
> I see these as being two are different things.  There *is* a need for a
> full-blown static analyzer for PL/PgSQL, but I don't think it needs to be in
> core.  However, there seems to be a number of pitfalls we could warn the
> user about with little effort in core, and I think those are worth doing.

I don't want to be overly negative, but that sounds sort of like
you're saying that it's not worth having a good static analyzer in
core, but that you are in favor of having a bad one.

I personally tend to think a static analyzer is a better fit than
adding a whole laundry list of pragmas.  Most people won't remember to
turn them all on anyway, and those who do will find that it gets
pretty tedious after we have more than about two of them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/17/14 11:26 AM, Pavel Stehule wrote:

After some thinking I don't think so this design is not good. It  changing
a working with exception (error) levels - and it is not consistent with
other PostgreSQL parts.

A benefit is less than not clean configuration. Better to solve similar
issues via specialized plpgsql extensions or try to help me push
plpgsql_check_function to core. It can be a best holder for this and
similar checks.


I see these as being two are different things.  There *is* a need for a 
full-blown static analyzer for PL/PgSQL, but I don't think it needs to 
be in core.  However, there seems to be a number of pitfalls we could 
warn the user about with little effort in core, and I think those are 
worth doing.



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] plpgsql.warn_shadow

[Pavel Stehule]

Hello

After some thinking I don't think so this design is not good. It  changing
a working with exception (error) levels - and it is not consistent with
other PostgreSQL parts.

A benefit is less than not clean configuration. Better to solve similar
issues via specialized plpgsql extensions or try to help me push
plpgsql_check_function to core. It can be a best holder for this and
similar checks.

Regards

Pavel




2014/1/15 Marko Tiikkaja 

> On 1/15/14 3:09 PM, Pavel Stehule wrote:
>
>> You first should to say, what is warning and why it is only warning and
>> not
>> error.
>>
>
> Personally, I'm a huge fan of the computer telling me that I might have
> made a mistake.  But on the other hand, I also really hate it when the
> computer gets in my way when I'm trying to test something quickly and
> making these mistakes on purpose.  Warnings are really good for that: hard
> to ignore (YMMV) accidentally, but easy to spot when developing.
>
> As to how we would categorize these checks between warnings and errors..
>  I can't really answer that.  I'm tempted to say "anything that is an error
> now is an error, any additional checks we might add are warnings", but
> that's effectively just freezing the definition at an arbitrary point in
> time.
>
>
>  And why plpgsql warning processing should be different than general
>> postgresql processing?
>>
>
> What do you mean?  We're adding extra checks on *top* of the normal "this
> is clearly an error" conditions.  PostgreSQL in general doesn't really do
> that.  Consider:
>
>   SELECT * FROM foo WHERE fooid IN (SELECT fooid FROM bar);
>
> where the table "bar" doesn't have a column "fooid".  That's a perfectly
> valid query, but it almost certainly doesn't do what you would want.
> Personally I'd like to see a WARNING here normally, but I've eventually
> learned to live with this caveat.  I'm hoping that in PL/PgSQL we could at
> least solve some of the most annoying pitfalls.
>
>
>  My objection is against too general option. Every option shoudl to do one
>> clean thing.
>>
>
> It looks to me like the GUC *is* doing only one thing.  "list of warnings
> I want to see", or the shorthand "all" for convenience.
>
>
> Regards,
> Marko Tiikkaja
>

Re: [HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/15/14 3:09 PM, Pavel Stehule wrote:

You first should to say, what is warning and why it is only warning and not
error.


Personally, I'm a huge fan of the computer telling me that I might have 
made a mistake.  But on the other hand, I also really hate it when the 
computer gets in my way when I'm trying to test something quickly and 
making these mistakes on purpose.  Warnings are really good for that: 
hard to ignore (YMMV) accidentally, but easy to spot when developing.


As to how we would categorize these checks between warnings and errors.. 
 I can't really answer that.  I'm tempted to say "anything that is an 
error now is an error, any additional checks we might add are warnings", 
but that's effectively just freezing the definition at an arbitrary 
point in time.



And why plpgsql warning processing should be different than general
postgresql processing?


What do you mean?  We're adding extra checks on *top* of the normal 
"this is clearly an error" conditions.  PostgreSQL in general doesn't 
really do that.  Consider:


  SELECT * FROM foo WHERE fooid IN (SELECT fooid FROM bar);

where the table "bar" doesn't have a column "fooid".  That's a perfectly 
valid query, but it almost certainly doesn't do what you would want. 
Personally I'd like to see a WARNING here normally, but I've eventually 
learned to live with this caveat.  I'm hoping that in PL/PgSQL we could 
at least solve some of the most annoying pitfalls.



My objection is against too general option. Every option shoudl to do one
clean thing.


It looks to me like the GUC *is* doing only one thing.  "list of 
warnings I want to see", or the shorthand "all" for convenience.



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] plpgsql.warn_shadow

[Pavel Stehule]

2014/1/15 Marko Tiikkaja 

> On 1/15/14 2:27 PM, Pavel Stehule wrote:
>
>> 2014/1/15 Marko Tiikkaja 
>>
>>> Yeah, me neither, it's just something that needs to be communicated very
>>> clearly.  So probably just a list  plpgsql.warnings  would be the most
>>> appropriate then.
>>>
>>>
>> I am thinking so the name is not good. Changing handling warnings is messy
>> - minimally in Postgres, where warnings and errors are different
>> creatures.
>>
>> what about
>>
>> plpgsql.enhanced_checks = (none | warning | error)
>>
>
> You crammed several suggestions into one here:
>
>   1) You're advocating the ability to turn warnings into errors.  This has
> been met with some resistance.  I think it's a useful feature, but I would
> be happy with just having warnings available.
>   2) This syntax doesn't allow the user to specify a list of warnings to
> enable.  Which might be fine, I guess.  I imagine the normal approach would
> be to turn all warnings on anyway, and possibly fine-tune with per-function
> directives if some functions do dangerous things on purpose.
>   3) You want to change the name to "enhanced_checks".  I still think the
> main feature is about displaying warnings to the user.  I don't
> particularly like this suggestion.
>

You first should to say, what is warning and why it is only warning and not
error. And why plpgsql warning processing should be different than general
postgresql processing?

My objection is against too general option. Every option shoudl to do one
clean thing.

Regards

Pavel


>
>
> Regards,
> Marko Tiikkaja
>

Re: [HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/15/14 2:27 PM, Pavel Stehule wrote:

2014/1/15 Marko Tiikkaja 

Yeah, me neither, it's just something that needs to be communicated very
clearly.  So probably just a list  plpgsql.warnings  would be the most
appropriate then.



I am thinking so the name is not good. Changing handling warnings is messy
- minimally in Postgres, where warnings and errors are different creatures.

what about

plpgsql.enhanced_checks = (none | warning | error)


You crammed several suggestions into one here:

  1) You're advocating the ability to turn warnings into errors.  This 
has been met with some resistance.  I think it's a useful feature, but I 
would be happy with just having warnings available.
  2) This syntax doesn't allow the user to specify a list of warnings 
to enable.  Which might be fine, I guess.  I imagine the normal approach 
would be to turn all warnings on anyway, and possibly fine-tune with 
per-function directives if some functions do dangerous things on purpose.
  3) You want to change the name to "enhanced_checks".  I still think 
the main feature is about displaying warnings to the user.  I don't 
particularly like this suggestion.



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] plpgsql.warn_shadow

[Pavel Stehule]

2014/1/15 Marko Tiikkaja 

> On 1/15/14 2:00 PM, Florian Pflug wrote:
>
>> On Jan15, 2014, at 13:32 , Marko Tiikkaja  wrote:
>>
>>> On 1/15/14 1:23 PM, Florian Pflug wrote:
>>>
 The fact that it's named plpgsql.warnings already clearly documents
 that this only affects plpgsql. But whether a particular warning is emitted
 during compilation or during execution it largely irrelevant, I think. For
 example, if we called this compiler_warning, we'd couldn't add a warning
 which triggers when SELECT .. INTO ingores excessive rows.

>>>
>>> There is the fact that something being a "compiler warning" gives you an
>>> idea on its effects on performance.  But maybe that would be better
>>> described in the documentation (perhaps even more accurately).
>>>
>>> I like the idea of warning about SELECT .. INTO, though, but that one
>>> could have a non-negligible performance penalty during execution.
>>>
>>
>> I'm not overly concerned about that. I image people would usually enable
>> warnings during development, not production.
>>
>
> Yeah, me neither, it's just something that needs to be communicated very
> clearly.  So probably just a list  plpgsql.warnings  would be the most
> appropriate then.
>

I am thinking so the name is not good. Changing handling warnings is messy
- minimally in Postgres, where warnings and errors are different creatures.

what about

plpgsql.enhanced_checks = (none | warning | error)





>
>
> Regards,
> Marko Tiikkaja
>

Re: [HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/15/14 2:00 PM, Florian Pflug wrote:

On Jan15, 2014, at 13:32 , Marko Tiikkaja  wrote:

On 1/15/14 1:23 PM, Florian Pflug wrote:

The fact that it's named plpgsql.warnings already clearly documents that this 
only affects plpgsql. But whether a particular warning is emitted during 
compilation or during execution it largely irrelevant, I think. For example, if 
we called this compiler_warning, we'd couldn't add a warning which triggers 
when SELECT .. INTO ingores excessive rows.


There is the fact that something being a "compiler warning" gives you an idea 
on its effects on performance.  But maybe that would be better described in the 
documentation (perhaps even more accurately).

I like the idea of warning about SELECT .. INTO, though, but that one could 
have a non-negligible performance penalty during execution.


I'm not overly concerned about that. I image people would usually enable 
warnings during development, not production.


Yeah, me neither, it's just something that needs to be communicated very 
clearly.  So probably just a list  plpgsql.warnings  would be the most 
appropriate then.



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] plpgsql.warn_shadow

[Florian Pflug]

On Jan15, 2014, at 13:32 , Marko Tiikkaja  wrote:
> On 1/15/14 1:23 PM, Florian Pflug wrote:
>> The fact that it's named plpgsql.warnings already clearly documents that 
>> this only affects plpgsql. But whether a particular warning is emitted 
>> during compilation or during execution it largely irrelevant, I think. For 
>> example, if we called this compiler_warning, we'd couldn't add a warning 
>> which triggers when SELECT .. INTO ingores excessive rows.
> 
> There is the fact that something being a "compiler warning" gives you an idea 
> on its effects on performance.  But maybe that would be better described in 
> the documentation (perhaps even more accurately).
> 
> I like the idea of warning about SELECT .. INTO, though, but that one could 
> have a non-negligible performance penalty during execution.

I'm not overly concerned about that. I image people would usually enable 
warnings during development, not production.

best regards,
Florian Pflug



-- 
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] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/15/14 1:23 PM, Florian Pflug wrote:

The fact that it's named plpgsql.warnings already clearly documents that this 
only affects plpgsql. But whether a particular warning is emitted during 
compilation or during execution it largely irrelevant, I think. For example, if 
we called this compiler_warning, we'd couldn't add a warning which triggers 
when SELECT .. INTO ingores excessive rows.


There is the fact that something being a "compiler warning" gives you an 
idea on its effects on performance.  But maybe that would be better 
described in the documentation (perhaps even more accurately).


I like the idea of warning about SELECT .. INTO, though, but that one 
could have a non-negligible performance penalty during execution.



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] plpgsql.warn_shadow

[Florian Pflug]

On Jan15, 2014, at 13:08 , Pavel Stehule  wrote:
> 2014/1/15 Florian Pflug 
>> On Jan15, 2014, at 11:20 , Pavel Stehule  wrote:
>> > 2014/1/15 Marko Tiikkaja 
>> >   plpgsql.warnings = 'all' # enable all warnings, defauls to the empty 
>> > list, i.e. no warnings
>> >   plpgsql.warnings = 'shadow, unused' # enable just "shadow" and "unused" 
>> > warnings
>> >   plpgsql.warnings_as_errors = on # defaults to off?
>> >
>> > This interface is a lot more flexible and should address Jim's concerns as 
>> > well.
>> >
>> > In this context is not clean if this option is related to plpgsql compile 
>> > warnings, plpgsql executor warnings or general warnings.
>> >
>> > plpgsql.compile_warnings = "disabled", "enabled", "fatal"
>> 
>> This makes no sense to me - warnings can just as well be emitted during 
>> execution. Why would we distinguish the two? What would that accomplish?
> 
> When we talked about plpgsql compiler warnings, we talked about relative 
> important warnings that means in almost all cases some design issue and is 
> better to stop early.
> 
> Postgres warnings is absolutly different kind - usually has informative 
> character - and usually you don't would to increment severity.
> 
> More we talking about warnings produced by plpgsql environment - and what I 
> know - it has sense only for compiler.

The fact that it's named plpgsql.warnings already clearly documents that this 
only affects plpgsql. But whether a particular warning is emitted during 
compilation or during execution it largely irrelevant, I think. For example, if 
we called this compiler_warning, we'd couldn't add a warning which triggers 
when SELECT .. INTO ingores excessive rows.

best regards,
Florian Pflug



-- 
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] plpgsql.warn_shadow

[Pavel Stehule]

2014/1/15 Florian Pflug 

> On Jan15, 2014, at 11:20 , Pavel Stehule  wrote:
> > 2014/1/15 Marko Tiikkaja 
> > On 1/15/14 7:07 AM, Florian Pflug wrote:
> > On Jan15, 2014, at 01:34 , Marko Tiikkaja  wrote:
> > It's me again, trying to find a solution to the most common mistakes I
> make.  This time it's accidental shadowing of variables, especially input
> variables.  I've wasted several hours banging my head against the wall
> while shouting "HOW CAN THIS VARIABLE ALWAYS BE NULL?".  I can't believe
> I'm the only one.  To give you a rough idea on how it works:
> >
> > I like this, but think that the option should be just called
> plpgsql.warnings or plpgsql.warn_on and accept a list of warnings to enable.
> >
> > Hmm.  How about:
> >
> >   plpgsql.warnings = 'all' # enable all warnings, defauls to the empty
> list, i.e. no warnings
> >   plpgsql.warnings = 'shadow, unused' # enable just "shadow" and
> "unused" warnings
> >   plpgsql.warnings_as_errors = on # defaults to off?
> >
> > This interface is a lot more flexible and should address Jim's concerns
> as well.
> >
> > In this context is not clean if this option is related to plpgsql
> compile warnings, plpgsql executor warnings or general warnings.
> >
> > plpgsql.compile_warnings = "disabled", "enabled", "fatal"
>
> This makes no sense to me - warnings can just as well be emitted during
> execution. Why would we distinguish the two? What would that accomplish?
>

When we talked about plpgsql compiler warnings, we talked about relative
important warnings that means in almost all cases some design issue and is
better to stop early.

Postgres warnings is absolutly different kind - usually has informative
character - and usually you don't would to increment severity.

More we talking about warnings produced by plpgsql environment - and what I
know - it has sense only for compiler.

Regards

Pavel

P.S. lot of functionality can be coveraged by plpgsql_check_function. It is
pity so we have not it in upstream.


>
> best regards,
> Florian Pflug
>
>

Re: [HACKERS] plpgsql.warn_shadow

[Florian Pflug]

On Jan15, 2014, at 11:20 , Pavel Stehule  wrote:
> 2014/1/15 Marko Tiikkaja 
> On 1/15/14 7:07 AM, Florian Pflug wrote:
> On Jan15, 2014, at 01:34 , Marko Tiikkaja  wrote:
> It's me again, trying to find a solution to the most common mistakes I make.  
> This time it's accidental shadowing of variables, especially input variables. 
>  I've wasted several hours banging my head against the wall while shouting 
> "HOW CAN THIS VARIABLE ALWAYS BE NULL?".  I can't believe I'm the only one.  
> To give you a rough idea on how it works:
> 
> I like this, but think that the option should be just called plpgsql.warnings 
> or plpgsql.warn_on and accept a list of warnings to enable.
> 
> Hmm.  How about:
> 
>   plpgsql.warnings = 'all' # enable all warnings, defauls to the empty list, 
> i.e. no warnings
>   plpgsql.warnings = 'shadow, unused' # enable just "shadow" and "unused" 
> warnings
>   plpgsql.warnings_as_errors = on # defaults to off?
> 
> This interface is a lot more flexible and should address Jim's concerns as 
> well.
> 
> In this context is not clean if this option is related to plpgsql compile 
> warnings, plpgsql executor warnings or general warnings.
> 
> plpgsql.compile_warnings = "disabled", "enabled", "fatal"

This makes no sense to me - warnings can just as well be emitted during 
execution. Why would we distinguish the two? What would that accomplish?

best regards,
Florian Pflug



-- 
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] plpgsql.warn_shadow

[Florian Pflug]

On Jan15, 2014, at 10:08 , Marko Tiikkaja  wrote:
> On 1/15/14 7:07 AM, Florian Pflug wrote:
>> On Jan15, 2014, at 01:34 , Marko Tiikkaja  wrote:
>>> It's me again, trying to find a solution to the most common mistakes I 
>>> make.  This time it's accidental shadowing of variables, especially input 
>>> variables.  I've wasted several hours banging my head against the wall 
>>> while shouting "HOW CAN THIS VARIABLE ALWAYS BE NULL?".  I can't believe 
>>> I'm the only one.  To give you a rough idea on how it works:
>> 
>> I like this, but think that the option should be just called 
>> plpgsql.warnings or plpgsql.warn_on and accept a list of warnings to enable.
> 
> Hmm.  How about:
> 
>  plpgsql.warnings = 'all' # enable all warnings, defauls to the empty list, 
> i.e. no warnings
>  plpgsql.warnings = 'shadow, unused' # enable just "shadow" and "unused" 
> warnings

Looks good. For the #-directive, I think what we'd actually want there is to 
*disable* certain warnings for certain functions, i.e. "#silence_warning 
shadow" would disable the shadow warning. Enabling on a per-function basis 
doesn't seem all that useful - usually you'd develop with all warnings globally 
enabled anyway.

>  plpgsql.warnings_as_errors = on # defaults to off?

This I object to, for the same reasons I object to consistent_into. 

best regards,
Florian Pflug



-- 
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] plpgsql.warn_shadow

[Pavel Stehule]

2014/1/15 Marko Tiikkaja 

> On 1/15/14 11:33 AM, Pavel Stehule wrote:
>
>> 2014/1/15 Marko Tiikkaja 
>>
>>  I agree, it's better to include the word "compiler" in the GUC name. But
>>> do we really need WARNING, ERROR and FATAL levels though?  Would WARNING
>>> and ERROR not be enough?
>>>
>>>
>> I am not strong in level names - and it is my subjective opinion only (as
>> not native speaker)
>>
>> just
>>
>> plpgsql.compile_warning=warning
>>
>> or
>>
>> plpgsql.compile_warning=error
>>
>> looks little bit obscure (or as contradiction). More - "fatal" is used by
>> gcc and some compilers as "stop on first error"
>>
>
> I was talking about postgres error levels above.  If we define "fatal" to
> mean ERROR here, I'm quite certain that will confuse people.  How's:
>
>   plpgsql.compiler_warning_severity = 'error' # disable, warning, error
> matching PG error severity levels ("disable" disables, obviously)
>

I don't think it is correct - "warning" is "severity" - it is about
handling of warnings. It is little bit fuzzy, and I have no good idea now :(


>   plpgsql.compiler_warnings = 'list, of, warnings'
>

is not it useless? I don't think it is generally usable. Now plpgsql
compiler doesn't raise any warning and better to raise warnings only when
the warning can be really important.

Regards

Pavel


>
>
> Regards,
> Marko Tiikkaja
>

Re: [HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/15/14 11:33 AM, Pavel Stehule wrote:

2014/1/15 Marko Tiikkaja 


I agree, it's better to include the word "compiler" in the GUC name. But
do we really need WARNING, ERROR and FATAL levels though?  Would WARNING
and ERROR not be enough?



I am not strong in level names - and it is my subjective opinion only (as
not native speaker)

just

plpgsql.compile_warning=warning

or

plpgsql.compile_warning=error

looks little bit obscure (or as contradiction). More - "fatal" is used by
gcc and some compilers as "stop on first error"


I was talking about postgres error levels above.  If we define "fatal" 
to mean ERROR here, I'm quite certain that will confuse people.  How's:


  plpgsql.compiler_warning_severity = 'error' # disable, warning, error 
matching PG error severity levels ("disable" disables, obviously)

  plpgsql.compiler_warnings = 'list, of, warnings'


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] plpgsql.warn_shadow

[Pavel Stehule]

2014/1/15 Marko Tiikkaja 

> On 1/15/14 11:20 AM, Pavel Stehule wrote:
>
>  2014/1/15 Marko Tiikkaja 
>>
>>> Hmm.  How about:
>>>
>>>plpgsql.warnings = 'all' # enable all warnings, defauls to the empty
>>> list, i.e. no warnings
>>>plpgsql.warnings = 'shadow, unused' # enable just "shadow" and
>>> "unused"
>>> warnings
>>>plpgsql.warnings_as_errors = on # defaults to off?
>>>
>>> This interface is a lot more flexible and should address Jim's concerns
>>> as
>>> well.
>>>
>>>
>> In this context is not clean if this option is related to plpgsql compile
>> warnings, plpgsql executor warnings or general warnings.
>>
>> plpgsql.compile_warnings = "disabled", "enabled", "fatal"
>>
>
> I agree, it's better to include the word "compiler" in the GUC name. But
> do we really need WARNING, ERROR and FATAL levels though?  Would WARNING
> and ERROR not be enough?
>

I am not strong in level names - and it is my subjective opinion only (as
not native speaker)

just

plpgsql.compile_warning=warning

or

plpgsql.compile_warning=error

looks little bit obscure (or as contradiction). More - "fatal" is used by
gcc and some compilers as "stop on first error"

Regards

Pavel


>
>
>
> Regards,
> Marko Tiikkaja
>

Re: [HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/15/14 11:20 AM, Pavel Stehule wrote:

2014/1/15 Marko Tiikkaja 

Hmm.  How about:

   plpgsql.warnings = 'all' # enable all warnings, defauls to the empty
list, i.e. no warnings
   plpgsql.warnings = 'shadow, unused' # enable just "shadow" and "unused"
warnings
   plpgsql.warnings_as_errors = on # defaults to off?

This interface is a lot more flexible and should address Jim's concerns as
well.



In this context is not clean if this option is related to plpgsql compile
warnings, plpgsql executor warnings or general warnings.

plpgsql.compile_warnings = "disabled", "enabled", "fatal"


I agree, it's better to include the word "compiler" in the GUC name. 
But do we really need WARNING, ERROR and FATAL levels though?  Would 
WARNING and ERROR not be enough?




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] plpgsql.warn_shadow

[Pavel Stehule]

2014/1/15 Marko Tiikkaja 

> On 1/15/14 7:07 AM, Florian Pflug wrote:
>
>> On Jan15, 2014, at 01:34 , Marko Tiikkaja  wrote:
>>
>>> It's me again, trying to find a solution to the most common mistakes I
>>> make.  This time it's accidental shadowing of variables, especially input
>>> variables.  I've wasted several hours banging my head against the wall
>>> while shouting "HOW CAN THIS VARIABLE ALWAYS BE NULL?".  I can't believe
>>> I'm the only one.  To give you a rough idea on how it works:
>>>
>>
>> I like this, but think that the option should be just called
>> plpgsql.warnings or plpgsql.warn_on and accept a list of warnings to enable.
>>
>
> Hmm.  How about:
>
>   plpgsql.warnings = 'all' # enable all warnings, defauls to the empty
> list, i.e. no warnings
>   plpgsql.warnings = 'shadow, unused' # enable just "shadow" and "unused"
> warnings
>   plpgsql.warnings_as_errors = on # defaults to off?
>
> This interface is a lot more flexible and should address Jim's concerns as
> well.
>

In this context is not clean if this option is related to plpgsql compile
warnings, plpgsql executor warnings or general warnings.

plpgsql.compile_warnings = "disabled", "enabled", "fatal"

Regards

Pavel


>
>
> 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] plpgsql.warn_shadow

[Marko Tiikkaja]

On 1/15/14 7:07 AM, Florian Pflug wrote:

On Jan15, 2014, at 01:34 , Marko Tiikkaja  wrote:

It's me again, trying to find a solution to the most common mistakes I make.  This time 
it's accidental shadowing of variables, especially input variables.  I've wasted several 
hours banging my head against the wall while shouting "HOW CAN THIS VARIABLE ALWAYS 
BE NULL?".  I can't believe I'm the only one.  To give you a rough idea on how it 
works:


I like this, but think that the option should be just called plpgsql.warnings 
or plpgsql.warn_on and accept a list of warnings to enable.


Hmm.  How about:

  plpgsql.warnings = 'all' # enable all warnings, defauls to the empty 
list, i.e. no warnings
  plpgsql.warnings = 'shadow, unused' # enable just "shadow" and 
"unused" warnings

  plpgsql.warnings_as_errors = on # defaults to off?

This interface is a lot more flexible and should address Jim's concerns 
as well.



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] plpgsql.warn_shadow

[Florian Pflug]

On Jan15, 2014, at 01:34 , Marko Tiikkaja  wrote:
> It's me again, trying to find a solution to the most common mistakes I make.  
> This time it's accidental shadowing of variables, especially input variables. 
>  I've wasted several hours banging my head against the wall while shouting 
> "HOW CAN THIS VARIABLE ALWAYS BE NULL?".  I can't believe I'm the only one.  
> To give you a rough idea on how it works:

I like this, but think that the option should be just called plpgsql.warnings 
or plpgsql.warn_on and accept a list of warnings to enable. I'm sure you'll 
come up with more unsafe coding practices to warn about in the future - for 
example, consistent_into immediately comes to mind ;-)

best regards,
Florian Pflug



-- 
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] plpgsql.warn_shadow

[Jim Nasby]

On 1/14/14, 6:34 PM, Marko Tiikkaja wrote:

Hi all!

It's me again, trying to find a solution to the most common mistakes I make.  This time 
it's accidental shadowing of variables, especially input variables.  I've wasted several 
hours banging my head against the wall while shouting "HOW CAN THIS VARIABLE ALWAYS 
BE NULL?".  I can't believe I'm the only one.  To give you a rough idea on how it 
works:

=# set plpgsql.warn_shadow to true;





No behaviour change by default.  Even setting the GUC doesn't really change 
behaviour, just emits some warnings when a potentially faulty function is 
compiled.


I like it, though I'm not sure I'm a fan of WARNING by default... perhaps 
plpgsql.warn_shadow_level that accepts a log level to use? That way you could 
even disallow this pattern completely if you wanted (plpgsql.warn_shadow_level 
= ERROR).

FWIW, this is just one kind of programming pattern I'd like to enforce (and not 
just within plpgsql)... I wish there was a way to plug into the parser. I also 
wish there was a good way to enforce SQL formatting...
--
Jim C. Nasby, Data Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] plpgsql.warn_shadow

[Marko Tiikkaja]

Hi all!

It's me again, trying to find a solution to the most common mistakes I 
make.  This time it's accidental shadowing of variables, especially 
input variables.  I've wasted several hours banging my head against the 
wall while shouting "HOW CAN THIS VARIABLE ALWAYS BE NULL?".  I can't 
believe I'm the only one.  To give you a rough idea on how it works:


=# set plpgsql.warn_shadow to true;
SET
=#* create function test(in1 int, in2 int)
-#* returns table(out1 int, out2 int) as $$
$#* declare
$#* IN1 text;
$#* IN2 text;
$#* out1 text;
$#* begin
$#*
$#* declare
$#* out2 text;
$#* in1 text;
$#* begin
$#* end;
$#* end$$ language plpgsql;
WARNING:  variable "in1" shadows a previously defined variable
LINE 4: IN1 text;
^
WARNING:  variable "in2" shadows a previously defined variable
LINE 5: IN2 text;
^
WARNING:  variable "out1" shadows a previously defined variable
LINE 6: out1 text;
^
WARNING:  variable "out2" shadows a previously defined variable
LINE 10: out2 text;
 ^
WARNING:  variable "in1" shadows a previously defined variable
LINE 11: in1 text;
 ^
CREATE FUNCTION


No behaviour change by default.  Even setting the GUC doesn't really 
change behaviour, just emits some warnings when a potentially faulty 
function is compiled.


Let me know what you think so I can either cry or clean the patch up.


Regards,
Marko Tiikkaja
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
***
*** 352,357  do_compile(FunctionCallInfo fcinfo,
--- 352,358 
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->warn_shadow = plpgsql_warn_shadow;
  
if (is_dml_trigger)
function->fn_is_trigger = PLPGSQL_DML_TRIGGER;
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
***
*** 335,340  static List*read_raise_options(void);
--- 335,341 
  %token   K_USE_VARIABLE
  %token   K_USING
  %token   K_VARIABLE_CONFLICT
+ %token   K_WARN_SHADOW
  %token   K_WARNING
  %token   K_WHEN
  %token   K_WHILE
***
*** 364,369  comp_option: '#' K_OPTION K_DUMP
--- 365,379 
else
elog(ERROR, 
"unrecognized print_strict_params option %s", $3);
}
+   | '#' K_WARN_SHADOW option_value
+   {
+   if (strcmp($3, "on") == 0)
+   
plpgsql_curr_compile->warn_shadow = true;
+   else if (strcmp($3, "off") == 0)
+   
plpgsql_curr_compile->warn_shadow = false;
+   else
+   elog(ERROR, 
"unrecognized warn_shadow option %s", $3);
+   }
| '#' K_VARIABLE_CONFLICT K_ERROR
{

plpgsql_curr_compile->resolve_option = PLPGSQL_RESOLVE_ERROR;
***
*** 727,732  decl_varname   : T_WORD
--- 737,754 

  $1.ident, NULL, NULL,

  NULL) != NULL)
yyerror("duplicate 
declaration");
+   if 
(plpgsql_curr_compile->warn_shadow)
+   {
+   PLpgSQL_nsitem *nsi;
+   nsi = 
plpgsql_ns_lookup(plpgsql_ns_top(), false,
+   
$1.ident, NULL, NULL, NULL);
+   if (nsi != NULL)
+   ereport(WARNING,
+   
(errcode(ERRCODE_DUPLICATE_ALIAS),
+   
 errmsg("variable \"%s\" shadows a previously defined variable",
+   
$1.ident),
+   
 parser_errposition(@1)));
+   }