Re: [PATCH] parser: optionally warn about missing AS for column and table aliases

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.