Database Layer: The Next Generation

subscribing.

subscribing.

Subscribing.

subscribing.

For those following at home: I just installed Drupal with the new layer (and previous tests as long as a week ago showed installed Drupal working). We still need to work on error handling.

yes, another "subscribing" (with hopefully more informative followups :)

We have the system installing and reinstalling, users, node and upload tests are passing. When all tests pass I will submit the patch, not until. It's close, though.

subscribing

@chx: Where are those tests that are failing, or are you just talking in general that not everything's working? If there's a list of what's wrong, I'd like to pitch in.

I'm new to PDO, but I thought that FETCH_ORI_ABS, FETCH_ORI_REL do the same as the pager_query().

Hope to mention that we still have an alternative choice of PDO implementation from http://drupal.org/node/172541, which based on existing DB API architecture plus totally 5 databases supporting. I think both issues have no conflict and should able to contribute for each other, e.g. this issue target for ORM/active record/PDO, but the other focus on multiple database supporting plus backward compatible. Hope we may have more join development for better implementation.

subscribing

Smal, trivial patch, hardly needs any explanation.

About stability. This patch passed all existing simpletests plus the pager one I have even written for it -- with two exceptions: one, if locale is not enabled the modules test fail -- but manually doing the same does not fail. As simpletest had module issues recently, I am not worried, Two, blogapi fails in HEAD and I have not yet figured it out why so it fails with this one too. Aside from that all simpletests work.

This patch implements my object-based transaction system, so I of course support that. :-) Reviewing this patch will give me something to do on the plane to Drupalcon.

Big patch, I'm reviewing it (slowly).

$ diffstat pdo_1.patch 
 includes/bootstrap.inc                  |   57 
 includes/cache.inc                      |   29 
 includes/database.inc                   | 2639 ++++++++++++++++++++++++++++----
 includes/database.mysql-common.inc      |  533 ------
 includes/database.mysql.inc             |  419 +----
 includes/database.mysqli.inc            |  376 ----
 includes/database.pgsql.inc             |  926 -----------
 includes/form.inc                       |    2 
 includes/install.inc                    |  111 +
 includes/install.mysql.inc              |  117 -
 includes/locale.inc                     |   29 
 includes/menu.inc                       |   16 
 install.php                             |  159 -
 modules/dblog/dblog.install             |   13 
 modules/filter/filter.module            |   16 
 modules/menu/menu.module                |   11 
 modules/node/node.module                |    9 
 modules/path/path.admin.inc             |    8 
 modules/profile/profile.admin.inc       |    2 
 modules/profile/profile.pages.inc       |    5 
 modules/search/search.module            |   20 
 modules/statistics/statistics.admin.inc |    6 
 modules/statistics/statistics.module    |   29 
 modules/taxonomy/taxonomy.pages.inc     |    5 
 modules/trigger/trigger.module          |    6 
 modules/user/user.module                |   13 
 modules/user/user.pages.inc             |    2 
 sites/default/default.settings.php      |  109 +
 28 files changed, 2893 insertions(+), 2774 deletions(-)

Just curious but did we do any performance comparisons? This patch has the advantage that we can exploit database specific optimizations, but at the same time, how much slower did the average select query became? Do we have any early insight in this?

ajk did benchmarks a few days before the patch was rolled and he found that there is no slowdown and sometimes there is some speedup. the patch is not really about performance but features. Of course, we took as much care as possible with performance, for eg. not using native prepared statements for mysql because those are not cached by mysql.

As chx's mentions, I've been looking at how it performs for a while now as the question was going to be asked sooner or later. And since HEAD hasn't deviated too far from Drupal 6 now is a good time to publish some results so far. Rather than clog up this issue with lot's of performance/profile related junk I created a more detailed post here:- http://drupal.org/node/229132

If anyone wants more info, drop me a line. I'll continue to profile this patch as it matures but based on the forum post above it's a +1 from a performance angle at the moment at least.

Here's a few things I came up with on the plane.

Functional problems:
* Functions and variables are CamelCase, which isn't Drupal style.
* Transaction object doesn't handle nested transactions properly. It needs a static counter.
* Doesn't implement LOW_PRIORITY for UPDATEs, even though DELAYED is supported.

Questions:
* Does the inheritance of PDO allow freeing result sets?

About CamelCase, we feel that as PHP largely uses CamelCase for its objects while it uses lowercase for functions so we though we will adopt. LOW_PRIORITY would be easy to support but what's the point...? What do we gain by supporting it?

Regarding LOW_PRIORITY, I just think that we should be consistent in support of non-standard MySQL extensions. If we support DELAYED, I think we should support the other options.

subscribe

subscribing

DELAYED is extremely useful for performance so we are supporting it -- do we really want to debate this in this issue? is this the big issue w/ this? can't we have a followup issue ?

hello Strauss,

several days ago, i had sent a letter to your mailbox: shout at thatotherpaper dot com.

i hope that you can reply it.

many thanks.

Subscribing. I agree with only supporting features of benefits. If there's no real use in supporting LOW_PRIORITY lets keep it out. It can always land at a later stage once this gets in.

Subscribe. I've started reviewing this patch. I already have a list of comments; no show-stoppers but some things to fix.

OK, I'm really late to this thread. Bear with me. Can someone explain why replication (and the major changes that come with it, like db_select() vs. db_update()) outweighs a solution like memcached, which many would argue is a much simpler solution to scaling than clustering? It doesn't seem like MySQL Clustering is that impressive -- and for obvious, fundamental reasons relating to the challenges of master-master, master-slave db replication, and it costs you a whole lot of new code complexity.

@Crell I agree that it's much less useful. I was arguing from API consistency, not pragmatism. I won't be offended if we leave out LOW_PRIORITY support; I just want it to be a conscious decision.

should this patch include this fix? http://drupal.org/node/220064

@36: I don't think so. The bug in #220064 is that a query is broken: it is treating strings and ints as directly comparable, and the latest PostgreSQL is picky enough to complain about it (as opposed to mysql that will sleep with anyone it can recognize). Arguably, the query is broken because our schema is broken. But in no case does our API to execute queries have anything to do with this.

Someone might argue that we should use schema information to automatically cast data types when necessary to make queries like this one work. I'd say no; the solution is not to write broken queries.

@bjaspan - if it should not be included, let's move that patch to 6 .x and get it in sooner.

Regarding DELAYED, are we ok that it's not supported on InnoDB ?

• INSERT DELAYED works only with MyISAM, MEMORY, and ARCHIVE tables.

http://dev.mysql.com/doc/refman/5.0/en/insert-delayed.html

Something somewhere reminds me d.o is using InnoDB tables for something. Not sure if this is/was the case though.

Could we please stop painting the bikeshed? If not, I am perfectly happy to roll a patch without INSERT DELAYED if that's needed to move on but really, instead of painting the bikeshed, let's discuss the atomic plant:

1. Are we happy with classes and objects in Drupal?
2. Are we happy with naming those CamelCase-style
3. Are we happy with the chainable query builder in here?

My review is still underway and I have some feedback that will require changes (nothing show-stopping), but one immediate issue is that the schema.inc and schema.*.inc files are missing from the patch. Could we have a new patch file including them?

You may find cvsutils (http://www.red-bean.com/cvsutils/) helpful in tricking CVS into including newly created files in a cvs diff. I had to do this for the Schema API patch.

Also, if at some point in the future this patch is sitting still for a while, I'll volunteer to go through and make (just a few) changes for further compliance with coding standards. From what I've seen so far, there are really very few "nitpicky" changes that need to be made, and the code comments are excellent. The biggest offender I've seen so far is two spaces between most sentences, rather than core's preferred one.

WRT to ajk's question in #39: Yes, drupal.org indeed uses innodb for everything but the search tables (which are myisam). However, I don't think that using insert delayed would be a problem for e.g. watchdog inserts. We should use the archive type which is IMO more suitable for this.

@chx in #40. Ticks to 1, 2 and 3.

The patch came in at #17 and this is now #45 without a re-roll/etc. I can see DELAYED is supported but it's not actually used by Core. If it's the only stumbling block so far congratulations to Crell and Chx on some fine work.

Remove INSERT DELAYED if it will settle things if you like, call it a natural finish and get it committed. I'm sure Keith would be rather rolling his patch to remove extraneous spaces/lines against HEAD.

We all just need somewhere to park our bikes ;)

p.s. assuming we get the missing files bjaspan refers to.

The patch is not ready to apply in its current state. I set it to CNW for a reason. I plan to finish my initial review and post the results tomorrow at the code sprint.

I personaly don't like classes, at least for what developers will actually use (although I think this whole abstraction is completly doable with functions). And I don't see much value in chainable methods (specifically on db_insert() calls). Some examples:

db_insert('my_query', 'node')->fields($fields)->execute();

How this differ, from:

db_insert('my_query', 'node', $fields);

is there cases when the following syntax is necessary?

$foo = db_insert('my_query', 'node');
$foo->fields($fields);
$foo->execute();

Or is there cases when you want to call $foo->execute() before setting fields using $foo->fields()? IMO this: db_insert('my_query', 'node')->execute()->fields($fields); doesn't make any sense and it doesn't works as jQuery's chainable methods does, i.e. the order is not important unless you are filtering/traversing DOM elements.

And for the duplicateUpdate($update_fields) method. Is there cases where $insert_fields without the keys is different from $update_fields? If not, so db_insert() should be capable of knowing where a record already exists, and do run a UPDATE if so. So the db_insert() should be renamed to something else like db_save() or db_write() as it not only does INSERTs but UPDATEs too.

Batch UPDATEs seems to be a case for chainable methods in db_insert() however I don't see any example of this use in the patch.

What's the problem with a $params array with conditions and other properties for doing a select?

My opinion is that we need to provide a simple interface for trivial tasks, not enforce users to write complex code for very simple data manipulation stuff.

Subscribing.

Interesting pattern question there, especially in light of Drupal's stance on OOP.

Chained methods are neat - jQuery does everything like this. But can we mix patterns like this in Drupal too? Calling member methods on the return value of other functions strikes me as fitting more appropriately into prototype-oriented programming than either functional or object-oriented programming... but I'm not well-versed in patterns.

Here is what InsertQuery::fields says: "This method may be called multiple times. If it is, multiple sets of values will be inserted. The order of fields must be the same each time.". That's useful, isn't it? PDO already uses objects and methods and we would need to provide a wrapper for each -- we thought that's not the best.

Changing back to 'code needs work' as in #47.

I am going to work on a pgsql driver at code sprint today.

#17.11. "A Fluent API query builder for SELECT statements that weighs in at only a few hundred lines (not counting comments). It doesn't replace the Views query builder, yet, but I will be working with merlinofchaos at DrupalCon and afterward to beef it up to the point that it does."

Adding join info to schema might help us here. Or maybe we can leave that to 'core fields API' and just provide the interface to generate such queries?

I've discovered a bit of a snafu.

Here is one example scenario. BOOTSTRAP_LATE_PAGE_CACHE calls variable_get('cache') which, if the variables cache entry has expired, calls cache_set('variables'). cache_set() calls db_insert()->duplicateUpdate(). On pgsql (or any database without its magic "INSERT ... ON DUPLICATE UPDATE" feature), duplicateUpdate() requires calling drupal_get_schema() to identify the primary key of the table to generate the WHERE clause for the generated UPDATE query. Problem: drupal_get_schema() is defined in common.inc, not loaded until BOOTSTRAP_FULL, but we're only at BOOTSTRAP_LATE_PAGE_CACHE.

We can move drupal_get_schema() into bootstrap.inc so it is loaded earlier, but drupal_get_schema() calls drupal_alter('schema'). drupal_alter() is also defined in common.inc, and also requires all modules to be loaded anyway, so moving the function to bootstrap.inc does not help. drupal_alter('schema') isn't used or documented or used, so we could just yank it.

I did yank it. However, then the UpdateQuery generated for duplicateUpdate() calls drupal_alter('query'). This fails for the same reason drupal_alter('schema') does; drupal_alter() just cannot run during early bootstrap.

This explains, interestingly enough, why the drupal_alter() call in database.mysql.inc's InsertQuery_mysql::execute() is commented out. The reason this problem shows up with pgsql is because its duplicateUpdate() handler calls the normal UpdateQuery class which has a non-commented out call to drupal_alter().

It's late and at the moment I do not have a proposed solution.

One other point: Any query that calls a join method that wants to use the schema to determine the join columns will have the same problem.

Why not just move drupal_alter()? I know that some modules are not loaded and won't have a chance to alter but thats tough luck. they can get loaded earlier by implementing hook_boot() or marking selves for bootstrap=1 in system table.

I was thinking that while waking up this morning. Perhaps it is the answer. After all, modules already can't alter queries that run before they are loaded---or after they are loaded---so there won't be a big change.

The bottom line is that by defining a database system that uses the schema and creating drupal_alter('query'), we're increasing the amount of code that must/might be loaded at BOOTSTRAP_DATABASE. We always wanted to use the schema information for more intelligent query handling anyway , I just hadn't realized this consequence until now.

A very useful use-case for query_alter is the one we discussed at the sprint: treating it as a query listener interface. It allows (e.g.) devel module's query log to operate without a special-purpose hack. It also allows us to kill update_sql() but continue to produce the update query log, including *all* queries, not just the ones performed with update_sql().

If we decide allowing non-select queries to be altered isn't a great idea we could certainly rename it to query_listener or somesuch.

Another snafu from the pgsql driver:

The old _db_query_callback() hack converted %d to "(int) array_shift($args)". Unfortunately, "(int) ''" (i.e. "(int) empty-string") evalutes to 0. This allowed bugs in which a query uses %d but the $args value corresponding to the %d is an empty string or FALSE. This bug appears in several places in core (node.module and menu.inc so far) and, presumably, lots of places in contrib.

Now that we are replacing %d (and others) with ?, the translation from empty-value to 0 for %d args is not happening. MySQL appears lenient/sloppy enough to accept the empty string as an integer but PostgreSQL rejects it as an invalid string-encoded integer (which it is).

If we want to provide backwards compatibility for db_query(), we are going to have to update the new db_query() to use something like the old callback system, at least to process %d and %f correctly. I'll have to think about %s and %b. I also observe that %% was previously translated into %, we'll have to fix that, too.

I have not yet tried to fix this.

The alternative, of course, is to make people fix their broken usage of db_query(). The db_query() spec never said you could pass FALSE or empty-string as 0.

Fine with me, though we'll be finding and fixing these bugs for a long time. If we had tests that covered every query that caused every %d argument that can be zero to be zero then we could find them all quickly but, frankly, there is no way we'll have that if we get "100% test coverage."

Here is an example of one such fix, a patch to menu.inc:

-  $item['_external'] = menu_path_is_external($item['link_path'])  || $item['link_path'] == '<front>';
+  $item['_external'] = (menu_path_is_external($item['link_path'])  || $item['link_path'] == '<front>') ? 1 : 0;

Later, $item['_external'] is passed as a %d arg to db_query.

I was thinking of an interesting option this morning. Just as we could write a mock-database driver, we could also write a pedantic-database driver filter that sat in front of a real driver but did all kinds of integrity checking on queries such as making sure the data types of the arguments matched the query fields.

Another snafu. As previously discussed, the pgsql driver needs to call drupal_get_schema() early in the boot process. Even if we stopped supporting drupal_alter('query'), drupal_get_schema() is still necessary to implement the duplicateUpdate() method.

drupal_get_schema() caches the combined schema of all installed modules. When invoked during bootstrap, it caches the limited set of modules that are loaded at that point. During the install process, it seems to cache the tables defined by system.module and nothing else.

A limited schema cache breaks drupal_write_record() because that function fails if it cannot find the table schema. It will presumably break other things as well.

The solution I've thought of so far is to have drupal_get_schema() refuse to create a cache entry if the boot process is less than BOOTSTRAP_FULL. It can still use the schema cache if it exists (since any cached entry will now be a full schema). This will require slight changes to the bootstrap API since it does not currently make the boot phase available. I'll give it a try and see what happens.

Barry, Larry stated multiple times that duplicateUpdate won't require schema. That's good. About condition putting the op to the end, omg. Just OMG, are you sure you want to make people understand (and do) RPN logic in Drupal? I love RPN, I am using RPN calculators since 1989 but... Larry also wanted the same order of arguments and I was more than reluctant to do so.

I have installation and logging in as uid 1 working with the pgsql pdo driver though node/add still whitescreens. Obviously there are some bugs left. :-) Calling drupal_get_schema() and/or drupal_alter() inside the database driver during bootstrap is triggering a lot of subtle bugs that have probably existed since the bootstrap code was added.

duplicateUpdate() absolutely requires schema information unless we require that the caller specify the primary key fields (i.e. the fields to move from the INSERT list to the WHERE clause of the UPDATE). It seems kinda odd to require the caller to specify that information since we just went to all the trouble of creating Schema API to provide exactly that kind of information and capability.

Regarding condition(), I think condition($field, $value, $op) makes more sense that condition($field, $op_unless_you_do_not_provide_a_value_in_which_case_value, $value_if_you_provided_an_op). It isn't really RPN logic, it's an optional argument at the end to override a default value, a very common pattern in Drupal. We could hold a survey but I'd have to reserve the right to ignore responses from people who do not know what they are talking about. :-)

I should have some more time to work on this tomorrow.

Well, currently you do specify the keys when writing INSERT, db_affected_rows and a full UPDATE. Also what if you want to work with a non-Drupal database? I am leaning towards mandating a full blown update statement and then MySQL can ignore the keys.

I'm not prepared to express an opinion on "mandating a full blown update statement" but understand that doing so will resolve NONE of the issues I've reported so far unless we also remove drupal_alter('query') and engage in some cleverness with lastInsertId() (which, granted, we may need to do anyway).

I suggest waiting until I'm done before making substantial changes. It should be soon.

By the way, this code is a perfect use case for unit tests. We have a (in theory) clearly defined API that is easily testable and multiple implementations that are supposed to behave the same way. If the database code is buggy or inconsistent in any way, difficult-to-find bugs will result. IMHO, we should not commit this code until we have thorough unit tests for it.

@Barry - we are at the very beginning of the code cycle for D7. I think it is a bit early to deny patches that omit unit tests. We have not even added a single unit test to core yet. Your motivation is sound, but the timing is off IMO. If someone wants to write tests that terrific.

@moshe: Yes, we're at the beginning of the cycle, but I draw the opposite conclusion: This is the perfect time to set the tone that patches require tests. Otherwise, when is the right time? Everyone will say "I'm too busy, I'll write tests later/after code freeze" which means "never." Also, if the tests reveal bugs at that point, it is much harder to change the new APIs, etc. than it is before the patch is committed.

More importantly, as the person most familiar with this code besides crell and chx, I have little confidence that it implements the documented APIs correctly, and I have no confidence that the pgsql driver implements it correctly, because I'm not sure the API docs are even self-consistent. Writing tests will flush that out.

@crell: I do not know if there is a plan for where to put core tests yet. For now we can create a dbtest module and move the tests when they have a correct home.

I'm still hoping to commit the pgsql driver to svn today.

Even if not all patches require tests, this one really should. It will screw up everyone if it is wrong.

All core tests are currently patches to simpletest module for now so they can be brought over in bulk. Somehow I don't think this is applicable for tests for patches which aren't into core yet (although two patches, one in the core queue and one in the simpletest queue seems sane to keep things as centralised as currently possible).

afaik, simpletests aren't required for patches to be committed, but they are required for a code freeze extension. If there's going to be an additional requirement for unit tests on big API-rewrite patches then it'd be good to have this made clear asap. But since we're only a week after testing was officially announced in the first place, it also seems a little early to me.

I was hoping that we can write tests for this one once its in. I can certainly accept that forthcoming big patches need a test -- but then that's part of the process... I just would not like this one held up by the unanswered questions about testing.

(subscribing)

With the attached patch, the pgsql PDO driver correctly handles installation, login as uid 1, posting a node, and posting a comment. There are still known bugs, but I wanted to get something committed. This code is NOT ready to commit, so I'm leaving it as CNW, but reviews are welcome.

If you are following chx's SVN repo, this is revision 357.

## To do items

* I have NOT done any pgsql BLOB handling, bindParam(), etc. stuff. I've noticed that sometimes the 'schema' entry in the cache table gets lost, causing a PDOException. I suspect these are related.

## Recommendations (not yet implemented)

* There should be a central anonymous-placeholder name generator, otherwise we might get conflicts. Consider:

  UPDATE {cache} SET serialized=:db_update_placeholder_0, created=:db_update_placeholder_1, expire=:db_update_placeholder_2, headers=:db_update_placeholder_3, data=:db_update_placeholder_4
  WHERE (cid = :db_placeholder_0)

There is not a conflict between SET and WHERE, but there could be. If the placeholders are all just :db_placeholder_NNNN, there will never be a conflict.

* The return value of Query::execute() is not clearly specified and appears to vary by query type. What should InsertQuery::execute() if it performs an UPDATE query instead? Since there is no lastInsertId() to return in this case, the caller cannot rely on the return value being a last insert id (we surely do not want the return value to depend on whether duplicateUpdate() was called).

## Changes to the database layer in this patch

* I fixed the "FALSE/empty-string as %d" problem by making db_query() (and friends) take care of it. I just couldn't deal with fixing queries all over core at this time (there are a lot of them). I did fix a few before changing db_query(), and I left those fixes in.

* Remove abstract protected functions from class DatabaseSchema. Not all schema drivers implement the same internal functions.

* When throwing a PDOException, include the full text of the query and args.

* DatabaseConnection::lastInsertId() requires some special care (surprise!). For mysql, it takes no arguments. For pgsql, it requires the name of a sequence object. Our db_last_insert_id() function takes $table and $field, which identifies the sequence object, but according to the specs you have to pass lastInsertId() an argument or not depending on which driver you are using (not very portable!), so we can't just pass $table and $field to lastInsertId() and let the mysql driver ignore them. In the case of an InsertQuery() object, the object already knows the table and can get the field from the primary key. But in the case of db_last_insert_id(), we have to preserve the information without passing args to lastInsertId(). Therefore, I added a setLastInsertInfo() method to class DatabaseConnection. mysql's does nothing, pgsql's remembers what it needs.

* Add db_driver() method as replacement for $GLOBALS['db_type']. Warning! Many 6.x updates and other code will stop working because this variable is no longer available.

## Core changes in this patch (required for pgsql support)

* Add drupal_get_bootstrap_phase(). drupal_get_schema() needs to not cache the schema of we're not at BOOTSTRAP_FULL.

* Load includes/module.inc in BOOTSTRAP_DATABASE. This is so database queries can call drupal_alter(), drupal_get_schema(), etc. during early bootstrap.

* Move drupal_get_schema() and drupal_alter() to bootstrap.inc. Database queries need these.

* Fixed %d/'' bug in session.inc INSERT and UPDATE query.

* In module_list(), initialize static $list = array(). Otherwise, if module_list(FALSE) is called before module_list(TRUE), NULL is returned instead of an array() and foreach throws an E_WARNING. This happens (e.g.) when a query calls drupal_alter() during early bootstrap.

* In _drupal_bootstrap_full(), rebuild the module_implements hook cache. During bootstrap (perhaps only during install.php), the hook cache is limited to system and filter modules before BOOTSTRAP_FULL and needs to be reset once new modules are loaded.

* Fix two %d/FALSE bugs in menu.inc queries.

* Fix three %d/'' bugs in node.module.

* Cache a fully-build schema during install.php so schema-based queries can work on a brand new site before anyone visits the modules page.

Addenda:

1. When I said, "There are still known bugs, but I wanted to get something committed" I meant "committed to chx's SVN repo.

2. I did not attach I patch to the previous comment. I meant to say "With the SVN commit I'm about to make...". If you want to see the changes I made to Larry+chx's work, use svn diff.

3. To this comment I am attaching a complete patch against HEAD for the most recent version of the PDO changes including my pgsql+related stuff. Note that this is Larry's issue so you should not take this as an authoritative patch, I'm just being helpful, and the code needs work anyway so it is not like this will get committed. Unlike the previous patch, this one actually contains all the files you need to use it. :-)

I still have to do a proper review but I decided to hold that off for a while -- Barry's review is excellent and I'm happy to let him drive this for a while.

Here are a couple of thoughts that might help decision making and debates:

* Delayed inserts are useful. Let's keep it.

* Supporting multi-insert statements is useful. On MySQL it can speed up things by a factor 20. Let's keep it -- but let try to document it better. It took me a while to figure this one out.

* For me, writing db_insert($id, $table)->fields('foo', 'bar')->values($foo, $bar)->execute(); is more natural but also more prone to errors. I might like db_insert($id, $table)->fields('foo' => $foo, 'bar' => $bar)->execute(); better as I'd make fewer mistakes. Still undecided about this one.

@dries: fields('foo' => $foo) is syntactically invalid; you can't use => outside an array. fields(array('foo' => $foo)) is how the current interface works. Allowing an inline-array with fields() as above while still supporting duplicateUpdate() is the purpose behind my proposed changes to that interface (which I think Larry agrees with).

@crell: re: dbtest, excellent! I'll add some tests when I can.

Multi inserts on other databases are inserts in a transaction, and if there is a unique key collision, I guess the transaction gets rolled back. This is just a guess. MySQL has tricky handling of this see MySQL handbook on VALUES(). Definitely a followup patch.

@crell: What should db_insert()->orUpdate() return if it actually performs an update? The id of the updated row? Should the caller be able to tell whether an insert or update occurred?

db_last_insert_id(), db_query(), etc. may be vestigial but the contrib update to D7 will be a lot slower if we yank them before D7 is released. This will be a decision the community has to make. We shouldn't pre-determine that they have to be yanked by not supporting them now. (And in any case, today's code uses it, so it needs to work for now.)

@chx:

Multi inserts on other databases are inserts in a transaction, and if there is a unique key collision, I guess the transaction gets rolled back. This is just a guess.

According to ANSI SQL, that's correct.

Random musings:

The "abstract" classes in database.inc, such as InsertQuery and UpdateQuery, perform some non-trivial computations on some method arguments. For example, InsertQuery::fields() splits its $fields array into keys and values and loses fidelity of the original data by storing "$fieldname=$placeholder" instead of just $fieldname.

This is a problem for drivers (like pgsql) that need to walk through the field values and do something to them based on the scheme (such as blob handling). Of course, the driver can override the method and store the original data (which is what pgsql currently does) but then it has to choose between calling parent:: which it knows to be all/mostly useless or not calling it and risking losing important functionality that may get added there in the future.

I am thinking that most of the work performed in these abstract classes should not be. The classes should either be pure interfaces or the methods should just store the raw input data as received. execute() or similar can perhaps serialize things into SQL as a "generic" implementation but doing so earlier (e.g. creating the placeholder variables, etc. in fields() or values()) I think is counter-productive. It makes it more confusing, not less, to create a driver.

I'm working on proper blob handling for pgsql. I'm having to override most everything in Query and, I recently realized, DatabaseCondition as well. When I'm done, we'll have a clearer idea of what the abstract classes should do.

Some thoughts on code style.

As we are changing the db_*() functions interfaces drastically, wouldn't it make sense to make them static class methods for consistency? So, for example, instead of:

db_insert('variable_set', 'variable')->fields($fields)->execute();

it would be:

Database::insert('variable_set', 'variable')->fields($fields)->execute();

The consistencies I'm talking about here are a) it would not mix functional code with class based ones, and b) the line of code would be entirely Camel-cased.

@crell: I actually find the way you are assembling placeholders, etc. quite confusing as it is. :-) I realize it is a tricky problem since you need to create placeholders separately for fields()/values(), conditions, etc.

Something I may need to do is iterate through the UPDATE SET fields (at least), identify which ones are blobs, and use bindParam on them. I'm not sure but I think this means I need to use bindParam on everything for the query. So that means I also need to be able to iterate through the condition fields and bindParam them, too. It was at this point that I punted.

Yes, yay for testing, for exactly the reason you said. Be sure to test the multi-insert functionality; I'm pretty sure I did not understand your data structure for that at first and got it wrong in the currently committed code.

My latest understanding of the bootstrap snafus I mentioned:

* The earliest queries Drupal performs are reads from the variable and cache tables. This is before any modules are loaded.
* The cache tables use blobs.
* Databases with special blob handling need access to the schema in order to know that cache tables use blobs.
* The schema structure lives in the cache but that requires access to the cache table to learn that the cache table uses blobs.
* The schema structure also lives in hook_schema(), but no modules are loaded yet.
* Catch-22.

In pgsql's case, we can apparently SELECT from a blob column without knowing it, so we get around the catch-22. For other databases, that trick may not work.

What this means is that if drupal_get_schema() cannot retrieve a schema from the database and no modules are loaded yet, it needs an emergency fallback of loading system.install (perhaps all required-modules .install), calling those hook_schema()s, and returning the results.

In pgsql's case, we can apparently SELECT from a blob column without knowing it, so we get around the catch-22. For other databases, that trick may not work. <= it will work, PDO stringify option is not DB specific to the best of my knowledge.

Well, reading pdo_stmt.c definitely underlines this:

case PDO_PARAM_LOB:
			if (value == NULL) {
				ZVAL_NULL(dest);
			} else if (value_len == 0) {
				if (stmt->dbh->stringify || new_type == PDO_PARAM_STR) {

Edit: pdo_stmt.c is, of course, not database specific. And in the further lines we have stream handling too. And this is in fetch_value. All is well.

@crell: I can live with orUpdate() taking field/value arrays if, when passed an empty array, it updates all the fields to the values originally specified to db_insert()->fields() (i.e. in the degenerate case, orUpdate() == orUpdateExcept()).

@chx: If PDO can magically handle selecting from blobs on all databases, why can't it magically handle inserting/updating them?

@bjaspan, ask Wez, I know not.

I've started to look into the tests a bit as that is a good place to learn how the API is intended to be used. Here are some initial comments/thoughts:

- testRepeatedInsert() and testMultiInsert() look identical? If not, the difference is probably too subtle and needs to be documented.

- $num_records_before = db_query("SELECT COUNT(*) FROM {test}")->fetchOne(); would be unnecessary if tearDown() was called.

- Can we rename fetchCol() to fetchColumn(). Also, shouldn't that be fetchRow()?

- I'd rename fetchOne() to fetchField(). I'd prefer to consistently use the proper database terminology; 'one' is undefined and could be a row or a field.

- I didn't understand the following yet -- nor the test that was using it:

// Dummy class for making sure we can fetch into classes.
class FakeRecord { }

Might be worth another line of documentation, or a more extensive test.

- It took me a short while to figure out the different between fetchAssoc() and fetchAllAssoc(). Here too, I'd clearly and consistently specify what is being returned. So maybe just write fetchNextRow() and fetchRowsAssoc()? I think we can and should make the API more self-explanatory.

- I don't like the fact that we need to create an empty dbtest.module and a dbtest directory for this. I'd prefer to put the database tests in the system module's directory as "db.test".

- How many of Barry's suggestions have been incorporated already? Do I go ahead and review the code some more, or is it best to hold of for a couple more days/weeks? Let me know.

I was also wondering how many of these changes are actually required to PDO-ify Drupal 7. As far as I can tell, PDO-ifying Drupal 7 does not necessarily require such heave API changes. I'm going to think about this some more, and review how many of the improvements listed in http://drupal.org/node/225450#comment-752493 actually require such drastic changes. I'm not necessarily against them, I just want to make sure we're not over-engineering this in light of the (stated) objectives.

It may be a silly question: what are we going to have with PDO-ify Drupal 7?

It seems our initial motivation are the use of PHP5-specific PDO driver backend, rather that legacy mysql/mysqli/pgsql drivers, in order to giant stability and performance improvement; moreover, promote the use of PHP5 starting from the level of database connection, so we will able to fully utilize those modern PHP5-specific features?

If we are just asking these features as starting point for further more development, the case of PDO-ify Drupal 7 should be much simple.

Let me mention that db_rewrite_sql is getting worse and worse release by release and this patch lets us kill it in a subsequent patch.

(subscribing)

subscribing

I will be getting back to working on this soon; I have some work time slotted for this issue.

Why remove all query IDs? I agree that they are not sufficient by themselves for identifying queries because, e.g., the replacement for db_rewrite_sql() can't know the query ID for every query it should rewrite. But certainly there are some times code will want to target a specific query from a specific module separate from other queries for the same base table, so the IDs are useful.

As for the db_rewrite_sql() case, basing it purely on base table isn't sufficient either since (e.g.) some node queries should involve all nodes and some should involve those the user has permission on. I was thinking what we need are query tags. So the db_rewrite_sql() replacement would document that it operates on all node queries having the "node-access" tag by doing the appropriate joins, and callers that want that behavior do db_select('node', ...)->tag('node-access')->etc(). "Property" is perhaps more meaningful than "tag" but has more letters in it. :-)

Crell: of all the motivations, the db_rewrite_sql() sounds like the most compelling one to me. Some of the other reasons could also be accomplished without this rewrite (i.e. multiple database support and array-based connection syntax). It would be helpful to explore the db_rewrite_sql() replacement a bit more, for example by trying to implement some actual use cases as part of the tests. Because what would be really bad, IMO, is that we'd have to resolve to a combination of regular expressions and this new database abstraction layer.

Crell: for the condition() stuff -- I'd go with condition($field, $value, $op) for now. It is a little bit awkward but certainly less awkward as the alternative.

Crell: I've just updated my SVN copy and I'll dig the code some more to help answer these questions. I'll be traveling the next two days so it might take me a little while.

Regardless of the "critical eye", this looks really compelling -- especially if we can make it work cleanly.

Comments on orUpdateExcept(). If you agree with these comments, I'll change the function docs:

* Must we require that it only be called after fields()? It seems logical to think "insert or update $fields" and therefore to write:

db_insert('myid', 'mytable')->orUpdateExcept('created')->fields($fields)->execute();

The requirement to call fields() first implies that you are performing some computation in fields() that needs the orUpdateExcept() info. Why can't that computation be performed later?

Incidentally, the same requirement is not listed in the orUpdate() docs.

* Looking at your usage, you seem to be passing the primary key fields to orUpdateExcept():

db_insert('cache_set', $table)->fields($fields)->orUpdateExcept('cid')->execute();

The whole point of orUpdate() and orUpdateExcept() is that the pkey already exists and will be used in an UPDATE WHERE clause. The orUpdate() query builder should never generate UPDATE foo SET pkey = :pkey WHERE pkey = :pkey (it's a no-op), so it should just implicitly remove the pkey from the fields it SETs. Thus I do not see the point in requiring the pkey to be passed to orUpdateExcept().

Note that if the point of requiring the pkey to be passed is to avoid depending on drupal_get_schema(), it won't help because the schema is needed anyway for databases that require special treatment based on column type.

* I'd suggest that calling orUpdate*() on a multi-insert query should result in a thrown exception, not just be ignored.

Comments on orUpdate:

* The docs for $fields arg say "if specified" but the function signature has the arg as required. I thought the plan was that orUpdate(/*no arg*/) would update all the fields to the values provided to fields() (except of course the pkey as discussed above). So it should be optional.

Again, if you agree, I'll make the changes.

Oh, and I'll update the tests too since I'll need to for pgsql.

subscribing

Progress!

The pgsql driver is now properly handling blobs via the bindParam() method and it seems to be mostly working. It passes all the tests Larry has written except for those that are in fact broken tests, in which case it is properly throwing exceptions. :-)

However, the code is very ugly and inefficient. There is a lot of wrangling with fields and values arrays in order to get the data I need when I need it. The next step is to clean it all up and refactor the code in database.inc to be doing only operations that are useful for all database drivers.

I'm pretty pleased to have gotten this far and I have another day of Acquia time left allocated to this task; I'll probably work on it on Monday.

Larry, regarding orUpdate() functionality, consider this query from testInsertOrUpdate():

    $query = db_insert('test_query', 'test')->fields(array(
      'id' => 5,
      'name' => 'Larry',
      'age' => '30',
    ));

    $query->orUpdate(array(
      'name' => 'Larry, Jr.',
      'age=age+1',
    ));
    $query->execute();

You are passing 'age=age+1' to orUpdate(). This works for mysql because of special-case code inside InsertQuery_mysql::orUpdate(). Questions/observations:

1. UpdateQuery does not support this notation, so InsertQuery::orUpdate() is actually more flexible than UpdateQuery(). Is this what you meant? It seems a rather incongruent API.

2. The fact that InsertQuery::orUpdate() accepts fields that UpdateQuery::fields() does not means that the former cannot be implemented in terms of the latter which is clearly not sensible for any database except mysql with its ON DUPLICATE UPDATE feature. So, if we support that notation in InsertQuery, we have to support it in UpdateQuery (though the converse is not true).

3. I'm not convinced we need to support this for InsertQuery(). IMHO, orUpdate() and orUpdateExcept() should take *only* a (possibly empty) list of field names, no values. If the insert cannot occur because the primary key exists, then the selected set of fields for that primary key are updated to the *same values* as would have been inserted. I think if you want more complex logic than that, then you should just write an insert query and, if it fails, run an update query (or vice versa). We should not try to build the ultimate generic "insert and/or update" operation.

4. I realize that we need to support 'age=age+1' in UpdateQuery somehow because that is a legitimate operation. However, I'm not a big fan of the notation in which some elements of the fields array are key/value and others are integer keys. One alternative is to specify key/value updates separately from expression updates, e.g.

db_update('id', 'mytab')->fields(array('name' => 'Larry Jr', ...))->expressions(array('age=age+1', ...))->execute();

In fact, the argument to expressions() could be array('age' => 'age=age+1'), putting the column being set in the key and the full expression (including the column being set) in the value. This would support a database which had to treat update assignments to certain column types differently (e.g. if they are blobs). I do not know if pgsql is such a database or if one exists.

5. I have a strong suspicion that any query builder we devise is going to be insufficient for some legitimate query that some module needs to perform, so that module will need to fall back on db_query(). Perhaps that is just life; hopefully core and the "important modules" can avoid doing so.

6. On a different topic, your test inserts explicit values into a type 'serial' column. This works on mysql but breaks on pgsql. On pgsql, you can do the insert but if you provide an explicit value the internal sequence on which the serial column is based does not get updated. So if the id sequence starts at 0 and you insert ids 0, 1, 2, and 3, then when you try to insert without specifying an id, pgsql gives you a duplicate-primary-key error because it is trying to insert 0 which already exists (whereas mysql will insert with id 4). Frankly, serial columns are extremely db-dependent, we have to stick to a very lowest common denominator if we want code to be portable, and in many ways I regret that we switched to using them.

7. On another different topic, according to the pgsql documentation, UPDATE and DELETE joins are in fact SQL extensions, though mysql and pgsql both implement them. I'm not sure whether we should support them or not. Perhaps we should research which databases support them; if they all do, we should.

I'm not convinced we need to support this for InsertQuery(). IMHO, orUpdate() and orUpdateExcept() should take *only* a (possibly empty) list of field names, no values

Then let me convince you, check statistics module which inserts a 0, and increments if the record is there. This is a typical case where update gets an expression.

@chx: The fact that some code wants to insert a new record or change an existing one with column-relative expressions does not, to me, imply that InsertQuery needs to support that in a "single operation." I see three possible levels of functionality:

1. InsertQuery does *insert*, UpdateQuery does *update*, and that's it. If you want to insert or update, do one and then, if necessary, the other.

2. InsertQuery supports unified "create or save existing object" semantics in which, if we are trying to insert for a primary key that already exists, do an update instead. In this case, we are always inserting or updating rows to the same constant values, we're just not always updating the full set of rows we would insert if the pkey did not already exist (this allows preserving, e.g., a "created time" field).

3. InsertQuery->orUpdate() is conceptually two completely different queries with completely different field specifications, as it is currently written now (and happens to be one query on mysql due to its particular SQL extension syntax).

The mere fact that code exists which could use #3 in order two reduce "if (!db_insert($foo)) { db_update($bar) }" into just "db_insert($foo, $bar)" is not nearly enough reason to introduce the complexity of the feature. If there is some reason that #1 *cannot* be implemented in a db-agnostic way and #2 or #3 can be, that might be a compelling argument for the feature.

In other words, you haven't convinced me yet.

Additionally, if we are going to support "two unrelated insert and update queries" in a single object, it should not be called InsertQuery but InsertOrUpdateQuery or something like that.

@crell: All the tests you've written (thanks!) now pass with the pgsql driver. Unfortunately, the tested code is broken even though the tests pass. How is this possible? Consider:

  function testInsertLastInsertID() {
    
    $id = db_insert('test_query', 'test')->fields(array(
      'name' => 'Larry',
      'age' => '30',
    ))->execute();
    
    $this->assertEqual($id, 5, 'Auto-increment ID returned successfully.');

Currently, InsertQuery_pgsql::execute() always returns TRUE, not last id, on success. Clearly that needs to be fixed but this test passes anyway because assertEqual() uses ==, not ===, and 'TRUE == 5' is TRUE. I thought that assertEqual() used ===, but in fact assertIdentical() exists for this purpose. (I well understand the difference between == and ===, I just did not realize which one assertEqual() used).

Unfortunately, converting assertEqual() everywhere in your code to assertIdentical() is not a quick solution. Consider:

  function testSimpleInsert() {
    $num_records_before = db_query("SELECT COUNT(*) FROM {test}")->fetchField();
    $query = db_insert('test_query', 'test')->...->execute();
    $num_records_after = db_query("SELECT COUNT(*) FROM {test}")->fetchField();
    $this->assertEqual($num_records_before + 1, $num_records_after, 'Record inserts correctly');

fetchField() is returning a string containing the digits for the count, but $num_records_before+1 returns a long. 5 == '5' but 5 !== '5', so assertIdentical() fails here even when the database driver is correct.

Given that we want these tests to be really scrupulous in making sure the results are correct, I'd say we need to use assertIdentical() everywhere and do the type casting ourselves if necessary.

Regarding

(Crell) - Query IDs will go away, for all query types. They're not appropriate here. Instead, hook_query_alter() will operate on the base table (only table in the case of insert/update/delete).

and

(bjaspan) 7. On another different topic, according to the pgsql documentation, UPDATE and DELETE joins are in fact SQL extensions, though mysql and pgsql both implement them. I'm not sure whether we should support them or not. Perhaps we should research which databases support them; if they all do, we should.

Postgres has supported joins during update and delete operations for 5+ years. They may well be extensions to the SQL standard, but Sybase, Oracle and IBM DB2 have supported them for at least a dozen years or more. I would bet Microsoft SQL Server has as well.

Which additional databases need to surveyed to answer this question? I'll be happy to go find out for you.

@Chris: The one I know I've heard discussion about that you did not list is SQLite. I just checked and it appears that it does NOT support joins for UPDATE or DELETE. So if we build them into the API, the API either won't support SQLite or the SQLite driver will have to figure out some way to implement them itself (which may or may not be possible).

SQLite does support sub-queries so presumably you can do things like UPDATE foo SET stuff WHERE id IN (SELECT id FROM t1 INNER JOIN t2...). Perhaps it is possible to translate API calls that add joins to UPDATE and DELETE queries into a sub-query approach. However, if that is always possible, then it also means that supporting joins on UPDATE and DELETE queries is unnecessary because the module author can just write the query in terms of a sub-query.

So for now perhaps we should just not support it.

re: SQLite vs. Postgres. I've never had any opinion about Postgres importance per se, only that supporting more than db is important to keep us honest and flexible; otherwise, we'll be tied to mysql for life.

re: expressions in update queries. I know we can't make it a normal key value pair without providing some other way to distinguish it from a literal string. However, in #120 I proposed a suggestion: providing a different method to specify update expressions vs update fields, e.g.

db_update('id', 'table')->fields(array('col1' => 'val'))->expressions(array('col2=col2+1'));

This gets us away from the hideous kludge of distinguishing fields vs. expressions in the fields() array with is_numeric($key). What do you think of this idea? I just really dislike having both string and numeric indexes in a single array; also, consider the perverse case of "CREATE TABLE foo (`0` integer)", which is legal at least in mysql. (As an aside, UpdateQuery::fields() should have the same argument options as InsertQuery::fields().)

re: assuming all UPDATE expressions are for "normal" column types. That's the assumption we have to make using the current approach, yes. I do not know if it is a reasonable assumption. For now, let's go with it.

re: insert-or-update. If mysql PDO does not support the if (!update()) { insert(); } logic because it always returns rows affected instead of rows matched, how to other systems that use PDO implement the equivalent functionality? There must be a way. If using mysql PDO means there is no way to write a functioning system without using ON DUPLICATE UPDATE, then IMHO PDO is useless as a multi-database API abstraction and we should not be using it.

re: SQLite vs. Postgres. I've never had any opinion about Postgres importance per se, only that supporting more than one db is important to keep us honest and flexible; otherwise, we'll be tied to mysql for life.

re: expressions in update queries. I know we can't make it a normal key value pair without providing some other way to distinguish it from a literal string. However, in #120 I proposed a suggestion: providing a different method to specify update expressions vs update fields, e.g.

db_update('id', 'table')->fields(array('col1' => 'val'))->expressions(array('col2=col2+1'));

This gets us away from the hideous kludge of distinguishing fields vs. expressions in the fields() array with is_numeric($key). What do you think of this idea? I just really dislike having both string and numeric indexes in a single array; also, consider the perverse case of "CREATE TABLE foo (`0` integer)", which is legal at least in mysql. (As an aside, UpdateQuery::fields() should have the same argument options as InsertQuery::fields().)

re: assuming all UPDATE expressions are for "normal" column types. That's the assumption we have to make using the current approach, yes. I do not know if it is a reasonable assumption. For now, let's go with it.

re: insert-or-update. If mysql PDO does not support the if (!update()) { insert(); } logic because it always returns rows affected instead of rows matched, how to other systems that use PDO implement the equivalent functionality? There must be a way.

I may be in the process of having an epiphany on the insert-or-update topic. Please stand by. :-)

So it turns out that pgsql PDO also returns rows affected and not rows matched for UPDATE operations. This leads me to conclusion that the answer to the question "How do you implement if (!update()) { insert(); }" is "you don't." The solution is to implement "if (!insert()) { update(); }" instead.

Consider:

* The whole topic of a joined insert/update operation is only meaningful if you have a primary key and provide it to the insert/update query (* see note). Otherwise, you can't construct the WHERE clause of the update without additional information.

* If you try INSERT (pkey, other data) it will either succeed, in which case the pkey did not yet exist so the INSERT is correct, or it will unambiguously fail with a "duplicate key" error.

* If the INSERT fails, the pkey exists, so the UPDATE is called for. You then do not need to know whether the UPDATE affects any rows. You know the row to update exists which is all that matters.

I previously had InsertQuery_pgsql trying the update first then, if it "failed", doing the insert. This didn't work because the update would succeed even if the pkey did not exist yet; it would affect 0 rows, but not result in any errors. I changed InsertQuery_pgsql to do the insert first (inside a try/catch) and do the update if it fails. Presto! Happy code.

I observe that this approach should work perfectly well on a "generic" database so we could, if we wanted, implement orUpdate() functionality in the database.inc generic classes. But since the code won't be ideal for mysql and won't work for pgsql (no blob support), I do not think there is a point.

The pgsql driver currently checked into svn successfully installs Drupal and passes all of the tests Larry has written so far. Note that it is far from done. If you look at how InsertQuery_pgsql and UpdateQuery_pgsql are written, you will see that they need to do a crazy amount of data massaging to get the bindParam() calls right. This requires some re-thinking of how data is massaged in the top-level InsertQuery and UpdateQuery calls, and in the DatabaseCondition class. Also, some other bugfixes and cleanups are needed.

* Note: A sub-case is when the primary key is type 'serial'. I assert that a dual insert/update query in this case makes no sense. If you are "inserting or updating" an id-based object and you have an id, then you know you are updating; otherwise you know you are inserting. As I've previously explain in this issue, it is (rather, it should be specified as) illegal to insert an explicit type 'serial' value under any circumstances because it breaks the way auto-increments are implemented on pgsql. So if someone calls db_insert('node')->fields(array('nid' => 12, ...)), that should just be an error and throw an exception. If they put ->orUpdate() on the end, it should probably still be an exception because they know perfectly well that nid 12 exists (or else where did they get the id from?), but we could for convenience just silently convert it to an update query with no attempt at insert.

Question: What should InsertQuery::execute() return?

DatabaseStatement::execute(), derived from PDOStatement, returns "TRUE on success, or FALSE on failure." without defining what success and failure means (I assume "failure" means "threw an exception" in which case FALSE will never actually get returned).

Class Query is our invention. Query::execute() is abstract and its return value is currently not documented.

InsertQuery::execute(), derived from Query, is also not documented but is written to return $this->connection->lastInsertId(). That's somewhat sensible except that not all inserts have a lastInsertId() (not all tables have an auto-increment column). But presumably the caller inserting into such a table knows not to expect a meaningful result. However, InsertQuery also supports orUpdate(). If an insert query may create a new row with a type 'serial' column or update an existing one, what should execute() return?

Actually, this dovetails nicely with my previous comment (I did not realize this when I started this comment).

* If you call db_insert('node')->fields(array(...))->execute(), the return value should be lastInsertId().

* If you call db_insert('node')->fields(array('nid' => 12, ...))->execute(), the return value should be Exception('DON'T DO THAT'). You can't provide an explicit insert value for a type serial field.

* If you call db_insert('node')->fields(array('nid' => 12, ...))->orUpdate()->execute(), the return value should either still be DON'T DO THAT or, if we are providing the convenience behavior I described in which this is converted into an update, the return value should be 12.

Comments?

Crell: I'm not against dropping support for the update handling, but it does make one wonder. One of the primary motivations of the proposed database abstraction layer (at least to me) is to be friendly towards database specific tricks. If we keep removing features, it becomes less compelling and it begs the question if we need to make such sweeping changes. I still think it is the right thing to do, but I do think it would be good to flush out the details as part of the first patch.

I actually see no reason to remove orUpdate() functionality. I've been arguing that the interface we define to it should be simpler but since my epiphany that db_insert()->orUpdate() needs to be implemented as insert-or-update instead of update-or-insert, the implementation is not a problem. mysql can use INSERT ... ON DUPLICATE UPDATE (note: that's insert-or-update), pgsql and anything else can do try { insert(); } catch { update(); }, and other drivers can use special syntax if available.

From an API perspective, here is a summary of my recent suggestions (I've written a lot on this thread and may not remember everything). These are now all easily implementable, we just haven't agreed on the interface.

## InsertQuery

* fields($fields, $values = NULL). If $values is non-NULL, it is an array of values and $fields is an array of keys; otherwise, $fields is assoc. This is as currently implemented.

* values($values). Can provide additional values regardless of how fields() was called. As implemented.

* orUpdate($keys = NULL). $keys is an array of field names, not assoc, no values, no expressions. Enables or-update functionality, but only for the listed keys, and only to the constant values given to fields()/values(), and only if exactly one set of values is provided. As a special case, if $keys is NULL, all keys given to fields() are updated. Yes, this is less functional than the current implementation, but much cleaner, and if you really want the more complex behavior, just call db_update() yourself if the insert fails.

* orUpdateExcept($keys = NULL). Same as orUpdate(), but keys listed in $keys are not updated. This means that orUpdate(NULL) is the same as orUpdateExcept(NULL), but so what.

* execute(). If you are inserting into a type 'serial' pkey table, returns lastInsertId(), otherwise the return value is undefined. If you call orUpdate*(), the return value is undefined. Here, I am accepting Larry's suggestion for simplification.

## UpdateQuery

* fields($fields, $values = NULL). Same as InsertQuery, though this is not currently how it is implemented.

* expressions($expressions). $expressions is an array of expressions like 'age=age+1' that are inserted literally into the SET clause of the UPDATE statement. No one has yet commented on this idea.

Now I'd like to summarize what I've learned about how fields and values need to be treated within the drivers.

The mysql driver calls queries like this:

$stmt = prepare('INSERT ... :place_0=%d, :place_1=%d, ...');
$stmt->execute(array('place_0' => '10', 'place_1' => 17));

In other words, it passes an array of arguments to the execute() method whose keys match the placeholders inserted into the text of the SQL. All it needs to do is make sure that the placeholders in the query are the same as the placeholders in the args array and that the values in the args array line up with the keys correctly.

The pgsql driver is more complicated. It does something like this:

$stmt = prepare('INSERT ... :place_0=%d, %place_1=%d, ...');
$args = array('place_0' => '10', 'place_1' => 17)
foreach ($args as $placeholder => $value) {
  switch (column_type_for_the_field($value)) {
    case 'blob':
       $stmt->bindParam($placeholder, $blobval, PDO::PARAM_LOB);
       $blobval = some_function_of($value);
       break;

   default:
      $stmt->bindParam($placeholder, $value);
      break;
   }
}
$stmt->execute();

The trick here is in the function column_type_for_the_field($value). It cannot just operate on $value alone (we might be inserting a PHP string into a varchar field or a blob field). It needs the *type* of the column this placeholder is going into. That information is in the schema, so what it needs is the *name* of column the placeholder is going into.

Some parts of database.inc make this easy but other parts may it impossible or inefficient. InsertQuery::fields() gets it right because $this->insertFields contains the name of each field and each($this->insertValues) is an array of the values for those fields. So, InsertQuery_pgsql::execute() can (and does) do something like this:

    foreach ($this->insertValues as &$insert_values) {
      foreach ($this->insertFields as $idx => $field) {
        switch ($schema['fields'][$field]['type']) {
           // call bindParam($insert_values[$idx]) appropriately based on the field type
        }
      }
    }

Compare this to (a slightly simplified version of) UpdateQuery::fields():

  public function fields(Array $fields) {
    $max_placeholder = 0;
    $this->fields = array();
    foreach ($fields as $field => $value) {
        $placeholder = ':db_update_placeholder_'. ($max_placeholder++);
        $this->fields[] = $field .'='. $placeholder;
        $this->arguments[$placeholder] = $value;
    }

The relationship of the field name to the placeholder is lost (it is only preserved in a stringified form). UpdateQuery_pgsql::execute() gets around this by storing its own copy of $fields in an overridden fields() method and then doing a bunch of array manipulation to get the relationships back.

Class DatabaseCondition is even worse (or maybe just equally as bad as UpdateQuery::fields()). The relationship between the fields being tested, the placeholders, and the values are completely obscured. If we are willing to assume that all condition columns are "normal", then DatabaseCondition::arguments() returns an array of ('placeholder' => value) that we can use for bindParam(), but if we want to be able to say something like "WHERE FUNCTION_OF(blob) = value" then we need to preserve the field name to placeholder relationship here too.

What we need is some organized, efficient, and re-usable way to create and preserve the fidelity of field names, placeholders, and values so they are available throughout the query. This probably isn't hard to come up with but I have not spent the time yet to think about the right way to do.

Crell et al: time permitting, it would be useful if you could update and extend the following overview table. It might sounds like a little bit of work, but it might be useful for documentation/blogging purposes later on. Until then, it would provide me and others a better overview of the motivations.

Feature = Cross database support (MsSQL, Oracle, etc.)
New DB layer required = YES

In general I agree with factoring all orUpdate() functionality out of InsertQuery. That way, Insert and Update both represent simply their primitive operations, with all the complex goo in MergeQuery. Good choice.

I suggest that we could even commit a PDO patch without db_merge() and add db_merge() in a subsequent patch. However, actually most/all of the API issues I'll mention below with db_merge() apply to db_update() anyway, and the simple db_merge() implementation is just try/catch, so perhaps separating it to another patch would not save us anything.

Specific comments on your MergeQuery interface:

* Specifying the key explicitly via a method call does not save you from accessing the schema. Perhaps on MySQL it does but any db with field-type-specific behavior needs the schema anyway. So, I question the redundancy of having to specify it every time. Also, it seems non-orthoganal to require it for MergeQuery but not {Insert,Update,Delete,Select}Query.

* If we do have a key() method, it needs to accept an array because tables can have multi-column primary keys.

* I still really dislike having a single array contain assoc entries for key/value and indexed entries for expressions. IMHO, it is a design flaw of PHP that the array() type allows this dual behavior and we should not build that design flaw into our API. It's just ugly and confusing.

* Please provide an example where you'd provide different values for the update operation than the insert operation, *excluding* expressions.

PHP arrays are wiz. I like the current update syntax. it's easy and intuitive. But, I find node_get_types intuitive too. I am weird.

Let's reiterate the two type of queries where core needs merge operation (by examples): variable_set where we want to make sure a given name,value pair exists in the database regardless what was there before and statistics where we want to insert an initial value and increase later. Patch currently supports these so ripping out merge is not an option. If we specify the keys then you can know --regardless of schema-- which operation to perform. Keys are definitely not needed for anything like {Insert,Update,Delete,Select}Query which executes only one kind of operation.

Hmmm. I'm sorta going in a circle:

I think I'm coming to agree that the key() method is sensible because MergeQuery() is fundamentally different from {Insert,Delete}Query(). MergeQuery says "Please update any rows matching the key KEY, and if there are not any, please insert instead." So it makes sense to specify KEY. However, given that we're specifying it and not just taking the primary key from schema, it seems logical to ask whether KEY really always needs to be the primary key. In the quoted translation above, KEY is really acting more like a WHERE condition. So perhaps MergeQuery() should have a where() method instead of a key() method.

However, the only reason that "try { insert(); } catch { update(); }" works is because we know the insert() will fail if the primary key already exists. If key() is replaced by where(), or in general can be anything *other* than the pkey, we cannot make that assumption. We would then have to do "update(); if (rows_matched == 0) { insert(); }" and, as we've established, we can't get rows_matched reliably. Therefore, key() cannot accept anything other than a pkey or unique key, and it is probably best to restrict it to pkey because all the databases' special hacks (ON DUPLICATE UPDATE, MERGE, etc.) for this purpose will require it.

So, the only advantage to requiring the key() method is avoiding the dependency on the schema for those databases that do not otherwise require it anyway. Why is that a desirable goal? The only reason I can think of is performance. Not having key() means the query needs to call a method to retrieve the schema for the table and (the first time that table is touched per request) unparse the serialized schema structure. Having the key() method means that we're explicitly requiring a method call too, though we do not have to unserialize the schema. So the performance gain is not unserializing each db_merge()d table's schema once per request.

The disadvantage is that we're introducing redundant information (== more difficult to maintain, less reliable) into our code. We have the info we need in the schema, and we're not using it.

Whatever. I'm not so motivated to argue about it.

In #147, I asked:

* Please provide an example where you'd provide different values for the update operation than the insert operation, *excluding* expressions.

If there is not a good answer to that question, there is no reason that MergeQuery::update() needs to support assoc key/value pairs instead of a simple list of fields to update to the values given to values().

Also, no one has offered a disadvantage of the expressions() method I'm proposing for UpdateQuery and MergeQuery. The advantage is that is simplifies the API from a user's point of view because then fields() and values() take *either* assoc arrays *or* indexed arrays of constants, not a mixed assoc/index array, while expressions() always takes an indexed array of expressions. Consistent == simpler.

@crell: "It's not really redundant since the key would have to be placed in fields() anyway if it's not in key(), so it's the same amount of data, just passed differently."

Okay, I'm convinced, db_merge()->key() is fine. It does need to support multi-column keys, but the method name should still be key() since the multiple columns define a single key for the query.

Subscribing

Lost in the comment clutter has been my suggestion about query tags. From #111:

As for the db_rewrite_sql() case, basing it purely on base table isn't sufficient either since (e.g.) some node queries should involve all nodes and some should involve those the user has permission on. I was thinking what we need are query tags. So node_query_alter() would document that it operates on all node queries having the "node-access" tag by doing the appropriate joins, and callers that want that behavior do db_select('node', ...)->tag('node-access')->etc().

Without this feature, node_query_alter() will have to add node-access joins to EVERY node query, which is incorrect behavior. Today, a module author decides whether to do that by calling db_rewrite_sql() or not.

In this particular example, I might support tag('no-node-access') so the access joins happen by default, but that is a separate issue from the feature of query tags in general.

Thoughts?

These query tags are what we called 'hints' and are currently sent as the 4th parameter of db_rewrite_sql(). they are seldom used, but as barry said should be used more. So, please keep this feature in some fashion.

I just thought of another snafu.

This new db layer (at least for some dbms's) requires schema information about tables for (at least) non-SELECT queries. This makes it impossible to create a local temporary table and INSERT INTO/UPDATE it, because the temporary table will not be exported by any module's hook_schema.

Actually, I now also realize that while we have db_create_table() we do not have db_create_temporary_table(), and probably should.

At the moment I'm thinking that db_create_temporary_table() can just add the schema for the table it is creating to a temporary schema to be returned as part of the normal schema for the current request (but not saved in the schema cache) so that INSERT/UPDATE queries can access it. I'm not yet sure if this is sufficient.

Re: #157, I'm not sure what to do about

   abstract function queryTemporary($query, Array $args, $tablename);

which performs CREATE TEMPORARY TABLE SELECT. The function is not passed a schema structure to add to the global schema. If the table is only queried by SELECT there is no problem but if the code wants to INSERT into it, there is.

Well, both MySQL and PostgreSQL supports CREATE TEMPORARY TABLE ... AS SELECT so that's why we Drupal had no need for more elaborate definitions. The supposed use of temp tables are to create a temp table and the select from it... More than that -- do we need to solve everything in the scope of one patch?

My point is that with the patch as it currently exists, on pgsql it will be *impossible* to insert into/update a temporary table using db_insert() or db_update(). db_query() will still work (because it does not try to inspect the scheme) provided you do not use any column types that require special treatment. That's probably an acceptable restriction for now, but it is worth noting.

FYI, over the weekend I also did some work on MergeQuery, making the generic implementation in database.inc work, and fixed a few other glitches around the code. I made all the pgsql tests, including the new merge tests, work again. I am part-way through making multiple blobs per query work (a limitation of the current pgsql code is that they do not) and discovered entertaining ways to crash Apache in the process. :-)

It looks like storing a connection's lastStatement is a permanent requirement, not something we can remove after taking out db_affected_rows(), because it is the only way to get the error code from (e.g.) a db_insert() statement. Well, possibly we can use the DATABASE_RETURN_STATEMENT option, though InsertQuery does not currently support it.

Of course, Larry new all of this already because he had to svn update the changes before committing... :-)

You can set it to CNR but it also absolutely needs work still.

I'm not sure I agree with not sending queries with no tags through hook_query_alter(). Some modules (e.g. devel) might want to operate on all queries, for example by logging them, caching them, checking them for efficiency or correctness, or who knows what.

"- __toString --> why not a method called getSql or similar. seems more obvious than using __toString. is it just a keystrokes thing, or was there another consideration? the fact that we need comments like:"

__toString() is a PHP magic method. It allows you to manipulate the object as a string in string contexts like when concatening you can use something like 'foo' . $object . 'bar', instead of 'foo' . $object->getSql() . 'bar'.

(subscribing)

Where can we find the patch for review?

Quick question: Since $db_url has been converted to $databases I wanted to make sure the new DB layer will still allow to alter this array dynamically at runtime. I'm doing this in Drupal vB (example: drupalvb.inc) and Migrator modules, so users are able to setup an additional connection for the remote database in the module settings and must not alter settings.php. Will that still be possible?

Also, shouldn't $db_prefix be moved into the $databases array? Why should we force users to have the same table prefix in all databases? I'm currently hacking around this limitation in both modules (see example above).

$db_prefix should be DB specific, only a global definition is often too limiting.

Another issue: In method openConnection() there is already dummy code for error handling, like in db_connect() currently. In Drupal vB (#253076) I have to fork mysql and mysqli connection functions from db_connect() to test a database connection, because db_connect() immediately displays a maintenance page and there is no way to prevent this. If this was supported by the new db layer, install.php may benefit from that, too.

Currently, there is no way to test a database connection. db_set_active() invokes db_connect() (in D5/6) and if the connect fails, the script exits displaying a maintenance page. That means, even if your primary database connection is perfectly valid and working, but an additional one is not, Drupal displays nothing else than a maintenance page stating that it is unable to connect to the database (which is not true).
My suggestion is to (a) either not hard-code, respectively, tie the maintenance page into openConnection(), or (b) add another method testConnection(), which could be used to test a database connection during installation of Drupal and by module developers that are doing stuff like me.
So, in short: db_connect() should be clean, just doing no more and no less what the function name implies.

subscribing

Only Crell would make an issue with "The Next Generation" in the title. Subscribing!

Subscribing

I just fixed a few glitches in database.inc and updated the pgsql driver to work again. The pgsql driver is still kind of a mess. I duplicate the bindParam() loop for the insert and update queries in very slightly different forms, and I have to essentially duplicate some code from the parent InsertQuery and UpdateQuery classes with some changes to support the bindParam() approach.

I'm thinking that the "clean" approach would be to move the bindParam() logic into DatabaseConnection_pgsql::runQuery() which could then convert its $args array into a bunch of calls to bindParam(), all in one place. For this to work, runQuery() would need to take a Query object, not an SQL string. I know that db_query() passes SQL to runQuery(); we could handle that by having db_query() pass a class SqlQuery extends Query object which wraps the SQL. But I'm not yet sure I love this approach, either.

The tests all pass except for generating some exceptions when db_create_table() is called with an empty table structure. Oddly, the tests pass anyway, so clearly the table is being created somehow. Not sure what is going on yet.

subscribing

diverting my attention here.

The incredible shrinking patch! (Note: I just rolled it against HEAD.)

Uh huh. I did a few cvs up and now it's indeed a lot smaller.

There's several tests that don't apply against so probably needs a re-roll.

More importantly, install is broken:

Fatal error</b>: require_once() [<a href='function.require'>function.require</a>]: Failed opening required './includes/registry.inc' (include_path='.:/usr/share/php:/usr/share/pear') in <b>includes/bootstrap.inc</b> on line <b>1396</b><br />

edited to fix crazy typos.

I'm looking at this today.

Crell: some test hunks don't apply.

I can't actually test tests because I can't install drupal with the patch applied ;)

tracker.test
search.test
drupal_web_test_case.php
profile.test
xmlrpc.test

Some things I noticed today in the SVN version:

- includes/registry.inc is missing
- Test fails: Select: Returned the correct number of rows at [dbtest.test:1004]
- Test fails: Query altering: Returned the correct number of rows at [dbtest.test:1177]
- QueryConditionInterface::condition() does not seem to describe DatabaseCondition::condition(), and the latter is not documented.

Let's see this one. grep ^=== pdo.patch |grep test only shows dbtest. registry.inc is not removed this time.

Edit: it's a royal pain in where the sun never shines, rolling a patch this size...

Rolling...

Patch still applies. Some hunks have offsets:

patching file includes/cache.inc
Hunk #1 succeeded at 29 (offset 1 line).
Hunk #2 succeeded at 100 (offset 1 line).
Hunk #3 succeeded at 175 (offset 1 line).

patching file includes/common.inc
Hunk #1 succeeded at 2453 (offset 4 lines).
Hunk #2 succeeded at 2631 (offset 4 lines).
Hunk #3 succeeded at 2679 (offset 4 lines).
Hunk #4 succeeded at 3028 (offset 4 lines).
Hunk #5 succeeded at 3072 (offset 4 lines).

patching file includes/menu.inc
Hunk #1 succeeded at 1807 (offset 5 lines).
Hunk #2 succeeded at 1885 (offset 5 lines).
Hunk #3 succeeded at 2206 (offset 5 lines).

Installation and use:

Drupal-PDO installs nicely.

1. Create an article fails:
   

   PDOException: SELECT f.fid AS fid, f.format AS format, f.module AS module, f.delta AS delta, f.weight AS weight FROM {filter_formats} AS f ORDER BY weight ASC - Array ( ) SQLSTATE[42S22]: Column not found: 1054 Unknown column 'f.fid' in 'field list' in /var/www/drupal/drupal7/testpdo/includes/database.inc on line 251
   

2. Create a page fails:
   

   PDOException: SELECT f.fid AS fid, f.format AS format, f.module AS module, f.delta AS delta, f.weight AS weight FROM {filter_formats} AS f ORDER BY weight ASC - Array ( ) SQLSTATE[42S22]: Column not found: 1054 Unknown column 'f.fid' in 'field list' in /var/www/drupal/drupal7/testpdo/includes/database.inc on line 251
   

3. Administer input formats fails:
   

   PDOException: SELECT f.fid AS fid, f.format AS format, f.module AS module, f.delta AS delta, f.weight AS weight FROM {filter_formats} AS f ORDER BY weight ASC - Array ( ) SQLSTATE[42S22]: Column not found: 1054 Unknown column 'f.fid' in 'field list' in /var/www/drupal/drupal7/testpdo/includes/database.inc on line 251
   

4. Create second user works!
5. Access Actions settings works!
6. Access Administration theme settings works!
7. Access Clean URLs settings works!
8. Access Date and time settings works!
9. Access Error reporting settings works!
10. Access File system settings works!
11. Access IP address blocking settings works!
12. Access Image toolkit settings works!
13. Access Logging and alerts settings works!
14. Access Performance settings works!
15. Access Site information settings works!
16. Access Site maintenance settings works!
17. Modifying blocks settings works!
18. Access Modifying menu settings works!
19. Access Installing core module (contact) works!
20. Access Uninstalling core module (contact) works!
21. Access Creating new content type works!

Not being able to test content additions sucks, but on this local machine this feels nice.

Forgot to set CNW

The filter_formats.fid column doesn't exist.

mysql> describe filter_formats;
+--------+--------------+------+-----+---------+----------------+
| Field  | Type         | Null | Key | Default | Extra          |
+--------+--------------+------+-----+---------+----------------+
| format | int(11)      | NO   | PRI | NULL    | auto_increment | 
| name   | varchar(255) | NO   | UNI |         |                | 
| roles  | varchar(255) | NO   |     |         |                | 
| cache  | tinyint(4)   | NO   |     | 0       |                | 
| weight | tinyint(4)   | NO   |     | 0       |                | 
+--------+--------------+------+-----+---------+----------------+

These lines are adding 'filters' table columns while querying 'filter_formats':

-    $query = 'SELECT * FROM {filter_formats}';
+    $query = db_select('filter_formats', 'f');
+    $query->addField('fid', 'f', 'fid');
+    $query->addField('format', 'f', 'format');
+    $query->addField('module', 'f', 'module');
+    $query->addField('delta', 'f', 'delta');
+    $query->addField('weight', 'f', 'weight');
+    $query->orderBy('weight');

It's on filter.module filter_formats() function.

recidive,

I am not sure what you are trying to tell me. As it is, the patch needs work since important functionality (creating content) stops working.

there are two issues.

The missing column and whether the code needs work. I think keeping the code needs work flag is pretty accurate.

As a test I added the fid, module, and delta columns which Drupal complained about and I am now able to access the Input Filter admin page as well as add content.

Administering the Input Filters is broken. (Admittedly, this is a hack.)

Adding content works.

The names for the filters do not show up, and editing the filters doesn't work properly.

Something funky happening in the interaction between the filters and filter_formats tables.

On a whim I dropped the whole filters table. Only leaving the filter_formats table and Drupal didn't die on me.

So I am assuming that filter_formats is supposed to supercede filters with this patch.
I am also assuming that the move from one table to another is incomplete.

Sorry, I didn't want to change to CNR.

Try applying this patch after the one in #199.

If it works, can someone commit this to SVN and reroll the patch?

Patch didn't get attached. Here it goes.

Delivered.

This patch is the patch from 199 as well as recidive's patch from 205 included. Viva la CVS.

I don't know why my patch is about 90KB smaller than the patch on 199.

With the combined patch of chx and recidive Drupal installs nicely. All the errors that popped up previously are gone.

Putting this in my tracker so that I can fix up some comments; mostly double spaces between sentences, sentences without full stops, etc. Nothing major.

Your patch is smaller because it's missing new files added. You need to do a few complex things to make CVS include new files in a patch, so it's a common mistake with these large patches. The same thing happened to me in the .schema -> .install patch, so I suspected it immediately when the size didn't match.

$ grep "=== added" pdo_5.patch 
=== added file 'includes/schema.inc'
=== added file 'includes/schema.mysql.inc'
=== added file 'includes/schema.pgsql.inc'
=== added directory 'sites/all/modules'
=== added directory 'sites/all/modules/dbtest'
=== added file 'sites/all/modules/dbtest/dbtest.info'
=== added file 'sites/all/modules/dbtest/dbtest.install'
=== added file 'sites/all/modules/dbtest/dbtest.module'
=== added file 'sites/all/modules/dbtest/dbtest.test'

These are the files that must be re-added.

I can reroll, I think.

This one seems to be the right size. It's #206 with the added files, or rather #199 with the #205 change.

Getting it to include sites/all/modules/dbtest was a bit of a challenge.

no patch

Trying again.

And again.

With this patch, simpletest users appear to be created in the main database instead of the prefixed one (or that's as far as I've got tracking down a lot of test failures).

Edit: Never mind, I didn't understand how SimpleTest prefixes tables.

Problem looks simple. SimpleTest changes the global $db_prefix table during the test. The user is registered through a remote HTTP request. Since this remote request does not share a persistent scope with the original request, the $db_prefix is no longer changed when the user is registered.

This is a log file output I got from the relevant functions:

34d8e39c: Starting test, client is at 92.192.x.x
34d8e39c: Setting prefix to simpletest256708
: user_save called, saving user in d7test_, client is at 208.113.x.x

As you can see, the user is saved by remote HTTP request, losing the global scope, and resetting db_prefix.

To fix this, you need to somehow remember that you are in a test across multiple requests. You can either do this via a cookie or a persistent variable (since you're unlikely to be doing other things at the same time on a testing site).

Sorry if some of this is obvious or supposedly fixed; I'm new to SimpleTest and had to basically investigate from scratch.

subscribing

This patch keeps 'fields' used as a synonym for 'column' - there's an old issue against schema API to change 'field' to 'column' to avoid confusion with fields in core - I still think it's a valid issue: http://drupal.org/node/190162

Very interesting, thanks. I am a PostgreSQL fan.

In Drupal 6.2 I am amazed by some unsigned int together with SERIALs which are int4 (or 8) don't remember.
As a result the database has to cast during JOINs, which results in sequentiel access.

I can only recommand you to choose one single INT for all integer values.
This will boost JOINs and produce better results.

Don't think in term of database storage space ... it costs nothing nowadays.
Think in term of queries.

The database will always adapt better to a situation where its own types are used.
SQL92 is there, so there is no need to create custom types IMHO.

Will test anyway. Great project.
I will log queries and read query plans to tell you if PostgreSQL
will cast / sequential access or not.

Kind regards, JMP

Subscribing.

@JMP: Your comment in #223 sounds like an important point but is not directly tied to this issue. Please create a separate issue here at d.o regarding the performance penalty of inconsistent types for pgsql joins.

The patch no longer applies for me.

Just a heads up on PostgreSQL installation.

I've just debugged the install on PostgreSQL and passed on my findings to Crell. Turns out the install code is passing $args == array(1) with the CREATE TABLE statement when creating the variable table. This in turn breaks the PostgreSQL PDO driver for some reason. Removing the array(true) from the update_sql() function results in the table creation being successful.

We appear to have a transaction error following on after this so some more to do still.

Following on from #138 above where it is stated that the following sequence is the way to get a merge to work in a DBMS without the MERGE query:

try {
  insert();
}
catch (Exception e) {
  update();
}

While this works in code it seriously breaks transactions. The insert failure will abort any surrounding transaction (i.e any transaction started before the call to db_merge()). So this method of doing can not be an option for us.

Provided db_merge() isn't used in heavy write scenarios it's safe to implement it as:

if ($num = db_result(db_query("SELECT COUNT(*) FROM $table WHERE key = %d"))) {
  update();
}
else {
  insert();
}

As annoying as this method is, it works and will get us moving forward with db_merge() not breaking transactions.

PostgreSQL can avoid the transaction breakage and allow us to use the insert()-or-update() method with savepoints. This will only work with versions >= 8.0. I've tested savepoints and they will allow the following sequence of queries:

pdo=# BEGIN;
BEGIN
pdo=# INSERT INTO variable (name, value) VALUES ('foo1', 'bar1');
INSERT 0 1
pdo=# SAVEPOINT db_merge;
SAVEPOINT
pdo=# INSERT INTO variable (name, value) VALUES ('foo1', 'bar1');
ERROR:  duplicate key violates unique constraint "variable_pkey"
pdo=# ROLLBACK TO SAVEPOINT db_merge;
ROLLBACK
pdo=# UPDATE variable SET value = 'bar2' WHERE name = 'foo1';
UPDATE 1
pdo=# COMMIT;
COMMIT

What will be the minimum postgresql version for D7?

Savepoint statements can't be run unless you're in a transaction already:

pdo=# SAVEPOINT db_merge;
ERROR:  SAVEPOINT may only be used in transaction blocks

http://www.postgresql.org/docs/8.0/static/sql-savepoint.html

Since I haven't looked into the code of the whizzbang new DB layer enough... Will we have an insideTransaction() function available to us? PDO may not implement one but we sure can put a static var in to track this (though I imagine this should be possible to override for some silly DBMSs).

@sammys: there's an issue here to up the minimum postgresql version, trivial patch but three months stale.

edit: forgot the actual link #248205: PostgreSQL surge #7: Require PostgreSQL 8.3

@Crell, can you clarify why this patch breaks more than half of tests?

It would help testers if this patch gets re-rolled more often.

Changing to CNW because it doesn't apply anymore.

I already have a checkout from SVN and am running tests on it. Just so you get a sense:

Block module tests:

Without the patch:

58 passes, 0 fails, 0 exceptions

With the patch:

38 passes, 37 fails, 0 exceptions

How can I help with code?

Can you give me commit access to SVN so I can help with cosmetic changes?

Posting patches against SVN here can cause confusion, i.e. people would try to apply the patch against their clean HEAD checkout.

So what is the process? I'm willing to help.

I tracked down the tests problems and discovered it is missing the "User-Agent with db prefix" hack originally set on drupal_set_active(), I added this code to Database::openConnection() as Database::setActiveConnection() is only called when switching databases, not when the first database connection happens (not sure if it's the place though).

All block module tests failures are gone. Patch against SVN code attached.

This patch is against PDO SVN code. Doesn't try to apply/test this on a clean Drupal HEAD checkout.

Edit by ChX: I gave recidive SVN access and he checked this and a bit more in.

Question: Since the eventual outcome of this work will be a patch against the CVS repository, I suppose that the SVN repository must be kept up to date with changes to CVS? session.inc is put in a conflicted state by a change earlier today. This patch (for SVN) merges in revision 1.49 of session.inc. There are other merges, but this is the only conflicting one.

So, I did a round of testing today. MySQL 5.0.51a, PHP 5.2.4, Linux.
Here are the results:

HEAD without the patch: 4775 passes, 24 fails, 0 exceptions
PDO patch SVN checkout: The tests did not complete. After 3708 passes, 38 fails, 0 exceptions simpletest stops with an uncaught exception:

Fatal error: Uncaught exception 'PDOException' with message 'UPDATE {search_index} SET score = score + ? WHERE word = ? AND sid = '?' AND type = ? - Array ( [0] => 1 [1] => ipsum [2] => 1 [3] => _test_ ) SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens' in [..]/pdo/includes/database.inc:251 Stack trace: #0 [..]/pdo/includes/database.inc(271): DatabaseConnection->runQuery('UPDATE {search_...', Array, Array) #1 [..]/pdo/includes/database.inc(2756): DatabaseConnection->query('UPDATE {search_...', Array, Array) #2 [..]/pdo/modules/search/search.module(575): db_query('UPDATE {search_...', 1, 'ipsum', 1, '_test_') #3 [..]/pdo/modules/search/search.test(40): search_index(1, '_test_', 'Ipsum') #4 [..]/pdo/modules/search/search.test(29): SearchMatchTestCase->_setup() #5 [..]/pd in [..]/pdo/includes/database.inc on line 251

And here is a list of tests that pass in HEAD but fail in the PDO SVN checkout:

Comment - comment functionality: 16 fails
Comment posted. Other comment.test 64 postComment
Comment id found. Other comment.test 64 postComment
Reply found. Other drupal_web_test_case.php 274 testCommentInterface
Found the requested form at Other comment.test 277 drupalPost
Found the Preview button Other comment.test 277 drupalPost
Failed to set field subject to simpletest_taJS Other comment.test 277 drupalPost
Failed to set field comment to simpletest_TkPr Other comment.test 277 drupalPost
Found the requested form at Other comment.test 279 drupalPost
Found the Save button Other comment.test 279 drupalPost
Failed to set field subject to simpletest_taJS Other comment.test 279 drupalPost
Failed to set field comment to simpletest_TkPr Other comment.test 279 drupalPost
Comment posted. Other comment.test 69 postComment
Comment id found. Other comment.test 69 postComment
Modified reply found. Other drupal_web_test_case.php 274 testCommentInterface
Link to the 2 comments exist. Other drupal_web_test_case.php 274 testCommentInterface
Page two exists. %s Other drupal_web_test_case.php 274 testCommentInterface
DBlog - 1 fail
DBLog contains 2 records for simpletest_vNsv that now have uid = 0 Other dblog.test 165 doUser

The other tests that fail do fail on HEAD as well.

And now results for Postgres 8.3.3. First of all, in current HEAD simpletests on Postgres for me simply stop after 6 out of 70 tests (at Categorize feed item functionality.) without any error message, the batch just doesn't continue. I will file a seperate bug report for this.

And on install in current HEAD I get several error messages, whereas in the PDO SVN checkout installation goes smooth without any errors! Nice, eh? ;)

In the PDO SVN checkout simpletests go better than in current HEAD, I get 1010 passes, 407 fails, 12 exceptions until it stops as well. Detailed results are here

Fatal error: Uncaught exception 'PDOException' with message 'UPDATE {search_index} SET score = score + ? WHERE word = ? AND sid = '?' AND type = ? - Array ( [0] => 1 [1] => ipsum [2] => 1 [3] => _test_ ) SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens' in [..]/pdo/includes/database.inc:251 Stack trace: #0 [..]/pdo/includes/database.inc(271): DatabaseConnection->runQuery('UPDATE {search_...', Array, Array) #1 [..]/pdo/includes/database.inc(2756): DatabaseConnection->query('UPDATE {search_...', Array, Array) #2 [..]/pdo/modules/search/search.module(575): db_query('UPDATE {search_...', 1, 'ipsum', 1, '_test_') #3 [..]/pdo/modules/search/search.test(40): search_index(1, '_test_', 'Ipsum') #4 [..]/pdo/modules/search/search.test(29): SearchMatchTestCase->_setup() #5 [..]/pd in [..]/pdo/includes/database.inc on line 251

I'm a noob with prepared statements and PDO, but is that error supposed to indicate that UPDATE {search_index} SET score = score + ? WHERE word = ? AND sid = '?' AND type = ? contains a different number of placeholders than Array ( [0] => 1 [1] => ipsum [2] => 1 [3] => _test_ ) contains values?

In the test results I saw a valid email address simpletest@test.com. We should use simpletest@example.com here.

Irrelevant to this issue though. I've made a separate patch:

#285184: contact.test uses bad email domain test.com

Is anyone working on an Oracle driver? I've got 4.7.2 on Oracle 10g and would eventually like to update to a more modern version of drupal.

Re: #241

According to SQL2003, null order is explicitly left to the vendor to choose how they want to implement it....

"If <null ordering> is not specified, then an implementation-defined is implicit. The implementation-defined default for <null ordering> shall not depend on the context outside of <sort specification list>."

So don't point your finger exclusively at the DB vendors, since the standards body also failed to define the standard. At least most DBs support nulls first|last SQL syntax (which is part of the standard).

This is all Crell's work, I am just rolling here.

Call to undefined function db_decode_blob() in cache.inc - looks like only one of them gets removed in the patch.

I hacked it out and I'm running tests again now. Quick re-roll?

edit: also getting DBLog functionality: 308 passes, 1 fail, 0 exceptions
Module list functionality: 53 passes, 4 fails, 0 exceptions
Database placeholders: 108 passes, 4 fails, 0 exceptions

Fails:

[numeric] Got 0 (1.00) after inserting invalid value 1x
[numeric] Got 0 (4.40) after inserting invalid value 4.4 OR 1=1
[numeric] Got 0 (9.00) after inserting invalid value 9 9
[numeric] Got 0 (44.00) after inserting invalid value 44-66

Tables matching "aggregator" found.
Tables matching "languages" found.
Tables matching "locale" found.

DBLog contains 2 records for simpletest__6Cq that now have uid = 0

edit again - this was with #252, trying with #251 now.

Well. #251 seems to be the better patch. I only get the one dblog failure and 4 system failures with it, and no undefined function in cache.inc.

Which one are we supposed to review?

#252 is about 80 kB smaller than #251. Sure that all files were diffed properly?

Edit: Looks like #252 does not delete certain files that #251 deletes.

diffing grep '^===' from patch #251 and #252.

1d0
< === modified file 'includes/batch.inc'
11d9
< === removed file 'includes/install.mysql.inc'
20,22d17
< === removed file 'includes/tests/cache.test'
< === removed file 'includes/tests/database.test'
< === removed file 'includes/tests/form.test'
24d18
< === removed file 'misc/help.png'
36d29
< === removed file 'modules/simpletest/simpletest.test'
39d31
< === removed file 'modules/statistics/statistics.test'
44d35
< === removed file 'modules/tracker/tracker.test'
49,52d39
< === removed directory 'profiles/expert'
< === removed file 'profiles/expert/expert.profile'
< === removed file 'scripts/password-hash.sh'
< === removed file 'scripts/run-tests.sh'

@Crell, in that case #252 still needs a re-roll for the cache.inc fatal error - it's just removing that one line.

I manually confirmed that aggregator installs properly, got a feed into it etc., so these look like test framework issues in system.test at least. I've already cleared my HEAD out but will see about a bit closer look later on.

const ARGUMENT_BOUND seems to never be used.

Tracked down two failures:

system.test calls http://api.drupal.org/api/function/simpletest_get_like_tables/7
Line 501 of simpletest.module

post a reply to a comment generates:

SELECT MAX(thread) FROM {comments} WHERE thread LIKE '?.%%' AND nid = ? - Array ( [0] => 01 [1] => 1 ) SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens in /home/catch/www/drupal7/includes/database.inc on line 272
Line 696 of comment.module

Also line 214 of dblog.test
db_query('SELECT wid FROM {watchdog} WHERE uid = 0 AND wid IN (%s)', implode(', ', $ids));

I think that's all the simpletest failures (the old db placeholders tests need to be removed or rewritten since we'll no longer use those placeholders).

Subscribing.

/**
 * The following utility functions are simply convenience wrappers.
 * They should never, ever have any database-specific code in them.
 */

..but then it progresses to put some pgsql-specific code in _db_query_process_args(), manually typecasting integers and floats to integers and floats, which is only necessary for pgsql, according to the inline comment. Would there be a way to move this to a pgsql-specific location and avoid the regex on other database types?

Here comes the patch. bzr rocks in rolling patches like this.

All simpletest errors fixed (only testing on MySQL) - except for module_list and I agree simpletest_get_like_tables() is just strange. Reading through the code now.

Subscribing

subscribe

subscribing

Patched version works as advertised -- nothing breaks; tests pass. I am still trying to walk through the code and comments to figure out exactly how.

Some initial notes:

database.inc

function defaultOptions() 
  -- Missing definition of 'already_prepared' option.

function prepareQuery()
  -- Do we not need a parameter to override the static cache of the query?

function makeSequenceName($table, $field)
  -- Not documented.

function runQuery()
  -- Do we not need a parameter to override the static cache of the query?
  -- ? placeholders documentation?
  -- Documentation refers to 'already_prepared' above.  Definition needs replication.
  -- Internal comments on the various sections would help -- esp. the switch case.
  
function runBoundQuery()
  -- Extra space in doxygen comments.
  -- @param $stmt should be $statement or $query, unless there is good reason.
  -- Internal comments on the various sections would help -- esp. the switch case.
  
function query()
  -- Purpose of this function is not clear.  Is this a procedural wrapper for the PDO functions?
  -- The whole group of function here (query, insert, delete) -- which are meant to be extended by db-specific includes, should be noted.

function openConnection()
  -- @params are not defined.

class DatabaseStatement
  -- http://us.php.net/manual/en/ref.pdo.php link outdated.  Use http://us.php.net/pdo ?
  -- public $dbh variable name is ambiguous -- assuming inheritance from PDOStatement.

And I'm only about 15% through the database.inc file.

My opinion -- not being familiar with OO code style -- is that this may be RTBC so we can really start testing, but we may need to be able to roll back the patch.

I do not believe we need to override the static cache of prepared statements.

I'm not yet very familiar with the PDO changes... only this sentence wonders me a little bit. I know that we often have issues in ColdFusion with prepared statements and this makes us often to hard restart the ColdFusion services at all as prepared statements cannot be cleared in CF by hand. This is unbelievable bad and I'm not sure how to clear PHP-PDO cache, but I hope we could do better in Drupal then Adobe does in CF.

We changed often in such situations from SELECT * ... to naming the columns or simply added a blank to a as only this makes CF to recompile the statement :-(. Nevertheless this solved the issues in CF we are restarting the services to not add stupid blanks to SQL statements. But this should all be easier... There are many statements using * in Drupal and if the DB layout changes you need to clear/overwrite the prepared statements cache or the new columns are invisible until the cache is cleared.

I've looked through about 70% of this patch now, looks like Ken has already picked up most of my initial concerns.

Whilst I think the db_insert() etc. syntax will probably take a little getting used to, looking at the converted queries it's going to be much more readable - since the fields and values sit right next to each other in the array - much easier to see which maps to which when there's lots of variables to insert.

Moving $db_url to the $databases array is also a nice change, and likely to reduce mess ups when new users have to manually edit it.

There's commented stuff in install.php- calls to db_set_active and install_already_done - these either need deleting, or @TODOs or something.

Otherwise this looks RTBC to me - the tests and phpdoc are verbose, I'm not really qualified to comment on the actual PDO implementation and abstraction layer, but I think we're past that point now since that's been looked at in depth by people who are.

@Crell: This gives me an idea why PDO is not faster then the older drivers :-). Bad thing... prepared statements should be cached over page requests as the time consuming process of binding data types and validating them to DB fields gives "often" only a win over multiple page requests. Thank you for the explanation.

#275 gave me the following:

An error occurred. /dbtng/batch?id=2&op=do <br /> <b>Fatal error</b>: Uncaught exception 'PDOException' with message 'SELECT watchdog.wid AS watchdog_wid FROM {watchdog} AS watchdog WHERE (uid = :db_condition_placeholder_584) AND (wid IN ()) - Array ( [:db_condition_placeholder_584] =&gt; 0 ) SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '))' at line 4' in /home/mike/public/dbtng/includes/database.inc:414 Stack trace: #0 /home/mike/public/dbtng/includes/database.inc(2317): DatabaseConnection-&gt;runQuery('SELECT watchdog...', Array, Array) #1 /home/mike/public/dbtng/modules/dblog/dblog.test(218): SelectQuery-&gt;execute() #2 /home/mike/public/dbtng/modules/dblog/dblog.test(165): DBLogTestCase-&gt;doUser() #3 /home/mike/public/dbtng/modules/dblog/dblog.test(39): DBLogTestCase-&gt;verifyEvents() #4 /home/mike/public/dbtng/modules/simpletest/drupal_web_test_case.php(274): DBLogTestCase-&gt;testDBLog() #5 /home/mike/public/dbtng/modules/simpletest/s in <b>/home/mike/public/dbtng/includes/database.inc</b> on line <b>414</b><br />

Crell fixed this in r642. Any chance you could re roll the patch?

system and node module have hunk failures as of this evening, needs a re-roll.

New one applies cleanly and passes all non-failing tests on MySQL. edit: except the four fails on system.module, which are a simpletest oddity, and ought to be handled in a different issue.

It seems to be quite a while since we had a positive review on postgres, IMO there should be one more on that in case of any regressions since the last one.

I installed postgres, installed drupal, tried to run tests - and they don't work on a clean install of HEAD with postgres, so pretty useless for comparison.

However, a bit of manual clicking, and I get Call to undefined function module_load_all_includes() in bootstrap.inc - this is in drupal_get_schema() - which is called from InsertQuery_pgsql { in database.pgsql.inc

We now defer cache_set until a full bootstrap. merge will need documentation: do not fire during hook_boot, not guaranteed to work because it uses schema.

With this new patch, install works fine on postgres. I got an error in search.test - but as noted, I can't even get to search.test in postgres with HEAD since simpletest is throwing exceptions and timing out etc.

So, any remaining test failures on postgres I think we should fix in a follow-up patch - since this is already a massive improvement on the status quo anyway.

Based on this having extensive review so far by numerous people and no caveats for quite some time - I'm marking RTBC.

I think Crell cross-posted.

minor note - the code example in the doxygen at the top of database.inc has been wrong since 4.7.x - there is no n.body field. see: http://drupal.org/node/293558

https://4si.devguard.com/svn/public/pdo/includes/database.inc

Please re-roll the patch as it's broken at about 8-9 places now.

I was able to apply the patch after some manual adjustments and it applied cleanly. But then I got an error about the Class PDO not available and I figured out it was because my php 5.2.6 wasn't compiled with it, so recompiled and got passed that.

Now though I get "Fatal error: Interface 'Countable' not found in /var/www/www.astro.nu/htdocs/includes/database.inc on line 2663" and I guess it's because something else not being compiled into php, but what? Yes SPL, I figured that out as well. Previously I found out that D7 now also demands php to be compiled with json support and maybe I have missed something but I haven't been able to see these new requirements listed anywhere? I just mean, it will probably save a lot of support fuzz if it was as it probably isn't uncommon that php is compiled w/o these extensions. Just a though.

I CVS upped hoping to attract some comitter attention :)

Ok some feedback,

latest patch apply cleanly and a fresh install with a remote (not localhost) mysql 5.0.60 db went fine. Only problem I have here is that D7 still cannot send mail due to my mailserver also is located on a separate host - but that's no blaim on this patch I think.

I will test furhter and will record back if I notice anything but as before this patch it wasn't even possibly to install D7 I would say it's RTBC - at least that would invite more ppl to start testing D7

If it hadn't been for this patch I probably had dropped it, but luckily I found it on the top of the issue quee ;-)

Now I will try t build a site on D7 which doesn't need so much extra, making it an excellent candidate.

This patch has come a long way! Good work, everyone.

I performed a static (human read-only) code review of the latest SVN code. Most of my comments are minor tweaks and some are just questions. The really important ones are marked HIGH. Since there are a couple HIGH (including at least one security vulnerability), I've marked it CNW again (sorry).

If a pgsql (or mysql) test run is needed, let me know.

# database.inc

## Opening docs

* Extra argument to db_insert() in example:
db_insert('my_table', 'node')->fields($fields)->execute();

## class DatabaseConnection

* makeSequenceName(): Why "@param unknown_type $field"?

* public function setLastInsertInfo() is obsolete.

* query() is public and calls runQuery() which is protected. InsertQuery (and others) call runQuery() directly, which means query() cannot ever really perform any additional functionality. So why not just rename runQuery() to query() and make it public?

* mapConditionOperator(): Why must $operator be case-sensitive? SQL is not.

## class Database

* parseConnectionInfo(): Is it possible that $databases[$index][$target]
will not be cleanly 0-indexed? If so, we should use

$databaseInfo[$index][$target] = $databaseInfo[$index][$target][array_rand($databaseInfo[$index][$target])];

* openConnection(): "// TODO. error handling."

## class InsertQuery

* protected $delay is not used in this class, should be removed

* useDefaults(): What is the difference between specifying a column to useDefaults() and not specifying it at all? Perhaps the doc should say so.

## class MergeQuery

* I continue to object to the non-orthogonal treatment of update() and updateExcept(). What is the compelling use case for update() to take a list of values separate from those given to fields()? I suspect this interface was chosen because MySQL's ON DUPLICATE clause allows it, not necessarily because it is worth the confusion. :-)

## class SelectQuery

* HIGH: addField(): Schema API functions accept ($table, $field), this function accepts ($field, $table). This is HIGH because one we release this API, we'll be stuck with the order forever.

## Schema API functions

* HIGH: db_type_placeholder(): Using %s without quotes for numerics allows SQL injection attacks. This bug has been fixed in HEAD and 6.x, so this patch should not re-introduce it. I *thought* a unit test for this fix was committed.

# database.pgsql.inc

## class DatabaseConnection_pgsql

* Remove commented-out, obsolete lastInsert code.

# database.mysql.inc

## class DatabaseConnection_mysql

* public static $rand and $timestamp are not used

## class MergeQuery_mysql

* Remove commented-out call to drupal_alter().

# Post-PDO SVN commits to CVS database files

HIGH: Some changes to database.inc and database.*.inc have been committed since the PDO SVN tree was created. Since these changes are to files that have been completely written, the periodic cvs updates probably missed these changes. I suspect there are really only one or two such changes that need to be integrated.

I generated this list with:

cvs log -d '02/20/2008

* database.inc: 1.93, 1.94, 1.95, 1.95
* database.mysql.inc: 1.91
* database.mysqli.inc: 1.56, 1.57
* database.pgsql.inc: 1.68.2.2, 1.70, 1.71

The 296 patch rolled by chx applies cleanly to a fresh checkout of head this morning, with just some offset.

I don't know if this is related, thought, but a few problems do happen:
- admin/reports/updates returns a WSOD.
- admin/build/modules returns twice: "Undefined index: #value in D:\src\drupal\head\modules\system\system.admin.inc on line 782"

I think I've tested all pages in a default install (only a few posts, though), and these are the only warnings I got.

MySQL: 5.0.51a-community-nt,
InnoDB engine, tables are created with utf8_general_ci although this was not the default for the DB.

Here is the patch.

Notices are from a commit yesterday, nothing to do with this patch.

message_id - test_id prevented 302 from running simpletests (it's a one line fix though), however after getting them running, there's fails in nearly every simpletest, so looks like a big re-roll to do.

Just tested from a fresh checkout

1. Patch applied cleanly.
2. Installed on WampServer in windows Vista with Mysql 5.0.45, php 5.26
3. Clicked around, created content taxonomy etc. things seemed to work
4. Ran tests.
4a with max execution of 30 seconds, the tests timed out after just aggregator tests. (My first time with running tests so I will assume we need a higher limit).
4b reset to max execution a higher limit (3000), and the tests are still running - currently at 30%. All passes so far except for:

Blog API functionality: 35 passes, 10 fails, 1 exception

I never ran the simpletest before patching, so no idea if that is new.

@nbz: The failure in BlogAPI probably means that you don't have clean urls enabled (see #296470: TestingParty08: BlogAPI tests should pass without clean-urls enabled).

I have cleanurls.

The test is now complete - 9 more failures at XML-RPC validator, but that may be because this is a localhost without webaccess (no idea).

Total failed tests:

Request for user's blogs returned correctly. Other drupal_web_test_case.php 284 testBlogAPI
Post created. Other drupal_web_test_case.php 284 testBlogAPI
Recent post list retreived. Other drupal_web_test_case.php 284 testBlogAPI
Post found. Other drupal_web_test_case.php 284 testBlogAPI
Post successfully modified. Other drupal_web_test_case.php 284 testBlogAPI
File successfully uploaded. Other drupal_web_test_case.php 284 testBlogAPI
Uploaded contents verified. Other drupal_web_test_case.php 284 testBlogAPI
Post categories set. Other drupal_web_test_case.php 284 testBlogAPI
Category list successfully retreived. Other drupal_web_test_case.php 284 testBlogAPI
Post successfully deleted. Other drupal_web_test_case.php 284 testBlogAPI

array of structs test: %s Other drupal_web_test_case.php 284 testValidator1
count the entities test: %s Other drupal_web_test_case.php 284 testValidator1
easy struct test: %s Other drupal_web_test_case.php 284 testValidator1
echo struct test: %s Other drupal_web_test_case.php 284 testValidator1
many types test: %s Other drupal_web_test_case.php 284 testValidator1
moderate size array check: %s Other drupal_web_test_case.php 284 testValidator1
nested struct test: %s Other drupal_web_test_case.php 284 testValidator1
nested struct test: %s Other drupal_web_test_case.php 284 testValidator1
multicall equals result Other drupal_web_test_case.php 284 testValidator1

Warnings:

array_key_exists() [function.array-key-exists]: The second argument should be either an array or an object Warning blogapi.test 84

I will le someone else be the judge of what that means.

Some first benchmark results, collected from irc this afternoon, by catch and swentel

ab -c1 -n500 http://drupal7/

* one node, front page, no page caching, no opcache
Catch: unpatched: 7.06 request/sec patched: 5.47 requests/sec (1/2 sec error margin.)
Catch: just ran again - 7 unpatched, 5.35 patched, so looks accurate.
Swentel: unpatched 6.35 r/sec , patched: 5.23 r/sec

* 5,000 nodes, 10,000 comments, 250 terms, 15 vocabs, 500 users.no opcache
Catch: unpatched: 3.64 reqs/sec patched: 3.29
Catch: normal page caching - 60 reqs/sec unpatched, 32 reqs/sec patched
Catch: aggresive: unpatched 55 reqs/sec, patched 37 reqs/sec

* 2500 nodes, 5,000 comments, 250 terms, 15 vocabs, 500 users. no page caching, no opcache
swentel: unpatched 5.15 [#/sec], patched 4.33 [#/sec]

[update] some more results
* 1500 nodes with terms attached, 0 comments, 250 terms, 15 vocabs, 500 users
note: devel generate triggered an pdo exception when trying to add random comments to nodes, so I left comments out of it for this testing

unpatched, no opcache, no page caching, 5.43 [#/sec]
unpatched, no opcache, normal page caching, 57.74 [#/sec]
unpatched, no opcache, aggressive page caching, 61.73 [#/sec]
unpatched, xcache, no page caching, 13.09 [#/sec]
unpatched, xcache, normal page caching, 175.44 [#/sec]
unpatched, xcache, aggressive page caching, 195.10 [#/sec]

patched, no opcache, no page caching, 4.40 [#/sec]
patched, no opcache, normal page caching, 34.07 [#/sec]
patched, no opcache, aggressive page caching, 34.69 [#/sec]
patched, xcache, no page caching, 9.19 [#/sec]
patched, xcache, normal page caching, 126.77 [#/sec]
patched, xcache, aggressive page caching, 138.59 [#/sec]

These are the results of Catch's tests. I had to find them in my IRC log by hand, so I hope I have found it all.