Table prefixes should be per database connection

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

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.

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.

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' );
  }
}

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.

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:

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

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.

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

Rerolled and revamped version. Should take care of every remark from #9. Needs a complete review.

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.

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! :-)

$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!

Thanks for the review, Crell.

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

It's perfectly innocuous, and quite cool ;) This is really not the first GIT patch I submit here ;)

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

Changed that to this, and added a bit of documentation to defaultPrefix.

- Ooo, prefixTables() gets so much shorter! :-)

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

Oups.

- 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?

This is an artifact of trying to reroll a three-months old patch ;) I fixed those. When ignoring whitespace changes, the only differences are:

@@ -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();
-    }
   }

- 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?

I extended them a bit and added a 'prefix' to most of the example $databases.

Rerolled (some conflicts since chx' update.php patch went in).

subscribing. #140860: Consistent table names and database handling of table names will be directly related to this

@DamZ, could you re-roll this patch, we should get this into D7 before freeze. Its almost a bug.

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.

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.

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.

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.

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?

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.

@Crell Makes sense. Time to reroll (which I'm handling now)!

Fresh reroll, but completely untested.

Tagging.

This installs OK, but SimpleTest is giving a 500 error when I attempt to run it.

Here's an updated patch with work from me and chx. Not perfect yet.

Actually, the failures we got may have been related to the queue infinite loop.

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.

Hi, I just ran into the problem that this patch is supposed to fix. Is this still in the running for D7, or is it too late now?

This was blocked for a long time by PIFR2, because this changes the way simpletest works. I doubt that webchick would allow it as an exception at this point, but we'd have to ask her.

I don't understand why this is critical -- it sounds a bit like an edge case to me. If someone can argument why this is important, that would help!

Dries:

Currently there are two problems:

1) Database prefixes are global. That is, they apply to all database connections. In practice, if you're using a connection other than the main Drupal database it's not a Drupal database, and therefore you don't want prefixes. It's a legacy database of some kind and you don't get to control table names. So in practice, you can *either* use DB prefixing on a single DB *or* you can use connections to multiple databases, but in practice doing both is infeasible in nearly all cases.

2) Simpletest, in order to use the host database for the transient test installs, does all sorts of evil things to the database prefix. It maintains the same DB connection as the host install, but screws around with the prefixes in order to trick unit tests and the code they call into writing into the prefixed tables. That is ugly, hacky, and prone to breakage.

Neither of those was solvable with the old DB API.

The solution to both problems is to move the prefix definition from a global $db_prefix variable to a part of the $databases array for each connection. That is, every connection can have its own prefix definitions that are whatever it wants.

That solves the above problems this way:

1) You can now set a prefix array for just the main Drupal database, and then add other connections to other databases of whatever type, driver, and origin. The prefixing won't affect those, so you can now use prefixing and multiple connections at once.

2) DBTNG makes cloning a database connection at runtime very easy, and makes manipulating two databases at once dead simple (whereas before it was a huge PITA). That means instead of playing screwy games with the prefix, SimpleTest could take the "default" connection, clone its definition, set a prefix on the cloned connection, and set that as the default. Then call unit tests. Those unit tests now use the cloned connection that has the prefix, but Simpletest itself still has the non-prefixed connection object. So it can write to the host database while unit tests are running. (Think much much much better debug capabilities.) When a given test run is done, delete all tables with that prefix, drop that connection, and repeat. Much cleaner, much less buggy, and more feature-full.

Unfortunately, messing with the way simpletest and the installer work break simpletest in a nice loop of meta fail. :-) So this issue has been stuck at "well this would be great but the testing framework won't ever pass it so we can't do it" for the better part of the year. In theory, what I last heard is that PIFR2 *should* make it possible to even write this patch and get the testing bot to pass it. Of course, PIFR2 didn't land until the other day, so this patch has been in "blocked on PIFR2 landing" since the summer, before the original code freeze date.

So now the question is, do we now try and see if this will get past PIFR2 without breaking because it changes the way simpletest works, or do we say "well that sucks that it was blocked by infrastructure issues for so long, I guess we'll see if those infra issues are fixed by the time we start Drupal 8 development".

That's a question only you and Angie could answer. :-)

My real-life use case (or edge case if you will):
I'm importing data from an existing site into a new Drupal site using Table Wizard and Migrate. (Table Wizard connects to the db's defined in settings.php and exposes the data to the Migrate module.) The old db has no less than 319 tables and is not prefixed. The Drupal db does have a prefix. Since I can only define one default prefix, I have two options:
1) set $db_prefix = ''; and manually set the prefix to 'drupal_' for all Drupal tables (this would probably break when installing new modules)
2) set $db_prefix = 'drupal_'; and manually set the prefix to '' for all external tables I want to import data from (currently my best option, but a lot of work)

I noticed tonight that $db_prefix is a string while $databases is an array and thought, "Hmmm..." and found this issue again.

I would be okay with fixing this as long as it's done before the first alpha. I guess that's going to depend on Jimmy's availability. But PIFR2 is deployed now, so whatever was holding this back before should hopefully be addressed.

Downgrading from critical, though. It's not a regression. Would be handy for it to work this way, though. I've experienced marcvangend's conundrum, myself.

I guess this is past the deadline then?

Moving it to 8.x-dev... :(

It's a shame this got blocked so long, but we really need to fix the prefix handling, or our new shiny database layer will remain unusable to access external data...

Here is a partial reroll. Will probably not pass.

Damien, thanks for picking this up. If this could still go into D7, that would be great... but Webchick said "I would be okay with fixing this as long as it's done before the first alpha" so you might want to consult her before spending your time on it now.

This is a very important issue, as it is blocking a few things (especially, it is completely blocking the possibility of running tests concurrently on SQLite), but more importantly because our API is completely inconsistent without this.

On the other hand, this is a *very tiny* API change. Most of the modules will not be affected by this change. Only people establishing several connections to different databases will be (positively) affected.

I argue this could be D7 material. This new version of the patch should pass.

Getting there!

As a quick summary, this patch:

• Gets rid of the global $db_prefix, and replace it by a per-connection prefix array in the $database array;
• Sanitizes the way DrupalWebTestCase handles the connections to the parent and the child sites: we now have to separate connections, no more belly-dancing necessary in ->assert() to remove the prefix, log the assertion and put it back;
• Stores global information about the test being run, accessible by the drupal_test_info() function, to get rid of the multiple references to $_SERVER['HTTP_USER_AGENT'];
• Removes Simpletest-specific code from the database layer and put it back into _drupal_bootstrap_database().

The following related modifications were necessary:

• drupal_valid_test_ua() / drupal_generate_test_ua() now uses the $drupal_hash_salt variable as a key, because we cannot rely on the database array anymore (it is modified during the request);
• We introduce the DatabaseConnection::setPrefix() API function, that complements the already existing API functions for prefixes (prefixTables() and tablePrefix());
• We introduce the Database::renameConnection() and Database::removeConnection() API functions for dynamic manipulation of the connections.

This patch doesn't modify the database configuration page to allow PIFR to test it without modifications. We have worked (notably with rfay) on putting tools in place to improve the manageability of the test clients. The modification required to PIFR is a one-liner, and we can deploy it any time.

Thanks for your work, I'll try to give it a spin this afternoon.

I skimmed this patch and it looks sane to me. There is one TODO in form handling code that still needs attention.

It doesn't make sense to enforce a single database prefix across multiple independent databases and this patch takes care of it. Plus, as mentioned by DamZ, it opens up the door for concurrent testing.

I just took a quick test-drive with this patch and I couldn't break it.

The TODO in the code is there for PIFR to be able to test the patch without failing at the installation step. We might want to commit this patch as-is, and fix PIFR in a follow-up.

Fascinating. conf_init() with this patch is still importing $db_url from settings.php. We should remove that.

drupal_test_info() looks old and vestigial. It can probably be replaced with drupal_static().

The renameConnection() method has various conditions in which it will do nothing. In those cases it should indicate that it did nothing, either via an exception or a return. I think in this case a return TRUE for success and FALSE for fail is sufficient.

removeConnection() can probably have a return as as well. TRUE if the connection was removed, FALSE if it wasn't there to begin with?

What is with the TODO in install_settings_form_validate()?

That this passes is a miracle in itself, and it looks like the resulting code is a lot cleaner in most places as well. It also means that tests can access the "host" database by just specifying it's key, which should be helpful for debugging.

Some simple cleanup and I think this is ready to go. w00t!

- removed drupal_test_info() and use a drupal_static() instead. This patch is way old :)
- added return values for renameConnection() and removeConnection()
- clarified the TODO in install_settings_form_validate()
- moved the calls to setPrefix() to the abstract DatabaseConnection::__construct(), avoiding to call it in every driver

We cannot reliably use drupal_static() here because it gets flushed at a few different places. Reverted to use a simple global variable (and documented it).

#57 resolves the issues I had, and I don't see any new ones. OMG I can't believe this issue is finally RTBC!

Alright, let's get this in still. Committed to CVS HEAD. Thanks a lot.

Great. Thanks everyone!

There's an API change here; let's get that documented in the 6 => 7 upgrade guide please.

I opened this follow-up in the PIFR issue queue: #839996: Stop enforcing an empty prefix uselessly, after this is in and deployed, we will be able to remove the TODO introduced by this patch.

There seems to be a good issue summary in #49.

I'll announce this if somebody could please summarize the impact to the developer. I see that $db_prefix is gone. What other direct impacts are there?

I guess if you are Table Wizard, Views, or Data module, it affects you because there's a different way of accessing external databases when prefixes are present. SimpleTest was affected too. But if you're not any of these, it probably doesn't affect you at all.

If you don't think it's worth an announcement, we can just bump back to 'fixed'. I'd love a mention in the upgrade docs anyway though for those using db prefixes so they fix their settings.php file accordingly.

I've added a mention on the upgrade guide. I defer to rfay as to whether or not to announce this on the dev list as well. As webchick notes, the number of people affected by it is really small.

That said, I noticed that the update page is woefully out of date. It still lists Alpha-3 as the most recent release. :-( I defer to someone who has been tracking that better than I have, though.

Yay, we're done with this issue!

Not quite. This still needs to be removed from the globals file (it's in the contrib repository):
http://api.drupal.org/api/global/db_prefix/7

Fewer global variables in Drupal++

Sorry, we need that added back now, after #850206: Converting db configuration gives wrong result when $db_prefix is set.

I do not believe so. We only have this during the time the old settings.php file is updated.

Oh! Fair enough.