Обсуждение: 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
Вложения
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