[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
Attachments:
0001-parser-optionally-warn-about-missing-AS-for-column-a.patchtext/plain; charset=us-asciiDownload
>From 478e694e5281a3bf91e44177ce925e6625cb44a5 Mon Sep 17 00:00:00 2001
From: Oskari Saarenmaa <os@ohmu.fi>
Date: Fri, 5 Sep 2014 22:31:22 +0300
Subject: [PATCH] parser: optionally warn about missing AS for column and table
aliases
Add a new GUC "missing_as_warning" (defaults to false, the previous
behavior) to raise a WARNING when a column or table alias is used without
the AS keyword.
This allows catching some types of errors where another keyword or a comma
was missing and a label is being used as an alias instead of something else,
for example cases like:
SELECT COUNT(*) files;
SELECT * FROM files users;
SELECT path size FROM files INTO f_path, f_size;
---
src/backend/parser/gram.y | 24 +++++
src/backend/utils/misc/guc.c | 11 +++
src/include/parser/parser.h | 2 +
src/test/regress/expected/select.out | 170 +++++++++++++++++++++++++++++++++++
src/test/regress/sql/select.sql | 47 ++++++++++
5 files changed, 254 insertions(+)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b46dd7b..06a71dd 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -65,6 +65,10 @@
#include "utils/xml.h"
+/* GUCs */
+bool missing_as_warning = false;
+
+
/*
* Location tracking support --- simpler than bison's default, since we only
* want to track the start position not the end position of each nonterminal.
@@ -10119,12 +10123,20 @@ alias_clause:
}
| ColId '(' name_list ')'
{
+ if (missing_as_warning)
+ ereport(WARNING,
+ (errmsg("alias without explicit AS and missing_as_warning enabled"),
+ parser_errposition(@3)));
$$ = makeNode(Alias);
$$->aliasname = $1;
$$->colnames = $3;
}
| ColId
{
+ if (missing_as_warning)
+ ereport(WARNING,
+ (errmsg("alias without explicit AS and missing_as_warning enabled"),
+ parser_errposition(@1)));
$$ = makeNode(Alias);
$$->aliasname = $1;
}
@@ -10156,6 +10168,10 @@ func_alias_clause:
| ColId '(' TableFuncElementList ')'
{
Alias *a = makeNode(Alias);
+ if (missing_as_warning)
+ ereport(WARNING,
+ (errmsg("alias without explicit AS and missing_as_warning enabled"),
+ parser_errposition(@1)));
a->aliasname = $1;
$$ = list_make2(a, $3);
}
@@ -10244,6 +10260,10 @@ relation_expr_opt_alias: relation_expr %prec UMINUS
| relation_expr ColId
{
Alias *alias = makeNode(Alias);
+ if (missing_as_warning)
+ ereport(WARNING,
+ (errmsg("alias without explicit AS and missing_as_warning enabled"),
+ parser_errposition(@2)));
alias->aliasname = $2;
$1->alias = alias;
$$ = $1;
@@ -12577,6 +12597,10 @@ target_el: a_expr AS ColLabel
*/
| a_expr IDENT
{
+ if (missing_as_warning)
+ ereport(WARNING,
+ (errmsg("alias without explicit AS and missing_as_warning enabled"),
+ parser_errposition(@2)));
$$ = makeNode(ResTarget);
$$->name = $2;
$$->indirection = NIL;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index af667f5..03b7457 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1398,6 +1398,17 @@ static struct config_bool ConfigureNamesBool[] =
},
{
+ {"missing_as_warning", PGC_USERSET, COMPAT_OPTIONS_PREVIOUS,
+ gettext_noop("Warn about missing AS for column or table aliases."),
+ NULL,
+ GUC_REPORT
+ },
+ &missing_as_warning,
+ false,
+ NULL, NULL, NULL
+ },
+
+ {
{"escape_string_warning", PGC_USERSET, COMPAT_OPTIONS_PREVIOUS,
gettext_noop("Warn about backslash escapes in ordinary string literals."),
NULL
diff --git a/src/include/parser/parser.h b/src/include/parser/parser.h
index 8d2068a..b3e8a71 100644
--- a/src/include/parser/parser.h
+++ b/src/include/parser/parser.h
@@ -30,6 +30,8 @@ extern int backslash_quote;
extern bool escape_string_warning;
extern PGDLLIMPORT bool standard_conforming_strings;
+/* GUC variables in gram.y */
+extern bool missing_as_warning;
/* Primary entry point for the raw parsing functions */
extern List *raw_parser(const char *str);
diff --git a/src/test/regress/expected/select.out b/src/test/regress/expected/select.out
index c376523..8cce371 100644
--- a/src/test/regress/expected/select.out
+++ b/src/test/regress/expected/select.out
@@ -781,3 +781,173 @@ select * from (values (2),(null),(1)) v(k) where k = k;
1
(2 rows)
+-- Test warnings for missing AS for aliases
+SET missing_as_warning TO false;
+select 1 foo, 2 bar;
+ foo | bar
+-----+-----
+ 1 | 2
+(1 row)
+
+select 1 as foo, 2 as bar;
+ foo | bar
+-----+-----
+ 1 | 2
+(1 row)
+
+select count(*) foo;
+ foo
+-----
+ 1
+(1 row)
+
+select 1 from onek o1 limit 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+select 1 from onek o1, onek o2 limit 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+select 1 from onek as o1, onek as o2 limit 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+select un1 from onek ok (un1) where un1 = 88888888;
+ un1
+-----
+(0 rows)
+
+select un1 from onek as ok (un1) where un1 = 88888888;
+ un1
+-----
+(0 rows)
+
+update onek ok set unique1 = -88888888 where unique1 = 88888888;
+update onek as ok set unique1 = -88888888 where unique1 = 88888888;
+create function func_for_missing_as() returns record language sql as 'select 1::int';
+select * from func_for_missing_as() f (id int);
+ id
+----
+ 1
+(1 row)
+
+select * from func_for_missing_as() as f (id int);
+ id
+----
+ 1
+(1 row)
+
+SET missing_as_warning TO true;
+select 1 foo, 2 bar;
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: select 1 foo, 2 bar;
+ ^
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: select 1 foo, 2 bar;
+ ^
+ foo | bar
+-----+-----
+ 1 | 2
+(1 row)
+
+select 1 as foo, 2 as bar;
+ foo | bar
+-----+-----
+ 1 | 2
+(1 row)
+
+select count(*) foo;
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: select count(*) foo;
+ ^
+ foo
+-----
+ 1
+(1 row)
+
+do $$
+declare
+ i1 int;
+ i2 int;
+begin
+ select unique1 unique2 FROM onek into i1, i2;
+ select unique1, unique2 FROM onek into i1, i2;
+end;
+$$;
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 6: select unique1 unique2 FROM onek into i1, i2;
+ ^
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: select unique1 unique2 FROM onek
+ ^
+QUERY: select unique1 unique2 FROM onek
+CONTEXT: PL/pgSQL function inline_code_block line 6 at SQL statement
+select 1 from onek o1 limit 1;
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: select 1 from onek o1 limit 1;
+ ^
+ ?column?
+----------
+ 1
+(1 row)
+
+select 1 from onek o1, onek o2 limit 1;
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: select 1 from onek o1, onek o2 limit 1;
+ ^
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: select 1 from onek o1, onek o2 limit 1;
+ ^
+ ?column?
+----------
+ 1
+(1 row)
+
+select 1 from onek as o1, onek as o2 limit 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+select un1 from onek ok (un1) where un1 = 88888888;
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: select un1 from onek ok (un1) where un1 = 88888888;
+ ^
+ un1
+-----
+(0 rows)
+
+select un1 from onek as ok (un1) where un1 = 88888888;
+ un1
+-----
+(0 rows)
+
+update onek ok set unique1 = -88888888 where unique1 = 88888888;
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: update onek ok set unique1 = -88888888 where unique1 = 88888...
+ ^
+update onek as ok set unique1 = -88888888 where unique1 = 88888888;
+select * from func_for_missing_as() f (id int);
+WARNING: alias without explicit AS and missing_as_warning enabled
+LINE 1: select * from func_for_missing_as() f (id int);
+ ^
+ id
+----
+ 1
+(1 row)
+
+select * from func_for_missing_as() as f (id int);
+ id
+----
+ 1
+(1 row)
+
+drop function func_for_missing_as();
+SET missing_as_warning TO false;
diff --git a/src/test/regress/sql/select.sql b/src/test/regress/sql/select.sql
index b99fb13..50823b5 100644
--- a/src/test/regress/sql/select.sql
+++ b/src/test/regress/sql/select.sql
@@ -207,3 +207,50 @@ drop function sillysrf(int);
-- (see bug #5084)
select * from (values (2),(null),(1)) v(k) where k = k order by k;
select * from (values (2),(null),(1)) v(k) where k = k;
+
+-- Test warnings for missing AS for aliases
+SET missing_as_warning TO false;
+select 1 foo, 2 bar;
+select 1 as foo, 2 as bar;
+select count(*) foo;
+
+select 1 from onek o1 limit 1;
+select 1 from onek o1, onek o2 limit 1;
+select 1 from onek as o1, onek as o2 limit 1;
+select un1 from onek ok (un1) where un1 = 88888888;
+select un1 from onek as ok (un1) where un1 = 88888888;
+update onek ok set unique1 = -88888888 where unique1 = 88888888;
+update onek as ok set unique1 = -88888888 where unique1 = 88888888;
+
+create function func_for_missing_as() returns record language sql as 'select 1::int';
+select * from func_for_missing_as() f (id int);
+select * from func_for_missing_as() as f (id int);
+
+SET missing_as_warning TO true;
+select 1 foo, 2 bar;
+select 1 as foo, 2 as bar;
+select count(*) foo;
+
+do $$
+declare
+ i1 int;
+ i2 int;
+begin
+ select unique1 unique2 FROM onek into i1, i2;
+ select unique1, unique2 FROM onek into i1, i2;
+end;
+$$;
+
+select 1 from onek o1 limit 1;
+select 1 from onek o1, onek o2 limit 1;
+select 1 from onek as o1, onek as o2 limit 1;
+select un1 from onek ok (un1) where un1 = 88888888;
+select un1 from onek as ok (un1) where un1 = 88888888;
+update onek ok set unique1 = -88888888 where unique1 = 88888888;
+update onek as ok set unique1 = -88888888 where unique1 = 88888888;
+
+select * from func_for_missing_as() f (id int);
+select * from func_for_missing_as() as f (id int);
+
+drop function func_for_missing_as();
+SET missing_as_warning TO false;
--
1.9.3
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2014-09-05 23:06 GMT+02:00 Marko Tiikkaja <marko@joh.to>:
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..
I had same idea - maybe some general mode "prune human errors" mode
Pavel
Show quoted text
.marko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2014-09-06 0:53 GMT+02:00 Marko Tiikkaja <marko@joh.to>:
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.
You used a good name for these mode in other thread "defensive". We can
support some "defensive" mode. Personally I am sure, so if somebody would
to use it, it would to use it as complex. Too less granularity increase a
complexity of Postgres configuration and we don't would it.
Novice mode is not correct name and can has degrading character.
In this mode people maybe got more very specific warnings (DBA doesn't like
it, because logs are massive), developers should to use explicit casting
much more (developers doesn't like explicit casting in SQL). But when we
use a good name, then they can accept it and use it. It is paradox, so
defensive mode is mainly for professionals with experience - absolute
beginner probably will hate this mode due too restrictivity
Regards
Pavel
Show quoted text
.marko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers