I have been testing provisioning hundreds of sites using Aegir (and feeds) and have run into an interesting issue.

When migrating sites Aegir will look for a valid database name to use, but it doesn't check properly, it uses the following method:

  function database_exists($name) {
    $result = $this->query("SHOW DATABASES LIKE '%s'", $name);
    if ($result) {
      return $result->fetchColumn(0);
    }
  }

But if the database name contains an underscore, then that is actually a single character wildcard, so you can run into the issue whereby there is no database with that exact name, but one that differs by a character in that position. This isn't really a huge issue, but the migration script explicitly adds underscores to database names, which can cause issues, as I've found.

So it can be changed to:

  function database_exists($name) {
    // An underscore in a LIKE clause is a single character wildcard, escape it.
    $name = str_replace('_', '\_', $name);
    $result = $this->query("SHOW DATABASES LIKE '%s'", $name);
    if ($result) {
      return $result->fetchColumn(0);
    }
  }

to fix the issue. Patch attached.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anarcat’s picture

Status: Active » Needs review

Patch seems like a good idea, but I wonder if it wouldn't be better to try to *connect* to the database directly? This way: a) we don't fool around with wildcards and b) we can more easily support other database systems...

At this point, I think we're running as MySQL root, so it should just work..

What do you think?

Steven Jones’s picture

Hmm...I don't really know enough to say what would be better!

Anonymous’s picture

I get what anarcat is saying (why waste time guessing when we can check, and be certain) - I'm only worried about the expense of doing a connection to each database as opposed to filtering a listing of them. Would it slow things down noticably? (I'm honestly not sure.)

Steven Jones’s picture

This method is in the mysql service anyway, so other DB systems just implement a different method if we're going after portability?

I say, just fix mysql with the quick and easy patch, and when we really do start to support different databases be prepared to change this code again to whatever is needed.

anarcat’s picture

Agreed, let's fix mysql first.

anarcat’s picture

Status: Needs review » Needs work

I committed your patch, now we should make this work so that it uses "connect()" or "USE" instead, to check the DB name existence.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
5.54 KB

Something like the attached. No idea if it works.

I've moved the method implementation into the pdo service, as it is probably usable by all.

anarcat’s picture

Status: Needs review » Needs work

That looks reasonable, in general, but:

1. you have way too many whitespace changes in there, it makes the patch hard to read, try -w or revert the whitespace before diffing
2. shouldn't this be done with unset() instead?

+      // Free the $conn memory.
+      $conn = NULL;

3. finally, I wonder why this is necessary....

+    $dsn = $this->dsn . ';dbname=' . $name;

I am puzzled to see that nowhere else do we do that kind of manipulation - how the heck do we choose the DB elsewhere? Maybe we just don't and that's the right way to do things...

Steven Jones’s picture

Version: » 6.x-1.1
Status: Needs work » Needs review
FileSize
1.64 KB

Much better patch attached.

To answer your comments, I do: $conn = NULL; because that's what provision does in provisionService_db_pdo::close(). Also, we don't do $dsn = $this->dsn . ';dbname=' . $name; elsewhere, because we never do operations within a DB in provision directly, we'll do them via a fully bootstrapped Drupal instead. We do it here though, because this forces the DB to be selected, if the DB doesn't exist the connection to it will fail.

anarcat’s picture

This patch looks much better, feel free to commit to 2.x and merge to 1.x after testing.

Steven Jones’s picture

Status: Needs review » Patch (to be ported)

Added to 7.x-2.x in 6e56f9e9efda9e34e186d89c6d0b16ab97f657b6.

anarcat’s picture

Status: Patch (to be ported) » Fixed

cherry-picked to 1.x.

Status: Fixed » Closed (fixed)

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

  • Commit ef36954 on debian, dev-migrate_aliases, prod-koumbit, dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-subdir-multiserver, 6.x-2.x-backports, dev-helmo-3.x by anarcat:
    #982452 - check for db existence correctly for dbs with underscorse
    
    
  • Commit 6e56f9e on 7.x-2.x, dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-subdir-multiserver, 6.x-2.x-backports, dev-helmo-3.x by Steven Jones:
    Issue #982452 by Steven Jones. Fixed DB existence checking.
    
    
  • Commit d3d91b5 on 6.x-1.x, dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-subdir-multiserver, 6.x-2.x-backports, dev-helmo-3.x authored by Steven Jones, committed by anarcat:
    Issue #982452 by Steven Jones. Fixed DB existence checking.