Table prefixes should be per database connection
David Stosik - November 26, 2007 - 14:57
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | chx-and-david-performance-sprint |
Description
Hello,
I think this one can only go in 7.x, as 6.x code is frozen...
Currently, we can specify a prefix per table name, and a default one, in the settings.php file. For example, we can have
<?php
$db_url = 'mysql://user:password@dbserver/db';
$db_prefix = 'drupal_';
?>For a single prefix applied to all tables.
Or we can have
<?php
$db_url = 'mysql://user:password@dbserver/db';
$db_prefix['users'] = '';
$db_prefix['default'] = 'drupal_';
?>To prefix with 'drupal_' all tables, except users table.
The lack comes when one is using multiple databases, with one connection string for each. The prefix will then apply to all tables, whatever the current database is. For example, we can have the following:
<?php
$db_url['default'] = 'mysql://user:password@dbserver/db';
$db_url['other'] = 'mysql://user2:password2@dbserver/other_db';
$db_prefix = 'drupal_';
?>What I suggest is to put a new level in the db_prefix array, to become like this:
<?php
$db_url['default'] = 'mysql://user:password@dbserver/db';
$db_url['other'] = 'mysql://user2:password2@dbserver/other_db';
$db_prefix['default']['default'] = 'drupal_'; // prefix all tables of my drupal database
$db_prefix['default']['users'] = ''; // don't prefix the users tables of my drupal database
$db_prefix['other']['default'] = 'other_';
?>This would allow a better abstraction.
Please let me know what do you think of such a feature.
Thanks,
David
#1
Could anyone please give me his opinion on this subject?
Thanks,
David
#2
Hi David - I'm also having this issue. At this point I've only just started delving in to the problem, but sounds like a sensible feature request to me.
Sam
#3
The multiple database connexion feature of Drupal is designed to allow module developers to query data outside of Drupal. In that case, the prefixing doesn't make much sense.
The best way to do this is for you to manually set $db_prefix after you db_set_active() call, and restore it afterwards.
#4
I'm posting this just as a simple example of how I hacked quickly round the problem. Might help someone else get on with the job.
<?php
function dbprefix_fix( $db_name )
{
global $db_prefix;
if( 'other_db' == $db_name )
{
$db_prefix = '';
db_set_active( 'other_db' );
}else{
$db_prefix = array(
'default' => 'surf_',
'users' => 'shared_',
'sessions' => 'shared_',
'role' => 'shared_',
'authmap' => 'shared_',
'sequences' => 'shared_',
'profile_fields' => 'shared_',
'profile_values' => 'shared_',
'users_roles' => 'shared_',
);
db_set_active( 'default' );
}
}
?>
#5
Agreed, table prefixes should be per database connection.
The DB:TNG patch #225450: Database Layer: The Next Generation is about to land, and will make this easy. Postponed until that.
#6
#7
DBTNG is in!
I am very much in favor of this. The way I'd suggest implementing it is to make 'prefix' another key in the $databases array:
<?php$databases['default']['default'] = array(
'type' => 'mysql',
'host' => 'locahost',
// ...
'prefix' => array('default' => 'foo_')
);
?>
The tricky part is that some modules do some really funky stuff with prefixing, including simpletest. So you'll have to be careful that simpletest doesn't break. Of course, simpletest may be able to work even better now if it can just establish its own database connection that defines its own prefixes... :-)
#8
Here is a full patch for this.
What this does:
- remove $db_prefix an re-implement it as a per connection 'prefix' key in the $databases array.
- add DatabaseConnection::(get|set)Prefix()
- add Database::renameConnection() and Database::removeConnection()
- change SimpleTest logic to take advantage of the new API:
- in setUp() : SimpleTest renames the default connection, then clones its parameters to create a new default connection with a prefix
- in tearDown() : SimpleTest removes the prefixed connection and rename the old default connection back
- moved simpletest prefix detection in conf_init()
- clean-up *a lot* install.php, by removing several unneeded references to $db_url (!!) and $db_prefix
This allowed me to find a bug in the database layer (#329080: Statics in objects are not per-instance). The fix in that other issue is required to make this work.
#9
Visual inspection only, as I just reviewed the linked patch and CNW'ed it. :-)
- I'm not sure we need to give prefix its own array/string. Why not tell each connection its entire DB info structure and let it use that?
- Hooray for removing the evil regex hack from openConnection()! :-)
-
if (preg_match("/^simpletest\d+/", $prefix = Database::getActiveConnection()->getPrefix())) {If the prefix is an array, that's going to break in exciting ways, isn't it?- For performance, since we're pre-processing the info array anyway, how about we pre-process a string prefix into an array with one key, default? That should save us some is_array() calls later, and an array_key_exists(), probably. (Just assume that default always exists, and pre-set it to '' if not specified.)
- DatabaseTestCase::setUp() has some error logging code in it. Probably OK for now, but let's remember to remove it before committing.
And looking at this patch, I realized that with addConnection() we can now remove references to the global $databases array from the database API entirely and just assign the connections from $databases in conf_init(). Sweet! Probably a separate patch, though. :-)
#10
Rerolled and revamped version. Should take care of every remark from #9. Needs a complete review.
#11
The last submitted patch failed testing.
#12
Hmm...it appears that automatic testing of patches that change the db installer is a big fail. I've run across the same situation in #346494: DB drivers need to be able to change the configure database form during install.
#13
Visual inspection of #10:
- There's a lot of git flotsam at the top of the file. Not sure if that's going to cause problems when applying or not.
- DatabaseConnection: "The default prefix used by that database connection". "This", not "That", since it's describing the self-same object. Ibid for non-default prefixes.
- Er, I don't get why we have default and non-default prefixes as separate properties? OK, I get it when I read down as far as setPrefix, but it should probably be a bit better documented.
- Ooo, prefixTables() gets so much shorter! :-)
<?php$this->setPrefix($connection_options['prefix'] ? $connection_options['prefix'] : '');
?>
That will throw a notice if $connection_options['prefix'] is not set at all. It needs an isset() or empty().
- There's a lot of changes in DrupalWebTestCase::tearDown() that don't look related to the DB prefixing at all. Are those really necessary here?
- The code removed from tearDown() includes the code that removes all of the simpletest-specific tables. I don't see that code replaced anywhere else, though. Simply removing the connection doesn't delete the tables (I would hope :-) ). So won't that leave a lot of testing tables lying about?
- The examples in default.settings.php don't quite make it clear that they're talking about the prefix key of a $databases entry. Perhaps expand the default samples of that, too?
Exciting!
#14
Thanks for the review, Crell.
It's perfectly innocuous, and quite cool ;) This is really not the first GIT patch I submit here ;)
Changed that to this, and added a bit of documentation to defaultPrefix.
Oups.
This is an artifact of trying to reroll a three-months old patch ;) I fixed those. When ignoring whitespace changes, the only differences are:
<?php
@@ -899,8 +907,8 @@ class DrupalWebTestCase {
* and reset the database prefix.
*/
protected function tearDown() {
- global $db_prefix, $user;
- if (preg_match('/simpletest\d+/', $db_prefix)) {
+ global $user;
+
// Delete temporary files directory and reset files directory path.
file_unmanaged_delete_recursive(file_directory_path());
variable_set('file_directory_path', $this->originalFileDirectory);
@@ -912,8 +920,9 @@ class DrupalWebTestCase {
db_drop_table($ret, $name);
}
- // Return the database prefix to the original.
- $db_prefix = $this->originalPrefix;
+ // Get back to the original connection.
+ Database::removeConnection('default');
+ Database::renameConnection('simpletest_original_default', 'default');
// Return the user to the original one.
$user = $this->originalUser;
@@ -937,7 +946,6 @@ class DrupalWebTestCase {
// Close the CURL handler.
$this->curlClose();
- }
}
?>
I extended them a bit and added a 'prefix' to most of the example $databases.
#15
The last submitted patch failed testing.
#16
Rerolled (some conflicts since chx' update.php patch went in).
#17
The last submitted patch failed testing.
#18
subscribing. #140860: Consistent table names and database handling of table names will be directly related to this
#19
@DamZ, could you re-roll this patch, we should get this into D7 before freeze. Its almost a bug.
#20
No almost about it. It's a bug that doesn't crop up very often. :-)
Last I heard, though, this was stalled on PIFR 2 work because this breaks automated testing. Automated testing needed to get better first before we could implement this. I don't know what the status of that is, though.
#21
dww has been busy with other things, so PIFR 2 won't be launched until he can setup some time for it. Last time we talked it was going to be second half of September sometime. Of course that means coordinating with DamZ to upgrade clients, Jimmy to do the testing master, etc.
webchick also wanted to wait until the code freeze was in place in case PIFR 2 upgrade broke things and made contributors unhappy.
#22
I consider this to be critical for the release. Could someone try to reroll the patch? I think tic2000 started to work on it, but he might have get lost in the middle.
#23
I would like to see #302327: Support cross-schema/database prefixing like we claim to in before we get do this, otherwise we'll have to re-roll again any how.
#24
I see no need to allow prefixing on anything but the default connection (and, of course, its slaves for correctness). If a site uses additional, separate connections, I don't see why it would need prefixes on those connections. Every time I've used extra connections, it's been for custom development/migration/legacy access purposes.
Could we substantially simplify things by only applying prefixes to the default connection?
#25
I don't think so.
1) The default connection is not really treated special right now, except by being, um, default. We'd have to hard-code in more magic behavior for one specific connection, which I'd prefer to not do.
2) One of the main reasons I want to make prefixes per connection is that it can simplify the simpletest code that handles testing. Rather than playing regex games with the global $db_prefix, simpletest could simply duplicate the active connection, set the simpletest DB prefix on the cloned connection, and make that the active connection. Then it can do whatever it needs to do, test code runs against the test DB, but simpletest still has access to the host database and can do all kinds of fun logging and stuff there.
#26
@Crell Makes sense. Time to reroll (which I'm handling now)!
#27
Fresh reroll, but completely untested.
#28
Tagging.
#29
The last submitted patch failed testing.
#30
This installs OK, but SimpleTest is giving a 500 error when I attempt to run it.
#31
Here's an updated patch with work from me and chx. Not perfect yet.
#32
Actually, the failures we got may have been related to the queue infinite loop.
#33
The last submitted patch failed testing.
#34
I really want #604138: features.node.inc doesn't check for array from hook_node_info in first, otherwise it will make the re-roll a real hassle.