Re: plpgsql.warn_shadow

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