PDO should quote index names in table declarations.

Interesting. Can you roll a patch for it?

Rather than using `, which is MySQL-specific, use $connection->quoteTable(). (I think it's a safe bet that the table quotes are valid here.)

Ah, OK. Yeah, that makes sense since the schema code differs greatly from one DB to another. Still, can you roll a patch? :-)

#140860: Consistent table names and database handling of table names has brought to my attention that maybe it is time that Drupal starts quoting all tables. We can use Database::prefixTables() how ever things like {tablename}_column_idx would need to be re-thought. It would also prevent a dbprefix being used to seperate tables over schemas/databases: schema.table or database.table must be schema."table" rather than "schema.table".

@Crell, it this a good place to discuss? is there a better issue around? thoughts anyhow?

I'm all for being more pedantic with the syntax we send to the database, provided we can do so consistently and without introducing performance problems. I don't know if we can pull that off, as #5 notes.

I think that's a separate issue from this one, however, as this is about mucking about in the schema. Let's stick to that for now, where we do need to better quote things.

I found my comment relates better to #371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements - a 6 year old issue that has faced these issues again and again

Once identifiers are quoted they become case sensitive.
SQL standard says they should be downfolded to uppercase if not quoted... unfortunately this is one of the rare cases where PostgreSQL doesn't follow the standard and downfold object to lowercase. (There is also another small issue with a pg function that behaves inconsistently, but I can't remember which one right now).
Quoting objects is going to raise a lot of issues especially in the upgrade path.

Consider also people never thought that objects were going to become case sensitive and they may have used them inconsistently.

Well table names are already case sensitive in MySQL on *nix, and Drupal convention has always been to lowercase everything, so I don't know how much of a problem that would be. Drupal developers should already be used to treating identifiers as all-lowercase.

I belive the PDO driver for PostgreSQL has issues with quoting tablenames. It would be better to reference tables by scope ie schema.table/db.table. To implement this we would reqiure defaults - 'public' for PostgreSQL and 'dbname' for MySQL (SQLite?) and also let the scope be overridable by db_prefix so we can have tables in different schemas or seperate out databases for different projects - what ever the end user's reason maybe.

You may have to deal with tables not strictly belonging to drupal and "forcing" case sensitiveness may become a burden.
Quoting seems a workaround to problems that are actually related to bad naming of identifiers and a now too simple implementation of prefixing.
It would be better to improve the choice of identifiers (not necessarily all at the same time) and the prefixing system.

Drupal shouldn't get into the way to get to the metal (SQL) or at least to offer different degree of "service".

I feel it should be under the control of the programmer if he want quoted or unquoted identifiers.

Quoting is not a good excuse to avoid reserved words as a longer term target or make the query system as a whole smarter (support better prefixing/schema and shared tables).

I'm OK with this patch (but it differs from Crell's quoteTable() suggestion above). I'd like to ask Crell to share his thoughts on this patch.

Nice and simple and to the point, which I like. :-) We don't need to deal with the larger question of naming conventions here. However, I would rather use the quoteTable() method than manually use the backticks here. It keeps the quoting logic in all one place and consistent. (Unless there are cases where tables and indexes need to be quoted differently in MySQL, which I don't believe there are.)

The schema object maintains a reference to its corresponding connection object at $this->connection. So it would be $this->connection->quoteTables(). See the first line of actual code in includes/database/schema.inc.

This change obviously broke everything ;)

-        $keys[] = 'UNIQUE KEY ' . $key .' ('. $this->createKeysSqlHelper($fields) . ')';
+        $keys[] = 'UNIQUE KEY `' . $this->connection->quoteTables($key) .'` ('. $this->createKeysSqlHelper($fields) . ')';

subscribe

is seems wrong to use the method 'quoteTables' on a piece of syntax that doesn't represent a table at all.
is it time yet for a method like 'escapeSQLSyntax' ?

I vaguely recall that some DBs may use different characters from some of the old threads with hswong3i regarding Oracle, but I don't recall the details. Can someone check those threads and see?

Whilst MySQL uses backticks (`) to quote table and column names etc, PostgreSQL uses double quotes ("), note that MySQL can also use double quotes if configured too. I have no idea about Orcale, SQLite or MSSQL. Surely this isn't an issue though since each driver could identify the character to use.

@Arancaytar, how about just ->identifier() as the function name? This could then suggest that the driver does not need to quote the identifier but is given the oppurtunity to preform actions on the identifier such as quoting.

This function will also need to be addressed in prefixTables(), how do we prefix the tables first?, then quote them? On top of that, what about schema use? database/schema.table?

Well, there's two issues here.

1) Every time we quote something dynamically we have another stack call, which is a performance hit. A tiny one, but remember we're dealing with critical path code so it could get called a lot.

2) On some databases, all identifiers use the same quotations. Some databases may not, but we're not sure yet. :-) That will determine if we have a single quoteIdentifier() method or separate quoteTable() and quoteOtherIdentifiers() methods.

Can someone check at least SQLite and MS SQL? (SQLite we have to support, and MS SQL I think is more likely to get support than Oracle at this rate.) If those are both single-identifier, then let's go with a single method, implement it everywhere, and then denormalize (for lack of a better term) as needed for performance. If either of those has separate identifiers, then let's go with the multi-method approach and then do the same sort of denormalization.

in response to #25:
1) if we didn't want to add another stack call then we could use syntax: {tablename} and [columnname]. This would be easy enough to implement in the db layer with functions like db_insert/update/merge/select/delete. Unlike {}, [] could be recommended but optional for calls with db_query, because writing SQL with {} and [] would really start to get ridiculous. At least with the other methods we could hide it in the db layer.

2) my solution to 1 would potentially solve this issue

thoughts?

Using [] for field names was already rejected a long time ago as too much of a DX WTF.

DX?

"Developer eXperience". Vis, it makes writing SQL for Drupal too weird and difficult for people familiar with SQL but not already immersed in Drupal.

I found a few other places where we need to quote an index or column name. This is using backticks rather than a quoteIdentifier() method but, let's just see if it passes tests.

Great, I don't see why we can't do this, manually insert escaping on index names since the db layer takes care of it. However, lets do this for all databases, not just MySQL.

mfb, can you re-roll the patch with index escaping for PostgreSQL and SQLite. PostgreSQL uses a double quote as its delimiter, not sure about SQLite, you'll have to find that one out.

Apparently double quotes are preferred in sqlite: http://www.sqlite.org/lang_keywords.html

By the way, it seems that column and table names in query.inc should be quoted just as column, table and index names in schema.inc, but this issue seems to be focused on schema.inc.

Also, there is no quoteTable() method mentioned above but there is escapeTable(), also known as http://api.drupal.org/api/function/db_escape_table/7

Lets just handle index escaping at the moment. As quoting columns and tables is a bigger task, There is a prefixTables method in DatabaseConnection where we would could escape tables but we should handle that in a seperate patch, potentially on a seperate issue.

I've attached a patch with postgres and sqlite equivalents of mfb's earlier patch. There's not much do to with sqlite as it doesn't support ALTER TABLE and requires table recreation instead.

Stuart

Munged both patches into a single file.

I don't see the create index being escaped in the the pgsql schema.inc

There is another problem with DBTNG that I'd like to fix while we're here. It *was* possible to run drupal or parts of it on separate schemas at one stage in PostgreSQL. By specifying the table 'prefix' as something like 'drupal.' Drupal could be installed inside of a schema called 'drupal'. However the function below attempts to create an INDEX called drupal.table_name_idx which doesn't make sense because AFAIK indexes are outside of the schema scope so they cannot be referenced as such.

I believe this is a documented feature in Drupal 7 so we should make this work. Bumping this up to critical.

I'll post a patch soon.

The patch to me previous comment is bigger than this issue so we'll stick with just the index and constraint quoting here for now. I added an additional line to stuzza'a patch which correctly quotes the index name in PostgreSQL.

removing last part of title. does not apply here.

Committed to CVS HEAD. Thanks all!