Schema function findTables fails on postgres - wrong SQL

As discussed on the IRC, we need to think about the relationship between the concepts of "Database", "Schema", "Drupal prefix".

The PostgreSQL concept of Schema (defined by the SQL standard) is quite close to the concept of "Drupal prefix": you can do queries across tables in different schemas in the same database, but you can't do queries across tables in different databases. MySQL concept of "Database" is more a "Schema" than anything else.

PostgreSQL: database (closed container) -> schema (open container) -> tables
MySQL: database (open container) -> tables
Drupal: prefix (open container) -> tables

Also related: #286986: MySQL: SHOW TABLES LIKE database.tablename doesn't work, that try to unify tableExists() and columnExists() across database engines.

@josh : your patch failed because :

':table_name' => this->explodeTable($table_expression)->table,

$this

The ternary at the top of prefixTables() is slower than doing an if (empty($info)) { ... } check. Let's do that instead.

+1 on making prefixTables a method rather than a static method.

db_result() is deprecated. findTables() should be using db_query()->fetchField() instead.

findTables() should only call explodeTable() once and save the result. Calling it 3 times results in unnecessary parsing. Optionally explodeTable() could save the parsed data to an object property and just return it next time. Or both. :-)

The docblock for explodeTable() doesn't explain what "Better" means. Huh?

Can you include a comment describing why the mysql prefixTables() has to be different than the global one?

I think I like what I'm seeing here. It's a good direction for schema to move, but it still has a long way to go.

Small code style issues. Comments should be styled as sentences (capitalize first word, period at the end). Example problem:

+ // require schema and database info

Also, we should avoid shorthand variable names, like $pfx and $tbl_bits.

I do like that this is getting us closer to compatibility with the SQL standard for RDBMSs that support it.

Here is something a little bit more with what I had in mind. With a little bit of condition magic (warning: you can't use db_select() in that context, because it prefixes all tables), it's possible to abstract all particularities of PostgreSQL and MySQL regarding the information schema. This would probably work out-of-the-box too with SQL Server.

Warning 1: this is untested
Warning 2: it is 3am, so it could even not make sense at all

Ok, in fact it is a mistake to prefix inside buildTableNameCondition() because that function assume that the table name passed to it is already prefixed, so that dotted tables can be split correctly.

So the prefixing should be moved back into tableExists() and columnExists() (those get as input a non-prefixed table). findTable() is good because, as a (well documented) special case, it actually takes an already prefixed table as input.

No changes are necessary in the tests.

You will need to manually add the enclosing "{}" before calling prefixTables(). Also, the MySQL implementation of buildTableNameCondition() was not fixed.

Slave #9 is apparently buggy.

Resubmit to force retesting.

This patch does the right thing. However, I think the different buildTableNameCondition() functions should have more phpdoc. A lot of the knowledge (and depth of information) captured in this issue is completely lost in the phpDoc. That is unfortunate because I found the information in comment #1 to be key in order to be able to understand this patch. The information in #1 was not new to me, but nonetheless, I was necessary to refresh my memory and to contrast the two databases. It helped tremendously. In other words, please explain _why_ this function is useful, _why_ the code is structed like this, explain terminology like information_schema a tiny bit better, and make sure people are setup to be able to understand this patch. Thanks!

All things considered, this issue illustrates _exactly_ why we should have more than one database driver in core. It forces us to design and architect things properly. It we'd only support MySQL, our database abstraction layer would probably be a failure.

Keep up the good work, folks.

+   * The information_schema is a SQL standard that provides information about the
+   * database server and the databases, schemas, tables, columns and users within
+   * it. This makes information_schema a useful tool to use across the drupal 
+   * database drivers and is used by a few different functions. The function below
+   * describes the conditions to be meet when querying information_schema.tables
+   * for drupal tables or information associated with drupal tables. Even though
+   * this is the standard method, not all databases follow standards and so this
+   * method should be overwritten by a database driver if the database provider
+   * uses alternate methods. Because information_schema.tables is used in a few
+   * different functions, a database driver will only need to override this function
+   * to make all the others work. For example see includes/databases/mysql/schema.inc.

Per coding standards, "drupal" is written as "Drupal".

Straight reroll... the last fail was a testing glitch.

tagging.

I really think this is ready. Can we have someone to test it manually (drewish, please help if you can).

tested on PostgreSQL 8.2.9.

tested lotsa db_create_table & db_drop_table from hook_install/uninstall. --

all worked as expected.

This is a pretty large patch that changes a lot of DBTNG internals. I'd really like Crell's input on this before committing.

For MySQL readers, the concept of "schema" will be totally alien. (What Postgres calls a schema, MySQL calls a database. And then there is no equivalent for what Postgres calls a database, AFAIK.) That's especially true given the Drupal usage of the word "schema". I'm not entirely sure how we should address that, but as a MySQL guy I found some of the comments and logic a bit confusing as a result. [Edit: At least from reading schema.inc. The comment in for mysql/schema.inc explains it. That may be sufficient; I'm just flagging it as a possible confusion point we should consider.]

I do not like the string concatenation in tableExists() for the conditional. I completely understand why we're doing it, but given how much effort we're putting into avoiding string manipulation for queries it still makes me uneasy. At the very least we should document why we're doing something that we heartily discourage elsewhere.

The comments for findTables() say that the caller is responsible for handling table prefixing. Since prefixTables() is now an object method rather than class method with this patch (which I very much agree with), should we make it public so that said caller can prefix the table properly? Seems sensible to me to do.

The docblock for DatabaseSchema_mysql::buildTableNameCondition() is invalid, as it has a multi-line first description.

So not quite RTBC, but the approach makes sense to me provided we document some of the somewhat uncooth query building we're doing properly.

I'd say this is ready now. Nice work, guys!

Committed to CVS HEAD. Thanks! :)