Thread

1. Making OFF unreserved

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-10-22T07:43:22Z

   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
   

2. Re: Making OFF unreserved

   Tom Lane <tgl@sss.pgh.pa.us> — 2010-10-22T13:54:47Z

   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
   
   

3. Re: Making OFF unreserved

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-10-22T14:47:08Z

   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