plpgsql.warn_shadow

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

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

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 CAN THIS VARIABLE ALWAYS BE NULL?". I can't believe I'm the only one. To give you a rough idea on how it works:

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

best regards,
Florian Pflug

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

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 CAN THIS VARIABLE ALWAYS BE NULL?". I can't believe I'm the only one. To give you a rough idea on how it works:

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

Hmm. How about:

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

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

Regards,
Marko Tiikkaja

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

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

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

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

2014/1/15 Marko Tiikkaja <marko@joh.to>

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?

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

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

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

2014/1/15 Marko Tiikkaja <marko@joh.to>

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)

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

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 of variables, especially input variables. I've wasted several hours banging my head against the wall while shouting "HOW CAN THIS VARIABLE ALWAYS BE NULL?". I can't believe I'm the only one. To give you a rough idea on how it works:

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

Hmm. How about:

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

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

plpgsql.warnings_as_errors = on # defaults to off?

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

best regards,
Florian Pflug

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

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 CAN THIS VARIABLE ALWAYS BE NULL?". I can't believe I'm the only one. To give you a rough idea on how it works:

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

Hmm. How about:

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

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

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

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

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

best regards,
Florian Pflug

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

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

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

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

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

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

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

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

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

best regards,
Florian Pflug

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

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

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

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

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

Regards,
Marko Tiikkaja

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

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 particular warning is emitted during compilation or during execution it largely irrelevant, I think. For example, if we called this compiler_warning, we'd couldn't add a warning which triggers when SELECT .. INTO ingores excessive rows.

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

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

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

best regards,
Florian Pflug

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

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 particular warning is emitted during compilation or during execution it largely irrelevant, I think. For example, if we called this compiler_warning, we'd couldn't add a warning which triggers when SELECT .. INTO ingores excessive rows.

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

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

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

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

Regards,
Marko Tiikkaja

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

2014/1/15 Marko Tiikkaja <marko@joh.to>

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

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

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

2014/1/15 Marko Tiikkaja <marko@joh.to>

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.

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

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

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

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

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

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

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

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

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

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

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

Regards,
Marko Tiikkaja

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