Watchdog can't log errors when using db_set_active(non-default)

What is the expected behavior in this situation? Should watchdog try to use the default DB if it cannot write to the current DB?

I changed this to a feature request since this seems more like a request for a change in behavior than a bug.

This seems like a bug to me. There is a bigger issue of how to handle any attempts to write to the default or primary drupal database, not just from watchdog, when using db_set_active().

watchdog seems to be the most likely and is also the only such instance I can find right now. Thus, a fairly simple patch which sets the active database to the default before issuing watchdog database queries should be all that is needed.

Patch forthcoming.

Here's the patch. We save and restore the alternate active DB connection in watchdog, where we set the active DB connection to the Drupal default so that we can log whatever watchdog message came to us without failing.

I've never had a reason to use alternate databases within Drupal.

That said, I'd be pretty confused and upset if I switched to an alternate DB, call watchdog, and then the message went into a secondary db.

I think that trying to write to watchdog in whatever database is current should happen first, and then try to default DB if that initial write fails would be a more expected behavior.

I'd like to see the patch improved, it is currently not really "nice".

First of all it needs to be documented.

Why do we need those if statements in watchdog? Would it hurt if we'd switch the db anyway? Can we maybe not switch if we are already using the current db and put that logic into db_set_active?

The reason for the funky if checks in watchdog() was because chx asked me to put them there to optimize resource usage for most installs which won't have alternative databases. That is, it avoids calling db_set_active() at all in that case.

As for greggles remarks: I chose this method because in every case of alternate database usage that I know about (mine, and the half a dozen mentioned on this site), the alternate database was not a Drupal install and did not have a watchdog table. The idea was to log the error to a watchdog table which should exist, rather than puke it out on the web page, which is generally not desirable. However, greggles has a good point -- the problem is we have no way of guessing perfectly what the site admin really wants. So should we not do anything at all? Or should we make a guess and use the default database? Or do something else?

I'm happy to refactor this patch until it's clean and documented, and even works in a different way, if people can agree on how it should work. I know I need it, and the way it works right now is perfect for my usage; my alternate databases are all non-Drupal databases.

I believe the change in functionality that I made to db_set_active() to return the name of the previously active database is must-have functionality. Regardless of whatever else we do about this problem, contrib modules which call db_set_active() need to know which database to reset to after they are done with alternate database access. Right now, all existing code that I've seen simply sets the database back to the default. That may not always be the right thing to do, especially given Drupal's hooks which allow modules to insert behaviors into other modules.

One wants to write code like this:

    $prevdb = db_set_active('provider');
    $provider = db_fetch_object(db_query("SELECT * FROM {provider} p WHERE pid  = $pid"));
    db_set_active($prevdb);

Well, I guess I was envisioning someone using distributed databases - e.g. the distributed "these databases are read only, this one is read/write" system - and logging watchdog to the read/write db. If someone were doing that then they might get confused if they changed the active DB and then watchdog changed it back underneath them. Then again, if someone is savvy enough that they are using multiple DBs, I think they are savvy enough to be able to edit/tweak this code if it behaves differently from their desired system.

So, I'm now thinking that the behavior of switching to use the drupal db for watchdog entries by default makes more sense. The only question I have is that since watchdog gets called pretty often, is there a performance hit associated with the extra check?

How about this patch? It needs some texting with actual db switching

Installed killes patch in my dev/test environment, where I am using multiple databases (all MySQL, though). Will report back how it goes.

I find it illogical that set_active() returns the prior db. i think it reads better like

$prevdb = db_get_active()
db_set_active('mynewdb');
// do stuff
db_set_active($prevdb);

I agree that it isn't that logical, but how does your code improve the situation?

in my code, get means get, and set means set. in the patch, set means 'set and give me prior value'

In general, I agree with Moshe. My original patch which got us going down this line was intended to absolutely minimize new code, due to proximity to the release, which is why I took this short-cut. (Ok, and I plead too many years writing Unix device drivers for retarded devices with registers that were read follow write. Yeah, that's my story and I'm sticking to it. :-)

Killes's patch has been running my dev/test environment since Friday afternoon with no problems, so it looks like it is bug free.

To fit Moshe's comments and have get/set functions would require a lot more work, because the connections are kept in a static array within db_set_active(). I don't know if I want to go there at this late hour before the release.

Note this would not be the only place in Drupal where get and set functions are mixed together. drupal_set_content(), drupal_set_breadcrumb(), etc. all are similar. They're all ugly, too. :-)

yay for bug free patches, applied.

the attached docs update was also applied.

so much for bug free patches :p

Weird. I guess as long as the previous DB was the default DB, the bug in the patch had no effect. I didn't test with more than one alternate database, sorry. Glad you spotted it and fixed it.