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.

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

ok so this is quite a big patch. it fixes the schema/prefix issues around findTables(), tableExists() and columnExists() also there is a function now to defuse $db_prefix for any database.prefix type prefixes. there still is issues surrounding using schemas in postgres such as they don't install. its on my todo list

ok that last patch was crap, after talking to DamZ on IRC, I came up with this, its better, not sure if its right just yet

The last submitted patch failed testing.

@josh : your patch failed because :

$this

the idea of making 'database.schema.table ' apart of prefixTables sounded great BUT, we overlooked indexes, you can't say for example: CREATE INDEX drupal7.public.simpletest305454batch_token_idx.... because the dot(.) would be a syntax error.

suggestions?

ok this patch moves things around a bit in the very core of DBTNG. if should pass with 4 fails on the 'Module list functionality' simple test in system, the query seems fine but the tables actually don't exist that I can see (unless they are removed after tests) I would love for someone to review and help me figure out the issue.

Explaination:
this patch proposes that every table is referenced like 'database.schema.tablename' or 'database.tablename' for MySQL. This is so that in MySQL querys can be queried across databases and in PostgreSQL, querys can be queried aross schemas.
In order to do this, the prefixTables function was re-written to so that each {table} became database.schema.tablename.

I think the findTables function still isn't right which is why there is 4 fails.

look forward to the feedback

OMG this one passes

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.

as per Crell's feedback

The last submitted patch failed testing.

spelling errors fixed from previos patch - rename tables sql error found on postgres also fix

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

@Damien: I like what you've done, I can see a couple of things that won't work in Postgres but I'll review and re-submit anyway

fixed bugs created by 3am madness including syntax etc, works on postgres

The last submitted patch failed testing.

the reason this patched failed was because of two things:
* The Module list functionality test attempted to find a table and prefixing that table before search where as now all the prefixing is kept within the schema and shouldn't be used outside of it.
* The operator (2nd param) of DamZ's function buildTableNameCondition wasn't being passed to the condition meaning '=' was being used instead of 'LIKE'

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.

ok try again

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

The last submitted patch failed testing.

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.

Hope these comments help clear things up

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

Upper case Drupal

The last submitted patch failed testing.

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).

I tested this this morning. Core still has 5 fails under postgres, after applying this patch, that is reduced to 2. So am keen to get this commited!

tested on PostgreSQL 8.2.9.

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

all worked as expected.

correction! with patch applied there is only 1 fail to HEAD with PostgreSQL! #355225: Inconsistant Insert Queries Between Database Drivers is addressing that single fail

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

err, someone mentioned to me that no one has tested mysql yet. was a good point because there was a syntax error in the mysql schema.inc.
After correcting the syntax error I ran the entire simpletest suite and got 8162 passes, 0 fails, and 0 exceptions.

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 guess then it would confuse most MySQL people that they store their database names in a column called table_schema in the information_schema. As mentioned in #1, what postgres does is more SQL standard and MySQL concept of a database if more like a schema. It even be educating to those MySQL people who look into the code to learn this.

re-roll with changes requested by Crell

forgot to set to review

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

Committed to CVS HEAD. Thanks! :)

Automatically closed -- issue fixed for two weeks with no activity.