All schema API operations should be reserved-word safe

@earnie, I disagree. This is a much narrower issue.

And we can't quote with ` because ` is MySql only.

That's great because we are in a MySQL context.

@earnie: it seems that we are taking the "don't use any of the reserved-words of any database engine and battle to change them if the list evolves" route. If we do, we have *no other choice* than to make *all* schema API operations reserved-word safe, because if we don't, we will not be able (as the original poster) to change the name of any column that uses a reserved word.

This is actually a prerequisite for fixing the use of reserved words, since we can't fix the problem otherwise. So this issue is valid, the other should be marked "postponed".

Visually the patch looks fine, but I need to try applying it and testing it first. I'll try to get to that soon.

Looks fine, applies fine. There's no Schema API unit tests yet, sadly, but this is trivial enough that I'm confident that it breaks nothing.

However, this only addresses MySQL. Can we give Postgres some love? (It is still a supported database, even if no one uses it.) Or should we push that off to a separate patch? (webchick/Dries, your call, given how lackluster Postgres interest is.)

hm. I don't know Postgres well enough to know either way, and we're still lacking good schema API code coverage on unit tests to check it. (And I am scheming to rewrite Schema API anyway).

Given that there are reserved word columns in core now, and we will need Schema API to support them in order to convert away from them, we can't simply ignore the problem.

I think I'm actually going to push this up to RTBC. Committers, if you really want Postgres checked/fixed as part of this patch, find me a Postgres person who can. :-) Otherwise let's get MySQL taken care of and fix Postgres (if needed) separately.

I committed this to CVS HEAD. I'm marking this 'active' as the PostgreSQL case needs fixing too.

Marking accordingly.

You shouldn't have a problem with PostgreSQL and reserved words in newer versions:

postgres=# \d type_test;
 Table "public.type_test"
 Column | Type | Modifiers 
--------+------+-----------
 type   | text | 

type is a reserved word.

postgres=# select type from type_test;
 type 
------
(0 rows)

If you want to be doubly sure (and conform to standard) you could do this:

postgres=# select "type" from type_test;
 type 
------
(0 rows)

Oh and BTW don't be fooled, lots of people use Drupal + PostgreSQL.

@linuxpoet: So the postgres driver is safe as-is, and doesn't need any more work? If so, let's mark this fixed. Or we can add " in the schema operations like we do ` for MySQL. Whichever you think is safer.

Postgres will still complain if you use certain reserved words - GROUP comes to mind as an example:

postgres=# select group from mytable;
ERROR: syntax error at or near "group"
LINE 1: select group from mytable;
^
postgres=# select version();
version
----------------------------------------------------------------------------------
PostgreSQL 8.3.3 on i486-pc-linux-gnu, compiled by GCC cc (Debian 4.3.1-1) 4.3.1
(1 row)

OK, so we need to quote fields in postgres, too. Is ' (apostrophe) the right character for that? If so, can you roll a quick patch a la the MySQL one above?

Patch rolled for Postgres following earnie's suggestion that the double quote is the proper escaping character.

I only wrapped them around the name of the existing field, and not the new field to stay in sync with the MySQL patch above.

Setting to 'needs review' - testbot was broken.

tagging for next Postgres Code Sprint

adding double quotes around the renamed old field. Doesn't hurt. Testbot shouldn't fail, it doesn't test PostgreSQL (yet), this is a no brainer to comit

Committed to HEAD. Thanks.

Island Usurper's patch in #26 is good. RTBC.

Committed to Drupal 6 too, thanks.

I came across another instance of this bug w/ Drupal 6.25 when using a reserved word (version) and trying to add an index at the same time. Here's my code in my module's update hook:

  db_change_field( $ret, 'rmgt_subscription', 'version', 'version', array(
    'type' => 'varchar',
    'length' => 50,
    'default' => 'print',
    'not null' => TRUE,
  ), array( 'indexes' => array( 'by_version' => array( array( 'version', 10 ) ) ) ) );

Results in this output and the field not being changed:

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use [warning]
near 'version(10))' at line 1
query: ALTER TABLE rmgt_subscription CHANGE `version` `version` VARCHAR(50) NOT NULL DEFAULT 'print', ADD INDEX
by_version (version(10)) database.mysqli.inc:169

Sorry about that ... I figured it would be better organized for my query to appear with the same bug.

Thanks!