Re: Re: [PATCH] parser: optionally warn about missing AS for column and table aliases - Mailing list pgsql-hackers

From David Johnston
Subject Re: Re: [PATCH] parser: optionally warn about missing AS for column and table aliases
Date
Msg-id CAKFQuwbVUPOHpMzrtU04UN0_Mw9MjZGMG2c5WBLfK7MoT2J9bg@mail.gmail.com
Whole thread Raw
In response to Re: Re: [PATCH] parser: optionally warn about missing AS for column and table aliases  (Marko Tiikkaja <marko@joh.to>)
List pgsql-hackers
On Fri, Sep 5, 2014 at 6:27 PM, Marko Tiikkaja <marko@joh.to> wrote:
On 2014-09-05 11:19 PM, David G Johnston wrote:
Marko Tiikkaja-4 wrote

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.

​This option implemented in this specific manner.​


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.

​My follow-on post posited a solution that still uses the EXPLAIN mechanism but configured in a way so users can have it be always on if desired.​


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.

​If you add lots more checks that is lots more ifs compared to a single if to see if static analysis should be attempted in the first place.  For a single option its largely a wash.  Beyond that I don't know enough to say whether having the parser dispatch the query differently based upon it being preceded with "EXPLAIN (STATIC)" or a standard query would be any faster than a single IF for a single check.​

​The more options you can think of for a "novice" mode the more appealing a formal static analysis interface becomes - both for execution and also because the number of options being introduced and managed can be made into formal configurations instead of an independent but related set of GUC variables.

​David J.
 

pgsql-hackers by date:

Previous
From: Marko Tiikkaja
Date:
Subject: Re: Re: [PATCH] parser: optionally warn about missing AS for column and table aliases
Next
From: Marko Tiikkaja
Date:
Subject: Re: Re: [PATCH] parser: optionally warn about missing AS for column and table aliases