remove db_num_rows(): complete version

I think this is a step backwards. Because of Oracle and PDO's limitations, we are now forced to have go lowest common denominator.

This in turn mandates two queries per pager query (one the real query, and the other the COUNT query).

There are 35 places in core that use db_num_rows() already. Some of that can go, but some of them are really needed.

On a large system, this means performance goes down the tube.

So, my recommendation is that the lesser endowed engines work around it, and leave engines that support it with db_num_rows() ability.

"Lesser endowed engines" includes the PDO-MySQL driver, if you're using the default forward-only cursor. Most places in core where num_rows is used it should only be one query anyway because that's all it's being used for. Doing a full normal select and getting back all of the data just to get a row count is wasteful.

Core needs to be able to handle all supported databases directly, so that means "working around it" in core for everything. PDO offers a simple solution, though.

$records = $result->fetchAll();

Now it's just an array and count() works. That's no slower than what the mysql driver already does internally, in fact. (That's why mysql_unbuffered_query() exists.) In most cases, though, SELECT COUNT(*) is going to be faster if that's all you need.

I've reviewed the patch and pointed out a few errors. The only real problem in core is the user online block and even there you can work around the problem.

I think it's OK to remove this function as it improves database compatibility. There is a good work-around for it, that shouldn't be too expensive in the vast majority of the cases.

I haven't reviewed the latest patch yet.

-  $number = db_num_rows(db_query("SELECT event FROM {flood} WHERE event = '%s' AND hostname = '%s' AND timestamp > %d", $name, ip_address(), time() - 3600));
+  $number = db_result(db_query("SELECT COUNT(event) FROM {flood} WHERE event = '%s' AND hostname = '%s' AND timestamp > %d", $name, ip_address(), time() - 3600));

shouldn't that be COUNT(*) in order to take advantage of mysql optimization? there is no drawback that i can see. it is (slightly) semantically better as well.

I'm OK with committing this patch, as it will really help development of other database backends. At the same time, I've no problem with postponing this patch to Drupal 7 either. As we're going to do a lot of database work in D7, we might be better of to postpone this patch. Some feedback would be appreciated.

i think it is OK to commit this patch remove the num_rows DB function and either put db_num_rows() in legacy.module or paste it into the upgrade docs for lazy upgraders. the fix is straightforward enough that we won't upset module developers too much.

I'm generally in support of lots of little patches in the same direction over one big patch, as it makes it much easier to maintain patches during development. For that reason, I'd favor committing this for D6.

If someone really needs to do old-style num_rows() behavior, the PDO answer would be ->fetchAll(). Do we want to consider implementing a db_fetch_all() now to replace db_num_rows(), or is that a silly addition if it's going to only last one revision?

If that's the general consensus, I'd be happy to commit this patch once it is marked "Ready to be committed". This should happen sooner than late.

I looked at the code and it looks good except for:

+          $authenticated_count = 0;
+          $max_users = variable_get('user_block_max_list_count', 10);
+          $items = array();
+          while ($max_users-- && $account = db_fetch_object($authenticated_users)) {
+            $items[] = $account;
+            $authenticated_count++;
+          }

Now, $authenticated_count willl be smaller or equal to $max_users. On drupal.org, $max_users is 15 but $authenticated_count should return 300 or so.

While reviewing the patch, I found a minor and a major problem:

- the session.inc changes introduce an E_NOTICE error as far as I see, $items is not initilized
- all changes related to node_title_list() calls should be revisited... this seem like passing on the data to theme('node_list') which invokes theme('item_list') which returns HTML output even if there were no rows in the result set... this is not only superfluous HTML output, but cases bugs in cases where it is used as if it returns FALSE if there were no rows in the set, like if ($daytop && ($result = statistics_title_list('daycount', $daytop)) && ($node_title_list = node_title_list($result, t("Today's:")))) {

This version looks OK now. But this is a significant amount of change, so it would be nice to see independent tests done.

minor suggestions on the code. Typo in these code comments:

+ * a SELECT COUNT(*) on the temporary table afterwards. db_affected_rows() do
+ * not give consistent result across different database types in this case.

"do" should be "does", as in "db_affected_rows() does not give"

Also, probably not much of a difference, but db_table_exists() and db_column_exists should be made consistently like this (use db_result rather than db_fetch_object):

 function db_table_exists($table) {
-  return db_num_rows(db_query("SHOW TABLES LIKE '{". db_escape_table($table) ."}'"));
+  return db_result(db_query("SHOW TABLES LIKE '{". db_escape_table($table) ."}'"));
 }

now the usage seems to vary between PostgreSQL and MySQL driver - unless there is a good reason to do so?

Finally, I think the logic change in function menu_tree_page_data() is faulty - $item will always be FALSE at that point in the loop. You need to create a flag instead.

here's a possible implementation for a correct change to the menu.inc part: http://drupal.pastebin.com/d2a1733d8

Note to see the bug introduced by the patch for menu.inc, you may need to be clearing the cache_menu table by hand regularly.

here's the test: set admin, admin/build, and admin/build/menu to be expanded

On visiting any page, the navigation menu should be expanded down fully to admin/build/menu. With the patch applied that fails to happen.

Let's focus on this patch rather than on the multiple smaller ones.

I've reviewed this patch once more and spent some time testing it. Everything looks OK. It gives us better cross-database compatibility and better performance.

(It's not clear whether pwoladin's suggestion made it in, the snippet already expired, but I assume it has.)

Hold. The patch is not yet committed and it should not be because at least taxonomy_render_nodes is broken.

+  while ($node = db_fetch_object($result)) {
+ $output .= node_view(node_load($node->nid), 1);
+  }
+  if ($node) {

It's guaranteed that $node is FALSE after loop. You wanted if ($output)

Found a few more of this in the patch. Edited version of the irc chat follows. You need a variable like $is_more for menu.inc find a good name for it make it FALSE before the while ($item = and make it TRUE inside the loop (I will call this control variable later aggregator_parse_feed , if ($item) , wrong, if ($items) . comment_render , tricky, might need use a control variable here as well. node_title_list , return $node ? change to return $items ?

node_revisions( , if ($node) ===> if ($output)

system_update_179 , use control variable

return count($authmaps) ? $authmaps : 0; <= no need for count, php will nicely cast empty array to false , nonempty to true, try it.

This patch still breaks the menu system and should not be committed - see my pastebinned code above for the fix.

well, at least that won't break menu, but there is no reason to increment a counter - all we care is whether we have 0 or > 0 results. Just use the code with the boolean falg I suggested which will be faster.

Committed the last version of this patch. I'm marking this 'active' until this has been documented in the upgrade instructions.

patch was not made from the drupal root directory

here's a new patch re-rolled from the root dir (the other applied using patch -p1). Otherwise identical. The logic looks right and at least the taxonomy module tests ok.

If $num_rows does not contain a number, why call it $num_rows? (In all of the files patched)? I think $has_rows or something even simpler would be better.

changed variable names.

This looks much better, thanks. Committed!