Обсуждение: plpgsql.warn_shadow

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

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

<snip>

> No behaviour change by default.  Even setting the GUC doesn't really change behaviour, just emits some warnings when
apotentially 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
levelto 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
wasa way to plug into the parser. I also wish there was a good way to enforce SQL formatting...
 
-- 
Jim C. Nasby, Data Architect                       jim@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net

On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko@joh.to> 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
CANTHIS 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
warningsto enable. I'm sure you'll come up with more unsafe coding practices to warn about in the future - for example,
consistent_intoimmediately comes to mind ;-) 

best regards,
Florian Pflug

On 1/15/14 7:07 AM, Florian Pflug wrote:
> On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko@joh.to> 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
CANTHIS 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
warningsto 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

On 1/15/14 11:20 AM, Pavel Stehule wrote:
> 2014/1/15 Marko Tiikkaja <marko@joh.to>
>> 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

On 1/15/14 11:33 AM, Pavel Stehule wrote:
> 2014/1/15 Marko Tiikkaja <marko@joh.to>
>
>> 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

On Jan15, 2014, at 10:08 , Marko Tiikkaja <marko@joh.to> wrote:
> On 1/15/14 7:07 AM, Florian Pflug wrote:
>> On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko@joh.to> wrote:
>>> It's me again, trying to find a solution to the most common mistakes I make.  This time it's accidental shadowing
ofvariables, especially input variables.  I've wasted several hours banging my head against the wall while shouting
"HOWCAN 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
ofwarnings 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
seemall 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

On Jan15, 2014, at 11:20 , Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2014/1/15 Marko Tiikkaja <marko@joh.to>
> On 1/15/14 7:07 AM, Florian Pflug wrote:
> On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko@joh.to> 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
CANTHIS 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
warningsto 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
generalwarnings. 
>
> 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?
Whatwould that accomplish? 

best regards,
Florian Pflug

On Jan15, 2014, at 13:08 , Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2014/1/15 Florian Pflug <fgp@phlo.org>
>> On Jan15, 2014, at 11:20 , Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> > 2014/1/15 Marko Tiikkaja <marko@joh.to>
>> >   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
generalwarnings. 
>> >
>> > 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?
Whatwould that accomplish? 
>
> When we talked about plpgsql compiler warnings, we talked about relative important warnings that means in almost all
casessome 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
incrementseverity. 
>
> 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
particularwarning is emitted during compilation or during execution it largely irrelevant, I think. For example, if we
calledthis compiler_warning, we'd couldn't add a warning which triggers when SELECT .. INTO ingores excessive rows. 

best regards,
Florian Pflug

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
particularwarning is emitted during compilation or during execution it largely irrelevant, I think. For example, if we
calledthis 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

On Jan15, 2014, at 13:32 , Marko Tiikkaja <marko@joh.to> 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
particularwarning is emitted during compilation or during execution it largely irrelevant, I think. For example, if we
calledthis 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
maybethat 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
duringexecution. 

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

best regards,
Florian Pflug

On 1/15/14 2:00 PM, Florian Pflug wrote:
> On Jan15, 2014, at 13:32 , Marko Tiikkaja <marko@joh.to> 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
particularwarning is emitted during compilation or during execution it largely irrelevant, I think. For example, if we
calledthis 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
maybethat 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
penaltyduring 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

On 1/15/14 2:27 PM, Pavel Stehule wrote:
> 2014/1/15 Marko Tiikkaja <marko@joh.to>
>> 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

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

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

On Fri, Jan 17, 2014 at 5:45 AM, Marko Tiikkaja <marko@joh.to> 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

On 1/20/14 2:25 AM, Robert Haas wrote:
> On Fri, Jan 17, 2014 at 5:45 AM, Marko Tiikkaja <marko@joh.to> 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

On Mon, Jan 20, 2014 at 7:16 AM, Marko Tiikkaja <marko@joh.to> 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

On 1/20/14 1:55 PM, Robert Haas wrote:
> On Mon, Jan 20, 2014 at 7:16 AM, Marko Tiikkaja <marko@joh.to> 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

On Jan20, 2014, at 14:05 , Marko Tiikkaja <marko@joh.to> wrote:
> On 1/20/14 1:55 PM, Robert Haas wrote:
>> On Mon, Jan 20, 2014 at 7:16 AM, Marko Tiikkaja <marko@joh.to> 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

On 15 January 2014 00:34, Marko Tiikkaja <marko@joh.to> 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

On Jan26, 2014, at 10:19 , Simon Riggs <simon@2ndQuadrant.com> 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

On 26 January 2014 15:53, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> 2014-01-26 Florian Pflug <fgp@phlo.org>
>
>> On Jan26, 2014, at 10:19 , Simon Riggs <simon@2ndQuadrant.com> 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

<p dir="ltr"><br /> Dne 26. 1. 2014 23:24 "Simon Riggs" <<a
href="mailto:simon@2ndquadrant.com">simon@2ndquadrant.com</a>>napsal(a):<br /> ><br /> > On 26 January 2014
15:53,Pavel Stehule <<a href="mailto:pavel.stehule@gmail.com">pavel.stehule@gmail.com</a>> wrote:<br /> >
><br/> > ><br /> > ><br /> > > 2014-01-26 Florian Pflug <<a
href="mailto:fgp@phlo.org">fgp@phlo.org</a>><br/> > ><br /> > >> On Jan26, 2014, at 10:19 , Simon
Riggs<simon@2ndQuadrant.com> wrote:<br /> > >> > Also, having<br /> > >> >
 plpgsql.warnings_as_errors= off (default) | on<br /> > >> > makes sense and should be included in 9.4<br
/>> >><br /> > >> I still think this is a bad idea, for the same reasons I don't like<br /> >
>>consistent_into (discussed in a separate thread).<br /> > >><br /> > >> But these objections
wouldgo away if restricted this to function<br /> > >> creation time only. So even with warnings_as_errors=on,
you<br/> > >> could still *call* a function that produces a warning, but not<br /> > >> *create*
one.<br/> > ><br /> > ><br /> > > +1 behave - and please, better name<br /> ><br /> > +1 to
that.<br/> ><br /> > I guess I only saw that way of working because I was thinking of it as<br /> > a
"compilerwarning".<br /> ><br /> > So perhaps we should call it<br /> >  
plpgsql.compiler_warnings_as_errors<br/> ><br /> > to make that behaviour more clear.<br /> ><br /> >  
plpgsql.error_on_create_warnings<pdir="ltr">I have a problém with joining word error and warning together.<p
dir="ltr">Somelike stop_on_compilation_warning or strict_compiler sound me more logical <p dir="ltr">Regards<p
dir="ltr">Pavel<br/> ><br /> > --<br /> >  Simon Riggs                   <a
href="http://www.2ndQuadrant.com/">http://www.2ndQuadrant.com/</a><br/> >  PostgreSQL Development, 24x7 Support,
Training& Services<br /> 

On 26 January 2014 22:44, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Dne 26. 1. 2014 23:24 "Simon Riggs" <simon@2ndquadrant.com> napsal(a):
>
>
>>
>> On 26 January 2014 15:53, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> >
>> >
>> >
>> > 2014-01-26 Florian Pflug <fgp@phlo.org>
>> >
>> >> On Jan26, 2014, at 10:19 , Simon Riggs <simon@2ndQuadrant.com> 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

On Sun, Jan 26, 2014 at 11:19 AM, Simon Riggs <simon@2ndquadrant.com> 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

On 27 January 2014 10:40, Marti Raudsepp <marti@juffo.org> wrote:
> On Sun, Jan 26, 2014 at 11:19 AM, Simon Riggs <simon@2ndquadrant.com> 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

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

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

<p dir="ltr"><br /> Dne 3.2.2014 21:48 "Marko Tiikkaja" <<a href="mailto:marko@joh.to">marko@joh.to</a>>
napsal(a):<br/> ><br /> > On 2014-02-03 9:17 PM, Pavel Stehule wrote:<br /> >><br /> >> I am not
happyfrom "warnings_as_error"<br /> >><br /> >> what about "stop_on_warning" instead?<br /> ><br />
><br/> > abort_compilation_on_warning?  I think if we're not going to make it more clear that warnings are only
promotedto errors at CREATE FUNCTION time, we can't do much better than warnings_as_errors.<br /> ><br /> ><br />
>>second question: should be these errors catchable or uncatchable?<br /> >><br /> >> When I work on
largeproject, where I had to use some error handler of<br /> >> "EXCEPTION WHEN OTHERS" I found very strange and
notuseful so all syntax<br /> >> errors was catched by this handler. Any debugging was terribly difficult<br />
>>and I had to write plpgsql_lint as solution.<br /> ><br /> ><br /> > Is this really an issue
consideringthese errors can only happen when someone runs CREATE FUNCTION?<p dir="ltr">Can you look, pls, what
terminologyis used in gcc, clang, perl or python.<p dir="ltr">I agree with idea, but proposed names I have not
associatedwith something.<p dir="ltr">Regards<p dir="ltr">Pavel<br /> ><br /> ><br /> > Regards,<br /> >
MarkoTiikkaja<br /> 

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

On Wed, Jan 15, 2014 at 1:34 AM, Marko Tiikkaja <marko@joh.to> 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.

Joel Jacobson <joel@trustly.com> 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

On Tue, Mar 4, 2014 at 12:55 AM, Tom Lane <tgl@sss.pgh.pa.us> 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?

Joel Jacobson <joel@trustly.com> writes:
> On Tue, Mar 4, 2014 at 12:55 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Tue, Mar 4, 2014 at 4:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Joel Jacobson <joel@trustly.com> writes:
>> On Tue, Mar 4, 2014 at 12:55 AM, Tom Lane <tgl@sss.pgh.pa.us> 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.

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 <<fn>>), 
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

On Tue, Mar 4, 2014 at 8:04 PM, Andrew Dunstan <andrew@dunslane.net> 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.

On 03/04/2014 03:40 PM, Joel Jacobson wrote:
> On Tue, Mar 4, 2014 at 8:04 PM, Andrew Dunstan <andrew@dunslane.net> 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

On 3 February 2014 20:17, Pavel Stehule <pavel.stehule@gmail.com> 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

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

On 14 March 2014 11:10, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> 2014-03-14 12:02 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>
>> 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

On 14/03/14 13:12, Simon Riggs wrote:
> On 14 March 2014 11:10, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>
>>
>> 2014-03-14 12:02 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>>
>>> 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

On 18 March 2014 12:23, Petr Jelinek <petr@2ndquadrant.com> 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

2014-03-18 13:23 GMT+01:00 Petr Jelinek <petr@2ndquadrant.com>

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

extra* sounds better

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

On 18 March 2014 19:04, Pavel Stehule <pavel.stehule@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.

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

2014-03-18 20:14 GMT+01:00 Simon Riggs <simon@2ndquadrant.com>:

On 18 March 2014 19:04, Pavel Stehule <pavel.stehule@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

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

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

On 18/03/14 20:36, Pavel Stehule wrote:
>
>    <para>
> +    To aid the user in finding instances of simple but common 
> problems before
> +    they cause harm, <application>PL/PgSQL</> provides a number of 
> additional
> +    <replaceable>checks</>. When enabled, depending on the 
> configuration, they
> +    can be used to emit either a <literal>WARNING</> or an 
> <literal>ERROR</>
> +    during the compilation of a function.
> +   </para>
>
> >>>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

2014-03-18 20:44 GMT+01:00 Petr Jelinek <petr@2ndquadrant.com>:

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

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

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

+  The default is <literal>"none"</>. Currently the list of available checks
+  includes only one:
+  <variablelist>
+   <varlistentry>
+    <term><varname>shadowed-variables</varname></term>

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

On Tue, Mar 18, 2014 at 9:29 PM, Petr Jelinek <petr@2ndquadrant.com> 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

On 18 March 2014 20:52, Marti Raudsepp <marti@juffo.org> wrote:
> On Tue, Mar 18, 2014 at 9:29 PM, Petr Jelinek <petr@2ndquadrant.com> 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

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