Lists: | pgsql-hackers |
---|
From: | Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> |
---|---|
To: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Making OFF unreserved |
Date: | 2010-10-22 07:43:22 |
Message-ID: | 4CC1409A.8060002@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
make-off-unreserved-1.patch | text/x-diff | 1.4 KB |
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Making OFF unreserved |
Date: | 2010-10-22 13:54:47 |
Message-ID: | 21367.1287755687@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)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
From: | Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Making OFF unreserved |
Date: | 2010-10-22 14:47:08 |
Message-ID: | 4CC1A3EC.20504@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On 22.10.2010 16:54, Tom Lane wrote:
> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)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