Allow both postgres and mysql database connections from within the same site.

Would love to have this in the core, but doesn't look like it's included yet, neither in 4.6.1 nor in the latest snapshot. Any idea yet if and when it'll be merged?

Oops, didn't intend to change the title of this bug report. Hereby corrected.

Looks like the added constants in common.inc are leftover from something else.

not a bug

Adrian's code appears to be in the 4.7 core files now, so I have closed this issue. There is still a problem with throwing a watchdog message when an alternate database is selected. See issue 42000.

I was wrong, wrong, wrong. Trying to tie all db_set_active() issues together too fast.

I just tried using both mysql & postgres with db_set_active() and I got error:

"Fatal error: Cannot redeclare db_connect() (previously declared in /var/www/html/thirdside/drupal/includes/database.mysql.inc:23) in /var/www/html/thirdside/drupal/includes/database.pgsql.inc on line 71"

Yes, that behaviour is not currently supported. That's why this is classified as a feature request. And feature requests are only ever applied against CVS, not a release.

Hey all!

I have applied the patch against 4.7.0, and am trying to use the module phpbb2drupal. However, I get the following:

Warning: mysql_real_escape_string(): supplied resource is not a valid MySQL-Link resource in drupal/includes/database.mysql.inc on line 349

repeatedly with some slight variation.

Is this because the module is badly formed? or does it represent a bug in the code?

tarek : )

Hey again all!

I believe that I have narrowed down the problem a little more.. After excluding some non-MySQL-friendly code, I now get:

warning: pg_connect(): Unable to connect to PostgreSQL server: FATAL: Password authentication failed for user "mysqluser" in drupal/includes/database.pgsql.inc on line 50.

The key here being that 'mysqluser' is supposed to go with a mysql URL.

I think, then, that my problem is related to this portion of code:

<?php
function db_invoke($function) {
  global $db_type;
  
  $args = func_get_args();
  $function = array_shift($args);

  if (function_exists('db_'. $db_type . '_' . $function)){
    // call Drupal function
    return call_user_func_array('db_'. $db_type . '_' . $function, $args);
  }
}
?>

Does the $db_type have to be set in some way by me? If so, perhaps it should auto-parse the db_url instead.

As it stands right now, this is the code being used to connect to the mysql:

<?php
    $db_type = "mysql";
    db_connect($db_url2['phpbb']);
?>

Thanks in advance for the assistance.

tarek : )

i'm doing a pgsql => mysql migration right nw and this patch is performing well. code looks sane also. RTBC.

If there would be a patch (code needs benchmark) I would have used that. This code will be executed a (few) hundred times on each and every Drupal page...

Glad there is such a patch!
I will benchmark that tomorrow and will post back the results.

Frankly, I think that this patch is a bit on the large side for a rarely used feature.

There has (surprise for me too) been enough interest to seriously consider this. I do think the approach fits with Drupal. Banchmarking is a good idea.

bookmarking because this issue is relevant for my phpbb2drupal.module.

lets resubmit this for next release

I wrote basically the same thing for 4.6 a couple months ago. Shame I didn't find this issue until now!

I didn't make a new hook though. I simply wrapped db_{$db_type}_foo() calls with the default db_foo() and used the global $db_type variable to call the correct function depending on the active connection.

I know it's a chore but if you would not use db_invoke and call_user_func_array but write $function($arg1, $arg2..) as needed (note that db_query has array syntax, so no variable number of arguments) then the speed loss would be actually small. variable function is slower but c_u_f_a is the slowest.

I don't mind the db('query', ...) approach.. instead of 14 db_query like wrapper functions. Although with the wrapper functions it's easier see exactly what the arguments are without diging too many levels down. (func_get_args type stuff is a real pain for readability when you're trying to grok some code).

Although maybe some database-specific functions require more arguments than others (can we get the Oracle / DB2 patch guys in on this discussion)?

Properly supporting more than one type of database at once, I think is pretty important to have since we are already so close.

Maybe benchmarks of the call_user_func vs variable function approach are needed to see how we specifically end up there. There was a page somewhere about how to run a good benchmark on Drupal wasn't there?

I have commented on the patch, sorry not on the further ideas of db(...).

If you want a db() then that probably would mean call_user_func_array and adding a hundred of that to a page is not a particularly good idea. You can bench this easily, change _db_query from a direct call to a call with call_user_func_array and let's see results.

This might be a case where it would be really helpful to have a database object. I eliminates a lot of ugly regex.

Regex? What regex? The only regex that appeared in the issue is one that Adrian mentioned that could be used to upgrade the code to his model. Noone wanted to add any regex to core here.

I ran some benchmarks on my laptop. Drupal core, vs vertice's db_invoke, vs using a generic wrapper with a variable function to call the db-specific implementation.

Testing environment...
Mac OS X 10.3.9
Drupal: 5
MySQL: 5.0.27
PHP: 4.4.4
Apache: 1.3.33

Vocabularies: 15
Terms: 250
Nodes: 2000
Comments: 8000

1. Core: 962.904 seconds
2. db_invoke: 971.670 seconds
3. variable function: 964.321 seconds

So, it looks like we could get full multi-database support by using variable functions without negatively affecting performance.

If the flexibility of Adrian's approach is desired, the impact doesn't seem to be huge, and there may be other places in the db layer to look for optimizations to offset that.

$func is a variable function. Rokerr's figures support my notion, I believe that call_user_func_array is too slow.

As the signature of db does not contain references it does not handle them but that's hardly a problem as we do not use references in the db layer.

So, I am eagerly waiting for Adrian's db function then we bench it and decide on it.

@33
I'm not sure what you mean by the variable function call? Did you just remove the db_invoke call and replace it with $function?

Would we still have to have all those wrapper function

Yes, I retained the wrapper calls like db_connect(...) -> $f = 'db_' . $db_type . '_connect'; return $f(...);

I don't really mind changing to db('connect' ...) instead, although the $arg1, $arg2 thing makes it really tough to figure out what arguments are useful in which context.

The only wrapper function I needed to use call_user_func_array in was db_query_range. (I think, there may have been one other).

Here's kind of a least-disruptive implementation, renaming db.*.inc functions to db_*_foo() and having the generic db_foo() functions in database.inc. And then, using the global $db_type and variable functions to do the work.

#22: 4 out of 58 hunks fail - needs to be re-rolled if there is still interest.

#48: Your db_version generic implementation needs parentheses after the $f to return the function result rather than the string, and the coding style for string concatenations is wrong.

I have re-rolled this patch from #48 for HEAD and tested it (briefly) with both MySQL and PostgresQL. Works fine. I think it is better to have a variety of short functions in database.inc than to apply changes all over core *and* require every module developer to do the same.

The failed hunks were due to db_column_exists() being added after the previous patch was created.

Note that the pgsql in system.install got broken in HEAD on 4/25/07; you have to replace "int(1)" with "int" in order to test this code. I'll submit a fix for that momentarily, but it does not affect this patch.

There is a pgsql system.install fix posted to http://drupal.org/node/139673 - I think it's the same bug you're referring to.

You meant to attach an updated patch, right?

Yes, I did. Sorry.

Maybe I'm missing something, but do we really gain anything by checking if the function exists? If the application is calling a db function it seems that the desired db layer implementation should at least have a stub function.

I don't think you are missing something. All the db functions are required. If a .inc file does not implement one, it is a critical bug that must be fixed so just crashing Drupal by calling an undefined function is acceptable (arguably preferrable to returning null which might hide the fact that the function is missing).

New patch attached.

I tested this on MySQL and couldn't find any bugs. Unfortunately I don't have access to mysqli/postgres to test on those.

I tested this with mysqli and postgres; no problems.

To review, the benefits of this patch are: 1. The ability to access multiple supported DBMS systems from within a a single Drupal process using the existing $db_url-as-array functionality. 2. A big step towards allowing DBMS backends as contrib modules; I'll be submitting a separate patch for the very small remaining step. 3. It is just ugly to define the same function names in multiple files.

Since Chris already reviewed the code for mysql and I just verified mysqli and postgres support, I'm marking RTBC.

Where are the benchmarks?

While making benchmarks I actually found a bug. system.install and system.module has code like ths:

if (function_exists('db_check_setup')) { db_check_setup(); }

Currently, only database.pgsql.inc defines db_check_setup(), and this works fine. With this patch, db_check_setup() is always defined as a dispatcher but database.mysql{,i}.inc did not define it. I added no-op functions.

A new patch is attached.

Benchmarks:

Intel Core2 6600, 2.4GHz
3.24GB RAM
Windows XP Pro
Apache 2.2.3 (Win32)
PostgreSQL 8.2
PHP 5

Drupal HEAD, 5/08/2007
1000 nodes, 5000 comments

test: ab.exe -n 100 -c 10 localhost/

without patch: 10.953s, 10.984s, 10.875s, 10.875s, 10.922s, avg: 10.922s
with patch: 11.625s, 11.000s, 11.141s, 11.109s, 11.000s, avg: 11.175s
difference: 0.253s
% difference: 2.32%

I confess to being surprised by these results. How can funcall+query > 102%*query?

If this is deemed unacceptable I'll look into what is causing the extra overhead (is it really one function call)? Can someone point me in the right direction for a PHP profiler?

You are calling a variable function. I would suggest rollng an --ugh-- patch which does a switch on known database types and bench that. You might be surprised.

see xdebug.org for the profiler, and wincachegrind or kcachegrind in order to visualize the results in pretty graphs.

Okay, I wrote a little "call a no-op function from within an empty loop 50,000,000 times" and now see that (1) variable-function calls are about 30% slower than direct calls and (2) PHP5 is about 100% slower than PHP4.

Please remind me why we are using this POS language.

Anyway, using a switch based on known database types eliminates a primary benefit of this patch: support for db backend contrib modules. Unless, of course, I provide a 'default' case that uses the variable function call. Then contrib modules will be slower, but will work.

Okay, new patch, using hard-coded function calls for known db types and a variable function call for unknown db types. New benchmarks:

Intel Core2 6600, 2.4GHz
3.24GB RAM
Windows XP Pro
Apache 2.2.3 (Win32)
PostgreSQL 8.2

Drupal HEAD, 5/08/2007
1000 nodes, 5000 comments

test: ab.exe -n 100 -c 10 localhost/

without patch: 12.48, 12.31, 12.23, 12.17, 12.18, 12.19, 12.34,
12.19, 12.23, 12.14
avg: 12.25

with patch: 12.33, 12.20, 12.44, 12.53, 12.30, 12.33, 12.23,
12.27, 12.34, 12.5
avg: 12.35

diff: 0.10
% diff: 0.82%

(The previous benchmarks were actually made with MySQL, not PostgresQL; apparently MySQL is faster at least on my system a few days ago.)

I told you willbe surprised. Now, it's up to others -- does this speed penalty worth the superb capabilities? I would vote 'yes' and here is why: there is a DDL layer coming and after that comes the avalanche of DB layers and I believe there are some interesting mixes with SQLite and MySQL. But even before that, this is a very, very long outstanding request...

I don't think it make sense to lose 1% of performance for a feature that will be used by less than 1% of users. To me its a solution in search of a problem. Can you describe a use-case for 2 db (postgre and mysql) in one site? I have to side with killes and drumm here and -1 it, but I'm open to discussion :)

I decided to exercise my new fast Macbook with some benchmarks. Unfortunately, I am seeing a 6% slowdown after this patch. I don't really believe it but there it is. I took a large sample (1000 requests) :(

Command: ab -n 1000 -c 10 http://mm/pr/index.php (against current HEAD)

BEFORE

Concurrency Level:      10
Time taken for tests:   26.652548 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      4374000 bytes
HTML transferred:       3829000 bytes
Requests per second:    37.52 [#/sec] (mean)
Time per request:       266.525 [ms] (mean)
Time per request:       26.653 [ms] (mean, across all concurrent requests)
Transfer rate:          160.25 [Kbytes/sec] received

AFTER

Concurrency Level:      10
Time taken for tests:   28.433135 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      4374000 bytes
HTML transferred:       3829000 bytes
Requests per second:    35.17 [#/sec] (mean)
Time per request:       284.331 [ms] (mean)
Time per request:       28.433 [ms] (mean, across all concurrent requests)
Transfer rate:          150.21 [Kbytes/sec] received

I doubt this can be solved without going OOP -- and I am unsure of the performance implications.

I do not fully believe either my or Moshe's benchmarks. We are adding a single extra function call which cannot be such a large percentage increase over loading the files, compiling them, making the database queries, etc. If it really is, then we could get a substantial performance boost by using some automated techniques for inlining virtually every function call. Who knows? Maybe that would help!

Anyway, I'm not going to work on this until after the schema patch gets in.

Barry, that's an interesting idea. I will experiment a bit with inling various little function.

I did a code review of our new template system and we use variable function calls very heavily in D6. FYI.

Subscribing

Let's continue this as part of the new system: http://drupal.org/node/225450

I am having a hard time finding a version of the original multiple database type patch that will work with with drupal 6. Has anyone maintained one? Would anyone be interested in me posting one?

Thanks,
Steven

I'd like to use a this patch in D6, but the last one in this thread fails on several hunks since it's been quite a long time since that patch was rolled. If somebody has a recent patch that would be great.

Hi, I want to know if this patch works for drupal 7. Obviously, my drupal 7 crashes when I try to work with "mysql" and "postgres" in the same site. Another cuestion is: Why this problem doesn't fixed in drupal core yet? Thank's in advance.