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.
Comment | File | Size | Author |
---|---|---|---|
#9 | aegir-db-exists-982452-9.patch | 1.64 KB | Steven Jones |
#7 | provision-982452.patch | 5.54 KB | Steven Jones |
aegir-db-underscores.patch | 542 bytes | Steven Jones |
Comments
Comment #1
anarcat CreditAttribution: anarcat commentedPatch 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?
Comment #2
Steven Jones CreditAttribution: Steven Jones commentedHmm...I don't really know enough to say what would be better!
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.)
Comment #4
Steven Jones CreditAttribution: Steven Jones commentedThis 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.
Comment #5
anarcat CreditAttribution: anarcat commentedAgreed, let's fix mysql first.
Comment #6
anarcat CreditAttribution: anarcat commentedI committed your patch, now we should make this work so that it uses "connect()" or "USE" instead, to check the DB name existence.
Comment #7
Steven Jones CreditAttribution: Steven Jones commentedSomething 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.
Comment #8
anarcat CreditAttribution: anarcat commentedThat 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?
3. finally, I wonder why this is necessary....
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...
Comment #9
Steven Jones CreditAttribution: Steven Jones commentedMuch better patch attached.
To answer your comments, I do:
$conn = NULL;
because that's what provision does inprovisionService_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.Comment #10
anarcat CreditAttribution: anarcat commentedThis patch looks much better, feel free to commit to 2.x and merge to 1.x after testing.
Comment #11
Steven Jones CreditAttribution: Steven Jones commentedAdded to 7.x-2.x in 6e56f9e9efda9e34e186d89c6d0b16ab97f657b6.
Comment #12
anarcat CreditAttribution: anarcat commentedcherry-picked to 1.x.