Making OFF unreserved
OFF is a reserved keyword. It's not a reserved keyword in the SQL spec,
and it's not hard to see people using off as a variable or column name,
so it would be nice to relax that. To make things worse, OFFSET is also
a reserved keyword, which would be the other natural name for a variable
or column that stores an offset of some sort.
I bumped into this because we have a test case in the EDB regression
suite that uses 'off' as a PL/pgSQL variable name. It used to work
before 9.0, because PL/pgSQL variable names were replaced with $n-style
parameter markers before handing off the query to the backend parser.
It's a problem with all keywords in general, but 'off' seems like a
likely variable name in real applications, and there was no ambiguity
with it.
Looking at the grammar, OFF is only used here:
opt_boolean:
TRUE_P { $$ = "true"; }
| FALSE_P { $$ = "false"; }
| ON { $$ = "on"; }
| OFF { $$ = "off"; }
;
And opt_boolean in turn is used in the following places:
var_value: opt_boolean
{ $$ = makeStringConst($1, @1); }
| ColId_or_Sconst
{ $$ = makeStringConst($1, @1); }
| NumericOnly
{ $$ = makeAConst($1, @1); }
;
...
copy_generic_opt_arg:
opt_boolean { $$ = (Node *) makeString($1); }
| ColId_or_Sconst { $$ = (Node *) makeString($1); }
...
copy_generic_opt_arg_list_item:
opt_boolean { $$ = (Node *) makeString($1); }
| ColId_or_Sconst { $$ = (Node *) makeString($1); }
;
...
explain_option_arg:
opt_boolean { $$ = (Node *) makeString($1); }
| ColId_or_Sconst { $$ = (Node *) makeString($1); }
Note that ColId is also accepted alongside opt_boolean in all of those
with the same action, so if we just remove OFF from opt_boolean rule and
make it unreserved, nothing changes.
ECPG uses OFF as a keyword in its "SET autocommit = [ON | OFF]" rule, so
we have to retain it as an unreserved keyword, or make it an
ecpg-specific keyword in the ecpg grammar. But I don't know how to do
that, and it feels like a good idea to keep it in the unreserved keyword
list anyway, so I propose the attached patch.
Any objections? Any objections to backpatching to 9.0, where the
PL/pgSQL variable handling was changed?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Attachments:
make-off-unreserved-1.patchtext/x-diff; name=make-off-unreserved-1.patchDownload
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 609c472..e4ad40b 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1344,7 +1344,12 @@ opt_boolean:
TRUE_P { $$ = "true"; }
| FALSE_P { $$ = "false"; }
| ON { $$ = "on"; }
- | OFF { $$ = "off"; }
+ /*
+ * OFF is also accepted as a boolean value, but is not listed
+ * here to avoid making it a reserved keyword. All uses of
+ * opt_boolean rule also accept a ColId with the same action -
+ * OFF is handled via that route.
+ */
;
/* Timezone values can be:
@@ -11184,6 +11189,7 @@ unreserved_keyword:
| NULLS_P
| OBJECT_P
| OF
+ | OFF
| OIDS
| OPERATOR
| OPTION
@@ -11443,7 +11449,6 @@ reserved_keyword:
| LOCALTIMESTAMP
| NOT
| NULL_P
- | OFF
| OFFSET
| ON
| ONLY
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index d3ea04b..2c44cf7 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -262,7 +262,7 @@ PG_KEYWORD("nulls", NULLS_P, UNRESERVED_KEYWORD)
PG_KEYWORD("numeric", NUMERIC, COL_NAME_KEYWORD)
PG_KEYWORD("object", OBJECT_P, UNRESERVED_KEYWORD)
PG_KEYWORD("of", OF, UNRESERVED_KEYWORD)
-PG_KEYWORD("off", OFF, RESERVED_KEYWORD)
+PG_KEYWORD("off", OFF, UNRESERVED_KEYWORD)
PG_KEYWORD("offset", OFFSET, RESERVED_KEYWORD)
PG_KEYWORD("oids", OIDS, UNRESERVED_KEYWORD)
PG_KEYWORD("on", ON, RESERVED_KEYWORD)
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
OFF is a reserved keyword. It's not a reserved keyword in the SQL spec,
and it's not hard to see people using off as a variable or column name,
so it would be nice to relax that.
While I can see the value of doing something about that, this seems
awfully fragile:
+ /* + * OFF is also accepted as a boolean value, but is not listed + * here to avoid making it a reserved keyword. All uses of + * opt_boolean rule also accept a ColId with the same action - + * OFF is handled via that route. + */
The production's correctness now depends on how it's used, and there's
no way to prevent somebody from misusing it.
I think it'd be better if you were to refactor the grammar so that ColId
was actually one of the alternatives in this very production (call it
opt_boolean_or_name, or something like that). Then at least there'd be
less of a flavor of action-at-a-distance about the assumption that OFF
was handled in a compatible fashion.
regards, tom lane
On 22.10.2010 16:54, Tom Lane wrote:
Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes:
OFF is a reserved keyword. It's not a reserved keyword in the SQL spec,
and it's not hard to see people using off as a variable or column name,
so it would be nice to relax that.While I can see the value of doing something about that, this seems
awfully fragile:+ /* + * OFF is also accepted as a boolean value, but is not listed + * here to avoid making it a reserved keyword. All uses of + * opt_boolean rule also accept a ColId with the same action - + * OFF is handled via that route. + */The production's correctness now depends on how it's used, and there's
no way to prevent somebody from misusing it.I think it'd be better if you were to refactor the grammar so that ColId
was actually one of the alternatives in this very production (call it
opt_boolean_or_name, or something like that). Then at least there'd be
less of a flavor of action-at-a-distance about the assumption that OFF
was handled in a compatible fashion.
Ah yes, that's much better. Committed that way.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com