Обсуждение: [PATCH] parser: optionally warn about missing AS for column and table aliases

I wrote the attached patch to optionally emit warnings when column or table
aliases are used without the AS keyword after errors caused by typos in
statements turning unintended things into aliases came up twice this week.
First in a discussion with a colleague who was surprised by a 1 row result
for the query 'SELECT COUNT(*) files' and again in the "pl/pgsql 2" thread
as plpgsql currently doesn't throw an error if there are more result columns
than output columns (SELECT a b INTO f1, f2).

The patch is still missing documentation and it needs another patch to
modify all the statements in psql & co to use AS so you can use things like
\d and tab-completion without triggering the warnings.  I can implement
those changes if others think this patch makes sense.

/ Oskari

On 2014-09-05 22:38, Oskari Saarenmaa wrote:
> I wrote the attached patch to optionally emit warnings when column or table
> aliases are used without the AS keyword after errors caused by typos in
> statements turning unintended things into aliases came up twice this week.
> First in a discussion with a colleague who was surprised by a 1 row result
> for the query 'SELECT COUNT(*) files' and again in the "pl/pgsql 2" thread
> as plpgsql currently doesn't throw an error if there are more result columns
> than output columns (SELECT a b INTO f1, f2).
>
> The patch is still missing documentation and it needs another patch to
> modify all the statements in psql & co to use AS so you can use things like
> \d and tab-completion without triggering the warnings.  I can implement
> those changes if others think this patch makes sense.

I think this is only problematic for column aliases.  I wouldn't want to 
put these two to be put into the same category, as I always omit the AS 
keyword for tables aliases (and will continue to do so), but never omit 
it for column aliases.


.marko

05.09.2014 23:45, Marko Tiikkaja kirjoitti:
> On 2014-09-05 22:38, Oskari Saarenmaa wrote:
>> I wrote the attached patch to optionally emit warnings when column or
>> table
>> aliases are used without the AS keyword after errors caused by typos in
>> statements turning unintended things into aliases came up twice this
>> week.
>> First in a discussion with a colleague who was surprised by a 1 row
>> result
>> for the query 'SELECT COUNT(*) files' and again in the "pl/pgsql 2"
>> thread
>> as plpgsql currently doesn't throw an error if there are more result
>> columns
>> than output columns (SELECT a b INTO f1, f2).
>>
>> The patch is still missing documentation and it needs another patch to
>> modify all the statements in psql & co to use AS so you can use things
>> like
>> \d and tab-completion without triggering the warnings.  I can implement
>> those changes if others think this patch makes sense.
>
> I think this is only problematic for column aliases.  I wouldn't want to
> put these two to be put into the same category, as I always omit the AS
> keyword for tables aliases (and will continue to do so), but never omit
> it for column aliases.

I prefer using AS for both, but I can see the case for requiring AS in 
table aliases being a lot weaker.  Not emitting warnings for table 
aliases would also reduce the changes required in psql & co as they seem 
to be using aliases mostly (only?) for tables.

What'd be a good name for the GUC controlling this, 
missing_column_as_warning?

/ Oskari

On 2014-09-05 22:56, Oskari Saarenmaa wrote:
> What'd be a good name for the GUC controlling this,
> missing_column_as_warning?

I don't know about others, but I've previously had the idea of having 
more warnings like this (my go-to example is  DELETE FROM foo WHERE bar 
IN (SELECT bar FROM barlesstable);  where "barlesstable" doesn't have a 
column "bar").  Perhaps it would be better to have a guc with a list of 
warnings, similarly to what we did for plpgsql.extra_warnings?

Now that I start thinking about it, more ideas for such warnings start 
springing up..


.marko

Marko Tiikkaja-4 wrote
> On 2014-09-05 22:38, Oskari Saarenmaa wrote:
>> I wrote the attached patch to optionally emit warnings when column or
>> table
>> aliases are used without the AS keyword after errors caused by typos in
>> statements turning unintended things into aliases came up twice this
>> week.
>> First in a discussion with a colleague who was surprised by a 1 row
>> result
>> for the query 'SELECT COUNT(*) files' and again in the "pl/pgsql 2"
>> thread
>> as plpgsql currently doesn't throw an error if there are more result
>> columns
>> than output columns (SELECT a b INTO f1, f2).
>>
>> The patch is still missing documentation and it needs another patch to
>> modify all the statements in psql & co to use AS so you can use things
>> like
>> \d and tab-completion without triggering the warnings.  I can implement
>> those changes if others think this patch makes sense.
> 
> I think this is only problematic for column aliases.  I wouldn't want to 
> put these two to be put into the same category, as I always omit the AS 
> keyword for tables aliases (and will continue to do so), but never omit 
> it for column aliases.

I agree on being able to pick the behavior independently for select-list
items and the FROM clause items.

Using this to mitigate the pl/pgsql column mis-match issue seems like too
broad of a solution.

I probably couldn't mount a convincing defense of my opinion but at first
blush I'd say we should pass on this.  Not with prejudice - looking at the
issue periodically has merit - but right now it seems to be mostly adding
clutter to the code without significant benefit.

Tangential - I'd rather see something like "EXPLAIN (STATIC)" that would
allow a user to quickly invoke a built-in static SQL analyzer on their query
and have it point this kind of thing out.  Adding a catalog for STATIC
configurations in much the same way we have text search configurations would
allow extensions and users to define their own rulesets that could be
attached to their ROLE "GUC: default_static_analyzer_ruleset" and also
passed in as a modifier in the EXPLAIN invocation.

Stuff like correlated subqueries without alias use could be part of that (to
avoid typos that result in constant true) and also duplicate column names
floating to the top-most select-list, or failure to use an alias on a
function call result.  There are probably others though I'm stretching to
even find these...

Anyway, the idea of using a GUC and evaluating every query that is written
(the added if statements), is not that appealing even if the impact of one
more check is likely to be insignificant (is it?)

David J.




--
View this message in context:
http://postgresql.1045698.n5.nabble.com/PATCH-parser-optionally-warn-about-missing-AS-for-column-and-table-aliases-tp5817971p5817980.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

David G Johnston wrote
> 
> Marko Tiikkaja-4 wrote
>> On 2014-09-05 22:38, Oskari Saarenmaa wrote:
>>> I wrote the attached patch to optionally emit warnings when column or
>>> table
>>> aliases are used without the AS keyword after errors caused by typos in
>>> statements turning unintended things into aliases came up twice this
>>> week.
>>> First in a discussion with a colleague who was surprised by a 1 row
>>> result
>>> for the query 'SELECT COUNT(*) files' and again in the "pl/pgsql 2"
>>> thread
>>> as plpgsql currently doesn't throw an error if there are more result
>>> columns
>>> than output columns (SELECT a b INTO f1, f2).
>>>
>>> The patch is still missing documentation and it needs another patch to
>>> modify all the statements in psql & co to use AS so you can use things
>>> like
>>> \d and tab-completion without triggering the warnings.  I can implement
>>> those changes if others think this patch makes sense.
>> 
>> I think this is only problematic for column aliases.  I wouldn't want to 
>> put these two to be put into the same category, as I always omit the AS 
>> keyword for tables aliases (and will continue to do so), but never omit 
>> it for column aliases.
> I agree on being able to pick the behavior independently for select-list
> items and the FROM clause items.
> 
> Using this to mitigate the pl/pgsql column mis-match issue seems like too
> broad of a solution.
> 
> I probably couldn't mount a convincing defense of my opinion but at first
> blush I'd say we should pass on this.  Not with prejudice - looking at the
> issue periodically has merit - but right now it seems to be mostly adding
> clutter to the code without significant benefit.
> 
> Tangential - I'd rather see something like "EXPLAIN (STATIC)" that would
> allow a user to quickly invoke a built-in static SQL analyzer on their
> query and have it point this kind of thing out.  Adding a catalog for
> STATIC configurations in much the same way we have text search
> configurations would allow extensions and users to define their own
> rulesets that could be attached to their ROLE "GUC:
> default_static_analyzer_ruleset" and also passed in as a modifier in the
> EXPLAIN invocation.
> 
> Stuff like correlated subqueries without alias use could be part of that
> (to avoid typos that result in constant true) and also duplicate column
> names floating to the top-most select-list, or failure to use an alias on
> a function call result.  There are probably others though I'm stretching
> to even find these...
> 
> Anyway, the idea of using a GUC and evaluating every query that is written
> (the added if statements), is not that appealing even if the impact of one
> more check is likely to be insignificant (is it?)

To protect users on every query they write there would need to be some kind
of "always explain first and only execute if no warnings are thrown"
mode...and ideally some way to then override that on a per-query basis if
you know you are correct and don't want to fix the SQL...

If the static check fails the query itself would error and the detail would
contain the status result of the static analysis; otherwise the query should
return as normal.

This at least avoids having to introduce 10 different GUC just to
accommodate this feature and neatly bundles them into named packages.  

A warn-and-execute mode would be possible as well.  I would make it
incompatible with "autocommit" mode so that a user doing this would have a
chance to evaluate the warnings and rollback even if the statement ran to
completion successfully.

David J.



--
View this message in context:
http://postgresql.1045698.n5.nabble.com/PATCH-parser-optionally-warn-about-missing-AS-for-column-and-table-aliases-tp5817971p5817982.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On Fri, Sep  5, 2014 at 02:33:00PM -0700, David G Johnston wrote:
> This at least avoids having to introduce 10 different GUC just to
> accommodate this feature and neatly bundles them into named packages.  
> 
> A warn-and-execute mode would be possible as well.  I would make it
> incompatible with "autocommit" mode so that a user doing this would have a
> chance to evaluate the warnings and rollback even if the statement ran to
> completion successfully.

Our TODO list had the idea of a "novice" mode that would warn about such
things.


--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On 2014-09-05 11:19 PM, David G Johnston wrote:
> Marko Tiikkaja-4 wrote
>> I think this is only problematic for column aliases.  I wouldn't want to
>> put these two to be put into the same category, as I always omit the AS
>> keyword for tables aliases (and will continue to do so), but never omit
>> it for column aliases.
>
> I agree on being able to pick the behavior independently for select-list
> items and the FROM clause items.
>
> Using this to mitigate the pl/pgsql column mis-match issue seems like too
> broad of a solution.

The PL/PgSQL column can be solved via other methods; see for example my 
suggestion of "PL/PgSQL warning - num_into_expressions" in the current 
commit fest (and the non-backwards-compatible solution of throwing a 
run-time error I suggested).  But some people run SQL queries from their 
apps, and I've seen people make this mistake and get confused.

> I probably couldn't mount a convincing defense of my opinion but at first
> blush I'd say we should pass on this.  Not with prejudice - looking at the
> issue periodically has merit - but right now it seems to be mostly adding
> clutter to the code without significant benefit.

Are you talking about this particular option, or the notion of parser 
warnings in general?  Because if we're going to add a handful of 
warnings, I don't see why this couldn't be on that list.

> Tangential - I'd rather see something like "EXPLAIN (STATIC)" that would
> allow a user to quickly invoke a built-in static SQL analyzer on their query
> and have it point this kind of thing out.  Adding a catalog for STATIC
> configurations in much the same way we have text search configurations would
> allow extensions and users to define their own rulesets that could be
> attached to their ROLE "GUC: default_static_analyzer_ruleset" and also
> passed in as a modifier in the EXPLAIN invocation.

If you knew there's a good chance you might make a mistake, you could 
probably avoid doing it in the first place.  I make mistakes all the 
time, would I then always execute two commands when I want to do 
something?  Having to run a special "check my query please" command 
seems silly.

> Anyway, the idea of using a GUC and evaluating every query that is written
> (the added if statements), is not that appealing even if the impact of one
> more check is likely to be insignificant (is it?)

I'm not sure how you would do this differently in the EXPLAIN (STATIC) 
case.  Those ifs have to be somewhere, and that's always a non-zero cost.

That being said, yes, performance costs of course have to be evaluated 
carefully.


.marko

On 2014-09-05 11:33 PM, David G Johnston wrote:
> To protect users on every query they write there would need to be some kind
> of "always explain first and only execute if no warnings are thrown"
> mode...and ideally some way to then override that on a per-query basis if
> you know you are correct and don't want to fix the SQL...
>
> If the static check fails the query itself would error and the detail would
> contain the status result of the static analysis; otherwise the query should
> return as normal.

This feels even sillier.  Instinctively, if you can contain the 
functionality into the EXPLAIN path only, I don't see why you couldn't 
do it in a single  if (..)  for every query.  I doubt you could ever 
measure that difference.

> This at least avoids having to introduce 10 different GUC just to
> accommodate this feature and neatly bundles them into named packages.

I disagree.  Even if there was such a "static analysis" mode, I think 
there would have to be some way of filtering some of them out.

But "10 difference GUCs" can still be avoided; see 
plpgsql.extra_warnings, for example.


.marko