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

<snip>

> 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 jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko(at)joh(dot)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 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

On 1/15/14 7:07 AM, Florian Pflug wrote:
> On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko(at)joh(dot)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 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

2014/1/15 Marko Tiikkaja <marko(at)joh(dot)to>

> On 1/15/14 7:07 AM, Florian Pflug wrote:
>
>> On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko(at)joh(dot)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 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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On 1/15/14 11:20 AM, Pavel Stehule wrote:
> 2014/1/15 Marko Tiikkaja <marko(at)joh(dot)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

2014/1/15 Marko Tiikkaja <marko(at)joh(dot)to>

> On 1/15/14 11:20 AM, Pavel Stehule wrote:
>
> 2014/1/15 Marko Tiikkaja <marko(at)joh(dot)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?
>

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
>

On 1/15/14 11:33 AM, Pavel Stehule wrote:
> 2014/1/15 Marko Tiikkaja <marko(at)joh(dot)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

2014/1/15 Marko Tiikkaja <marko(at)joh(dot)to>

> On 1/15/14 11:33 AM, Pavel Stehule wrote:
>
>> 2014/1/15 Marko Tiikkaja <marko(at)joh(dot)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)
>

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
>

On Jan15, 2014, at 10:08 , Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> On 1/15/14 7:07 AM, Florian Pflug wrote:
>> On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko(at)joh(dot)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 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

On Jan15, 2014, at 11:20 , Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2014/1/15 Marko Tiikkaja <marko(at)joh(dot)to>
> On 1/15/14 7:07 AM, Florian Pflug wrote:
> On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko(at)joh(dot)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 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

2014/1/15 Florian Pflug <fgp(at)phlo(dot)org>

> On Jan15, 2014, at 11:20 , Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> > 2014/1/15 Marko Tiikkaja <marko(at)joh(dot)to>
> > On 1/15/14 7:07 AM, Florian Pflug wrote:
> > On Jan15, 2014, at 01:34 , Marko Tiikkaja <marko(at)joh(dot)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 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
>
>

On Jan15, 2014, at 13:08 , Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2014/1/15 Florian Pflug <fgp(at)phlo(dot)org>
>> On Jan15, 2014, at 11:20 , Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> > 2014/1/15 Marko Tiikkaja <marko(at)joh(dot)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 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

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

On Jan15, 2014, at 13:32 , Marko Tiikkaja <marko(at)joh(dot)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 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

On 1/15/14 2:00 PM, Florian Pflug wrote:
> On Jan15, 2014, at 13:32 , Marko Tiikkaja <marko(at)joh(dot)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 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

2014/1/15 Marko Tiikkaja <marko(at)joh(dot)to>

> On 1/15/14 2:00 PM, Florian Pflug wrote:
>
>> On Jan15, 2014, at 13:32 , Marko Tiikkaja <marko(at)joh(dot)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 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
>

On 1/15/14 2:27 PM, Pavel Stehule wrote:
> 2014/1/15 Marko Tiikkaja <marko(at)joh(dot)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

2014/1/15 Marko Tiikkaja <marko(at)joh(dot)to>

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

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
>

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

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 <marko(at)joh(dot)to>

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

2014-01-26 Florian Pflug <fgp(at)phlo(dot)org>

> On Jan26, 2014, at 10:19 , Simon Riggs <simon(at)2ndQuadrant(dot)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

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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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

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

Regards

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

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

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

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

2014-01-27 Marti Raudsepp <marti(at)juffo(dot)org>

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

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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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

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 <marko(at)joh(dot)to>:

> 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

Dne 3.2.2014 21:48 "Marko Tiikkaja" <marko(at)joh(dot)to> 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

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(at)joh(dot)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(at)trustly(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)trustly(dot)com> writes:
> On Tue, Mar 4, 2014 at 12:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Joel Jacobson <joel(at)trustly(dot)com> writes:
>> On Tue, Mar 4, 2014 at 12:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(dot)stehule(at)gmail(dot)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

2014-03-14 12:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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.

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

+1

Pavel

>
>
> Regards,
> Marko Tiikkaja
>

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

2014-03-14 13:12 GMT+01:00 Simon Riggs <simon(at)2ndquadrant(dot)com>:

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

I understand .

+1

Pavel

>
> --
> 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(dot)stehule(at)gmail(dot)com> wrote:
>>
>>
>> 2014-03-14 12:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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

2014-03-18 13:23 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:

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

extra* sounds better

Pavel

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

On 18 March 2014 12:23, Petr Jelinek <petr(at)2ndquadrant(dot)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

On 18/03/14 13:43, Pavel Stehule wrote:
> 2014-03-18 13:23 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com
> <mailto:petr(at)2ndquadrant(dot)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

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

2014-03-18 19:56 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:

>
> On 18/03/14 13:43, Pavel Stehule wrote:
>
> 2014-03-18 13:23 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)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
>
>
> 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
>
>

On 18 March 2014 19:04, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)2ndquadrant(dot)com>:

> On 18 March 2014 19:04, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

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

On 18/03/14 20:15, Pavel Stehule wrote:
>
> 2014-03-18 20:14 GMT+01:00 Simon Riggs <simon(at)2ndquadrant(dot)com
> <mailto:simon(at)2ndquadrant(dot)com>>:
>
> On 18 March 2014 19:04, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com
> <mailto:pavel(dot)stehule(at)gmail(dot)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

Hello

Tomorrow I'll do a review

fast check

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

Regards

Pavel

2014-03-18 20:29 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:

>
> On 18/03/14 20:15, Pavel Stehule wrote:
>
>
> 2014-03-18 20:14 GMT+01:00 Simon Riggs <simon(at)2ndquadrant(dot)com>:
>
>> On 18 March 2014 19:04, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>
>

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(at)2ndquadrant(dot)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?

Pavel

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

On 18/03/14 20:45, Pavel Stehule wrote:
>
>
> 2014-03-18 20:44 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com
> <mailto:petr(at)2ndquadrant(dot)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?

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

+ 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

2014-03-18 20:49 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:

>
> On 18/03/14 20:45, Pavel Stehule wrote:
>
>
>
> 2014-03-18 20:44 GMT+01:00 Petr Jelinek <petr(at)2ndquadrant(dot)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?
>
>
> There is something like that said 2 paragraphs down the line:
>
> + The default is <literal>"none"</>. Currently the list of available checks
> + includes only one:
> + <variablelist>
> + <varlistentry>
> + <term><varname>shadowed-variables</varname></term>
>
>
ok

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

On Tue, Mar 18, 2014 at 9:29 PM, Petr Jelinek <petr(at)2ndquadrant(dot)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(at)juffo(dot)org> wrote:
> On Tue, Mar 18, 2014 at 9:29 PM, Petr Jelinek <petr(at)2ndquadrant(dot)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