Issue status update for http://drupal.org/node/29910 Post a follow up: http://drupal.org/project/comments/add/29910 Project: Drupal Version: cvs Component: database system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: nsk Updated by: Souvent22 Status: patch (code needs review) I vote for the 'or' method. Allows for the expansoin to other DB's if needed (not that drupal has any plans to support anything other than mysql/postgres). 'mysql' || 'mysqli' Souvent22 Previous comments: ------------------------------------------------------------------------ Mon, 29 Aug 2005 06:59:53 +0000 : nsk Attachment: http://drupal.org/files/issues/nskpatch.tar.bz2 (4.14 KB) I added MySQLi support to Drupal by adding a database.mysqli.inc file. After chx's suggestion I also removed $row from db_result from both mysqli and mysql files. Users are supposed to connect to Drupal with a mysqli "URL" in their settings.php. Just adding an "i" in the mysql entry in settings.php would work. MySQLi is the new MySQL client in PHP which connects to MySQL 4.1, 5.0 and beyond. MySQLi replaces the older MySQL library which was popular with MySQL 3.23 and 4.0. http://gr.php.net/manual/en/ref.mysqli.php The older MySQL client does not support full functionality with MySQL 4.1, that's why MySQLi is important. MySQLi works in PHP 4.1.3 or above, and is more popular in PHP 5. MySQLi means MySQL Improved. This patch should be included in core. ------------------------------------------------------------------------ Mon, 29 Aug 2005 07:08:07 +0000 : nsk Attachment: http://drupal.org/files/issues/nskpatch1.diff (7.1 KB) Here is the diff for mysqli against Drupal CVS HEAD. ------------------------------------------------------------------------ Mon, 29 Aug 2005 07:08:56 +0000 : nsk Attachment: http://drupal.org/files/issues/nskpatch2.diff (645 bytes) Here is the diff for mysql against DRUPAL CVS HEAD. The change was proposed by chx on IRC. ------------------------------------------------------------------------ Mon, 29 Aug 2005 07:13:28 +0000 : nsk With this message hereby I release all my changes to the files in this patch (database.mysqli.inc and database.mysql.inc) in GPL 2 (and later versions), as well as releasing them into the public domain. The changes I made are show in the diffs. An external URL where users can find some documentation is: http://jnanabase.org/index.php/User:NSK/Software/drupal_and_mysqli ------------------------------------------------------------------------ Mon, 29 Aug 2005 13:22:42 +0000 : Thomas Ilsche big +1, however not tested yet (will do later today) ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:03:33 +0000 : Thox See duplicate issue: #24264 [1]. [1] http://drupal.org/node/24264 ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:16:17 +0000 : m3avrck Using that file, I get this error right away: Parse error: syntax error, unexpected ',' in C:\Documents and Settings\tserbinski\My Documents\websites\drupal_cvs\drupal\includes\database.mysqli.inc on line 34 Doing some digging through the documentation, it looks like the mysqli_real_connect() [2] is being used *improplerly*. This function call works with no problems: @mysqli_real_connect($connection, $url['host'], $url['user'], $url['pass'], (substr($url['path'], 1)), $url['port'], null, MYSQLI_CLIENT_FOUND_ROWS); If this is indeed the correct call (it appears to be but can't really test whether the ports, sockets part is working), then the code circa line 27 that checks for the port can be deleted since the function call handles this. Additionally, the database check on line 51 needs to be moved up since the database is selected during the mysqli_real_connect() now. Overall, code needs some work and cleanup. Peformance wise, noticed a 10ms decrease in page generation times for a few pages (not thorougly tested performance wise however, just a quick and dirty observation). [2] http://us2.php.net/manual/en/function.mysqli-real-connect.php ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:17:34 +0000 : m3avrck Forgot to mention tested this with PHP5.0.3 and MySQL 4.1.11 on Windows 2003 SP1. ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:20:36 +0000 : m3avrck Thox, the reported duplicate issue is using an out of date database.mysql.inc file. I believe Drumm added quite a bit of code for friendlier errors. Seems those two files need to be merged for proper MySQLi support. I'll see what I can do later today if I have some time. ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:56:24 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database.mysqli_2.inc (8.51 KB) Ok I've taken the latest HEAD version of database.mysql.inc, merged this with NSK's recommendations, and then merged that with Thox's database.mysqli.inc (see link a few comments up) whose database code was based on out of an out of date version of the include file. Also went through and tweaked and it looks like this patch is ready to go. Just tested it here and it is running *now* on average, 40ms *faster* than the regular mysql library. Patch needs some more testing but I believe it is ready to go. Optionally, lines 27 can be removed and the mysqli_real_connect() can be updated with the $url['port']. No way to confirm if this will work 100% but we can get rid of some extraneous code if someone can confirm. See attached file. ------------------------------------------------------------------------ Mon, 29 Aug 2005 17:01:57 +0000 : m3avrck Updated status of feature request. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:38:18 +0000 : Thomas Ilsche Attachment: http://drupal.org/files/issues/database.mysql.inc.file (8.83 KB) Tested the file -> however des not work with HEAD due to [1]. However with a slight change it works well. I have attached the changed one (renamed so the content is not htmlscrambled) [1] http://drupal.org/node/22911 ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:45:53 +0000 : m3avrck Thomas this looks like the regular mysql file... nothing changed in this. Attached wrong file I am thinking? ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:57:03 +0000 : Thomas Ilsche Attachment: http://drupal.org/files/issues/database.mysqli.inc.file (8.71 KB) your right. sorry, wrong file. ------------------------------------------------------------------------ Mon, 29 Aug 2005 21:14:17 +0000 : m3avrck Great catch, working perfect now with latest HEAD. Any more testers, I think this code is just about ready to submitted to core. ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:22:49 +0000 : kbahey
From the performance point of view, MySQLi can be slower or faster than regular MySQL, depending on how ti is used.
According to this benchmark [3], using mysqli_stmt() has a performance edge over mysql_query(), which is in turn faster than mysqli_query(). NSK or Thomas can take a look and decide if it is worth so to do it. [3] http://www.johnjawed.com/benchmarks/ ------------------------------------------------------------------------ Sat, 03 Sep 2005 19:14:42 +0000 : nsk Yes I agree mysqli_stmt() is faster, if I have time I'll implement this later. ------------------------------------------------------------------------ Mon, 05 Sep 2005 15:56:59 +0000 : moshe weitzman tested - works for me. ------------------------------------------------------------------------ Mon, 05 Sep 2005 15:58:04 +0000 : moshe weitzman oops. set to proper status ------------------------------------------------------------------------ Mon, 05 Sep 2005 15:59:48 +0000 : m3avrck Although I do agree this code is working, we should in fact rewrite it to make user of the more advanced MySQLi techniques, notably mysqli_stmt() for *true* performance gains. Otherwise this patch doesn't really offer any incentives. ------------------------------------------------------------------------ Mon, 05 Sep 2005 16:18:04 +0000 : moshe weitzman one step at a time, folks. prepared statements is a much more complex undertaking. there are benefits without using that feature. i quote from http://www.zend.com/php5/articles/php5-mysqli.php: ----------------------- Beyond gaining access to the new features of MySQL 4.1+, why would anyone want to switch to using ext/mysqli? In addition to the functionality mentioned above, ext/mysqli also has some other serious benefits: * Greater speed. Enhancements in both the extension and in MySQL have made most operations faster, with certain operations becoming up to 40 times faster as compared to ext/mysql. * Better security. In older versions of the MySQL RDBMS, the possibility existed for an attacker to extract weak password hashes from the network and then recreate a user's password. The new authentication procedure is much more robust and mirrors the attack-resistant authentication procedure of tools like SSH. ------------------------------------------------------------------------ Tue, 06 Sep 2005 14:19:20 +0000 : m3avrck very true. i guess this patch then is indeed a good "first" step to get the initial, working functionality into core. then after that though, we do need to revisit the issue and update code to make better use of mysqli_stmt(), which will not only provide better performance, but much *better* security, and we could probably start to get rid of redudant code elsewhere in the code for such checks. ------------------------------------------------------------------------ Tue, 06 Sep 2005 18:54:16 +0000 : Dries I spent 30 minutes reading about mysqli and it looks nice, although it is not clear whether the old (or current) mysql extension will continue to evolve, or whether that is going to be deprecated. Anything about this in the PHP roadmap? Such information would help value the importance of this patch. As pointed out in the comments, the current Drupal mysqli backend has no advantage over the Drupal mysql backend. To me, it is not clear what potential performance improvements we are looking at or how this would affect our database API. Also, is any of this ANSI SQL? How is this going to affect the PostgreSQL backend/support? I'd like to see more discussion, before adding this to core. At this point, I don't think the code is ready to be committed simply because it's not clear how we'll go forward afterwards. While working, Let's try to outline what is ahead. ------------------------------------------------------------------------ Tue, 06 Sep 2005 19:58:37 +0000 : nsk The question is not whether mysqli is better than mysql. The issue is to make Drupal work with mysqli because some people use mysqli instead of mysql. For example, I could not install Drupal on my home test server because I only use mysqli here. It's possible to have both mysqli and mysql installed on the same PHP installation, but Drupal should not force people to install mysql. mysqli is used for MySQL 4.1 and 5.0, while mysql is used for MySQL 3.23-4.0. ------------------------------------------------------------------------ Tue, 06 Sep 2005 20:06:19 +0000 : Cvbge As I understand this is just another database type, like mysql or pgsql. So you can use $db_url = 'mysqli://blahblah' and it implements normal db_* functions, am I right? ------------------------------------------------------------------------ Tue, 06 Sep 2005 20:28:14 +0000 : m3avrck No, MySQLi is the *new* version of MySQL that is incorporated by default with PHP5. It cannot use the same MySQL functions because a lot has changed from 4.0 to 4.1 and this is future upgradable with the forthcoming 5.0 as well. It is my best guess that future versions of PHP will deprecate the current MySQL functions as the new MySQL 4.1 (and soon 5.0) becomes more and more widespread (both of which work with MySQLi only). ------------------------------------------------------------------------ Tue, 06 Sep 2005 20:51:55 +0000 : Cvbge I don't understand, can I use it as $db_url = 'mysqli://blah' or not? ------------------------------------------------------------------------ Tue, 06 Sep 2005 20:56:31 +0000 : m3avrck Yes you can use that to connect to a MySQL 4.1 database, *however* the code *will* be broken. regular mysql_ functions won't work, they to be mysqli_. Sorry for the confusion. ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:56:00 +0000 : Dries maverick, can you point us to additional information that states that mysqli is the default mysql extension for PHP5? Anything put forward by the PHP team on the issue of mysql versus mysqli? Such information is not critical, but I'd like to get good understanding of the situation. ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:58:03 +0000 : Dries Some extra insights from Goba: http://lists.drupal.org/archives/drupal-docs/2005-08/msg00076.html. ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:08:28 +0000 : Cvbge Either I don't uderstand what is this about, or you do not understand my questions. Why mysqli support can't be made the same way as mysql and pgsql? They are all php extensions and might be (un)available. ------------------------------------------------------------------------ Wed, 07 Sep 2005 08:27:31 +0000 : Goba Dries, what we are looking forward with mysqli instead of mysql is: * You can only connect to a mysql 4.x or 5.x database with mysqli, unless you use old_password() to set the MySQL user password. The new password encoding in MySQL 4+.x is more secure, this is why it is pushed forward. * People expect mysqli to be faster when dealing with mysql 4.x+, since it uses a new API (might use speedier stuff not supported by previous database clients). This is just something people assume, it would be nice to benchmark. * The real plus in using Drupal with MySQL 4.1.x+ is that it finally supports real uft8 text handling [4], ie. sorting, searching, etc. This is IMHO where Drupal is looking forward. I don't know whether the old mysql client in PHP can utilize the full potential of the new charset features, since these might not be client dependent. So the immediate pluses in MySQL 4.x+ are more secure passwords, real utf8 support and perceived faster operation. [4] http://dev.mysql.com/doc/mysql/en/charset-unicode.html ------------------------------------------------------------------------ Wed, 07 Sep 2005 10:46:41 +0000 : nsk mysqli is not a new database, it's a new database client for MySQL 4.1/5.0 which is installed by default in PHP 5 instead of mysql (the old client not updated anymore). We should not really discuss whether mysqli is faster or slower. We should instead focus on the real issue: Users with MySQL4.1/5.0 and PHP5 urgently need mysqli support in Drupal, so that they won't have to load the old mysql extension. PgSQL has nothing to do with mysqli. We talk about PHP extensions here, not databases. It is possible to get both mysqli and mysql compiled into PHP 5 at the same time, but it's isn't easy (many people get errors and can't compile). The patch works by "emulating" mysqli as another database type so you could connect to it by using a mysqli "URL" in settings.php. That was just what chx recommended me to do, and some people in IRC didn't like my initial idea to use "if" checks (which is how I have implemented mysqli support on my software, and it even recognises what client the user has installed on-the-fly without any need for configuration, I have posted an example here [5], it's really easy). I think we should focus on making Drupal 4.7 compatible with mysqli, so that developers won't be forced to load the old mysql if they don't want to, and later we can see how we can better utilise mysqli's new features and speed it up. [5] http://jnanabase.org/index.php/MySQLi ------------------------------------------------------------------------ Wed, 07 Sep 2005 10:55:39 +0000 : Dries Ok. Thanks for the additional information. From the looks we need mysqli support in Drupal 4.7. So let's focus on the technical aspects first. Is anyone opposed to introducing a new backend, or do some people prefer to add checks in the existing mysql backend? We should be able to commit this shortly. ------------------------------------------------------------------------ Wed, 07 Sep 2005 10:59:26 +0000 : killes@www.drop.org I'd prefer to have it as another backend. reason: Drupal might be moving to mysql 4 and we might want a mysql3 backend or Drupal continues to support mysql 3 and somebody wants a mysql4 backend (not that it would help too much). Also, a new backend is much cleaner and you avoid a lot of if-thens in the course of executing a page. ------------------------------------------------------------------------ Wed, 07 Sep 2005 11:01:34 +0000 : Goba Separate backend +1. It is much easier to maintain as well as it is much more performant (ie there is no need to load and continually skip unused code). ------------------------------------------------------------------------ Wed, 07 Sep 2005 11:05:55 +0000 : nsk if you use "if" checks, you can make it work without intervention from the administrator: Drupal could automatically check whether mysqli is available and use it on-the-fly, and if it isn't loaded it would fallback to mysql. With a separate mysqli included file, the administrator would have to specifically require their Drupal installation to use mysqli. This could cause some confusion for people who upgrade from php4 to php5 and their compilation only provides mysqli in php5, they may not even know about mysqli and wonder why Drupal doesn't work after they upgraded to php5, until someone informs them that they have either to recompile with mysql or change their settings.php in Drupal. Good documentation could help with this, of course. ------------------------------------------------------------------------ Wed, 07 Sep 2005 11:07:00 +0000 : chx It's simply not a question to have a separate backend or not. It's absolutely non-drupal to have two backends in one .inc. ------------------------------------------------------------------------ Wed, 07 Sep 2005 11:22:59 +0000 : jadwigo "With a separate mysqli included file, the administrator would have to specifically require their Drupal installation to use mysqli. This could cause some confusion for people who upgrade from php4 to php5 and their compilation only provides mysqli in php5, they may not even know about mysqli and wonder why Drupal doesn't work after they upgraded to php5, until someone informs them that they have either to recompile with mysql or change their settings.php in Drupal. Good documentation could help with this, of course. " So we only need to inform them... IMO it's important enough that if you're confused as a user you should be, because then you start wondering what has changed. If you maintain a site and upgrade essential tools like mysql and php it's only normal to know what is going on. For users who don't or can't maintain their own servers we should provide some information on the system requirements page [6] and in the install.txt [6] http://drupal.org/node/270 ------------------------------------------------------------------------ Wed, 07 Sep 2005 11:31:58 +0000 : stefan nagtegaal Well, we can include an 'compatibity_test.php' script inside the main drupal distro, which checks if all things which are required are present? (PHP-version, compiled with MySQL, etc) And after that does the same test for optional requirements like GD, Mod_Rewrite, mbstring, etc... How about that? Stefan ------------------------------------------------------------------------ Wed, 07 Sep 2005 13:24:48 +0000 : nsk The idea to have a dependencies check script is great. ------------------------------------------------------------------------ Wed, 07 Sep 2005 13:47:24 +0000 : m3avrck Now correct me if I'm wrong, but can't we just stick to how the original patch is working, e.g, create a new file database.mysqli.inc and then just change the $db_url string in settings.php so it knows to pickup up mysqli instead of mysql? This seems to be working great. As of a side note, on Windows, it is *much* easier to run the MySQL and MySQLi extensions in parallel with MySQL 4.1 ... that is how I'm currently running my dev machine and I can switch back and forth between regular database.mysql.inc and database.mysqli.inc just by changing the $db_url in settings.php. It would be my best recommendation to keep this approach for a few Drupal release cycles (say up to 5.1/5.2) and then deprecate the database.mysql.inc file and then eventually remove it as PHP5.1 (forthcoming) and MySQL 4.1 become dominant default installs. ------------------------------------------------------------------------ Wed, 07 Sep 2005 13:49:31 +0000 : Goba compatibity_test.php or something along the lines is going to be part of the install system anyway, as far as I know. ------------------------------------------------------------------------ Wed, 07 Sep 2005 14:10:26 +0000 : moshe weitzman I prefer separate backend too. I think the auto-switching between mysql and mysqli is great and easy to implement. Just create an if() block right before we include database.x.inc. That happens in db_set_active() [line 112 of includes/database.inc]. The if should look like: <?php // automatically use mysqli if loaded. it is more secure: <a href="http://www.php.net/mysqli">http://www.php.net/mysqli</a> if ($db_type == 'mysql' && extension_loaded("mysqli")) { $db_type == 'mysqli'; } ?> Sorry, I can't reroll the patch and re-test at the moment. ------------------------------------------------------------------------ Wed, 07 Sep 2005 14:37:57 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_4.patch (1.94 KB) Ok here is a patch that fixes some references to mysql and adds mysqli as well. ------------------------------------------------------------------------ Wed, 07 Sep 2005 14:40:40 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database.mysqli_3.inc (9 KB) This is the patch from comment #13 that has been tested and working great on WinXP with PHP 5.0.4 and MySQL 4.1.11. Sorry couldn't get both of these patches to roll into one. Also, I see no *reason* to include code to auto detect MySQLi over MySQL unless we add support to do this for PostgreSQL as wel. Otherwise that isn't consistent. It is clear in the settings.php that either mysql, mysqli, or pgsql can be used to setup a database. Easy enough. ------------------------------------------------------------------------ Wed, 07 Sep 2005 14:45:35 +0000 : m3avrck Correction, that last file should be placed in includes/ as database.mysqli.inc. Then just edit $db_url in settings.php to make use of it and switch from MySQL to MySQLi support. ------------------------------------------------------------------------ Wed, 07 Sep 2005 15:27:51 +0000 : m3avrck Further testing does show one *big* break in Drupal, updates.inc. Updates.inc is testing to see if ($GLOBALS['db_type'] == 'mysql'). If you set the the $db_url to mysqli, *no* updates work. The code will be the same for mysql and mysqli, so I think $GLOBALS['db_type'] needs to be ammended so that it accounts for both mysql and mysqli connections (since they are essentially the same in respect to SQL code). ------------------------------------------------------------------------ Wed, 07 Sep 2005 15:51:15 +0000 : chx if (substr($GLOBALS['db_type'], 0, 5) == 'mysql') big problem fixed. ------------------------------------------------------------------------ Wed, 07 Sep 2005 15:54:58 +0000 : Goba +1 on chx's big problem fix :) ------------------------------------------------------------------------ Wed, 07 Sep 2005 16:24:55 +0000 : m3avrck +1 to that fix as well :) Just need to make note of that in any furture database updates after MySQLi support is added. ------------------------------------------------------------------------ Wed, 07 Sep 2005 20:11:17 +0000 : Dries Please add a helper function for that to the update script. Something like using_mysql() or so. I'll commit this patch as soon update script is taken care of. Great work. ------------------------------------------------------------------------ Wed, 07 Sep 2005 21:41:38 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_7.patch (14.46 KB) Ok here is an updated patch that patches updates.inc as well. I put a check at the top that assigns $GLOBALS['db_type'] to it's only local var $db_type and then uses chx's above recommendation to make the fix. Works great! Use this patch in conjuctions with the above database.mysqli.inc file! ------------------------------------------------------------------------ Wed, 07 Sep 2005 21:48:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_8.patch (14.45 KB) Fixed a typo. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:42:33 +0000 : Dries I don't see how you can use global variables like that; $db_type will be undefined in the update functions. db_connect() should not return false -- the existing db_connect() functions appear to do their own error handling/reporting. Maybe the PHPdoc needs to be updated. (On a related note, there appears to be quite a bit of duplicated code in the error handling. A couple weeks ago, when the Drupal database server was down, these database error handling kicked in and we were sharing quite a bit of information -- like the hostname/IP of the database server -- with the rest of the world. Not sure that is a good idea.) ------------------------------------------------------------------------ Thu, 08 Sep 2005 21:34:17 +0000 : m3avrck Don't see the problem with $db_type. Only reason I did that in updates.inc is to reduce redunancy. Other option would be to do: if (($GLOBALS['db_type'] == 'mysql') || ($GLOBALS['db_type'] == 'mysqli')){ That is just so the correct SQL is used. Otherwise, the rest of the time $db_type needs to be *mysqli* and not just *mysql*. Make sense? Comments on which approach is better? ------------------------------------------------------------------------ Fri, 09 Sep 2005 06:35:05 +0000 : Dries Add a print "DB TYPE = $db_type"; to one of the update functions and see what happens ... ------------------------------------------------------------------------ Fri, 09 Sep 2005 17:16:45 +0000 : m3avrck Ok well in order to get updates.inc to work correctly, I've come up with a few simple solutions instead of adding the overhead of another function. *Option 1* - simple and practical if (($GLOBALS['db_type'] == 'mysql') || ($GLOBALS['db_type'] == 'mysqli')) *Option 2* - less overall code, more intial work to updates.inc. Basically invert the logic so instead of purely testing for MySQL we test for PostgreSQL and then if it isn't that, we assume it is either MySQL or MySQLi so the else catches both of these without having to test for each one like above. if ($GLOBALS['db_type'] != 'pgsql') *Option 3* - better structure, future support of other databases switch ($GLOBALS['db_type']): case 'pgsql': //PostgreSQL here break; default: //MySQL stuff here for both MySQL and MySQL So what do you guys think is the best method? I'll write a new patch that fixes update.inc based on this so it will properly support MySQLi and I'll fix that include for Dries comments above. Thanks! ------------------------------------------------------------------------ Fri, 09 Sep 2005 17:23:07 +0000 : killes@www.drop.org I prefer version 3 as it makes less hassle for adding more databases in the future.