batch API bugfixes

I'm fine with your merging my/hunmonk's batch patch into this one. I will review this patch (not tonight, though) if you look at http://drupal.org/node/157682. :-) It addresses a wide variety of update.php problems, including the batch problem (though it currently solves it as my previous patch did, not as this one does).

I reviewed this patch and I'm a bit puzzled by:

+      db_query('INSERT INTO {batch} (token) VALUES ("")');
+      $batch['id'] = db_last_insert_id('batch', 'bid');

What are you trying to achieve here? Could you document in the patch why it is important to have a valid bid -- will the other batch steps use this bid to write to the batch table? Please add one more line of documentation to make this clear.

That said, I think this may be fundamentally broken. 'bid' is a pimary key so there can only be one "" string in the database. When two batch processes run concurrently, this will trow SQL errors?

To me, I'm not sure if I agree with this by bjaspan here http://drupal.org/node/152383#comment-270233:

My fix is to insert a temporary value explicitly for the 'token' column. Adding a default value would work as well, but my thinking is that a default value should be one that is actually correct most of the time unless a specific value is provided. In this case, a default value will never be right and will always be immediately overriden, so it makes more sense to demonstrate that explicitly in the code.

but then doing this:

<?php
+      db_query('INSERT INTO {batch} (token) VALUES ("")');
+      $batch['id'] = db_last_insert_id('batch', 'bid');
?>

The way I've always dealt with MySQL, is that every varchar column gets a default value. Why don't we just add the default value for the token varchar ( http://drupal.org/node/158918 ).

i tested this along with node_access_rebuild patch. all worked well - this one is ready.

I meant to test this on pgsql and dropped the ball. Kicked in gear by Moshe, I just did.

I upgraded a Drupal 5.1 site with all optional core modules except locale enabled by Drupal 6 with this patch applied and with my update.php patch applied (I got one conflict with my patch b/c it also tries to fix a batch table problem that this patch already fixed). I got no errors, which improves on my patch that produces one error about 'id' being undefined somewhere in the batch code. Also, the final schema report is clean.

So, +1 from me. When this is applied, I'll re-roll my update.php patch it is will be ready, too.

Committed to CVS HEAD. Thanks.