We've discussed this on IRC, and have a few concerns. From IRC:

<@rangi> well theres one issue
<@rangi> you dont want auto_increment on old_reserves
< jcamins> Yeah, it's removed after the update.
<@rangi> hmmm
<@rangi> yeah i dont like it
< jcamins> I guess the proper way to do it is create a temporary table?
<@rangi> yes
<@rangi> and also, im not seeing where it changes the old_reserves
< jcamins> At the very end.
<@rangi> no
<@rangi> the update_sth
<@rangi> it does the same one twice
<@rangi> the 2 whiles
<@rangi> not changing renumbering old_reserves
<@rangi> or am i missing something
<@rangi> +    my $query = "UPDATE reserves SET reservenumber = ? WHERE 
         reservenumber = ?";
<@rangi> +    my $update_sth = $dbh->prepare( $query );
< jcamins> Oh, I see.
<@rangi> then it runs that updated_sth twice
<@rangi> so it just renumbers reserves twice
< jcamins> I think you are correct.
<@rangi> doesnt renumber old_reserves at all
...
< cait> so renumber them manually, first oldreserves then reserves?
<@rangi> that would work
< jcamins> It renumbers old_reserves at the beginning.
< jcamins> Using the ALTER TABLE.
<@rangi> hmm, that should definitely not do that
<@rangi> thats def a mysqlism
< jcamins> It also doesn't work properly.
<@rangi> yeah
< jcamins> It breaks the index.
<@rangi> yup
< jcamins> Somehow.
< jcamins> It's weird.
<@rangi> personally id renumber them with with a loop
<@rangi> old_reserves first, then reserves
< cait> makes sense
<@rangi> and id also make the column unique
< jcamins> That won't work either.
<@rangi> why not?
< cait> jcamins: why not?
< cait> lol
< jcamins> At least, as far as I can tell, adding a primary key leaves 
           the InnoDB indexes incorrect.
<@rangi> dont use auto_increment to number them, just select all, set the
         reservenumber
<@rangi> thats easy to fix
<@rangi> you just tell it to optimize the table
< jcamins> Ah.
<@rangi> the classy thing to do would be to do
<@rangi> if mysql
<@rangi> optimize reserves

This doesn't quite work for me. First of all, I'm a little dubious about listing every single change. Second, the update for old_reserves set the reservenumber to 0 for the first 1078 reserves in my database, leaving only 8 with valid reservenumbers. This led to the following error:
DBD::mysql::db do failed: Duplicate entry '0' for key 'PRIMARY' at installer/data/mysql/updatedatabase.pl line 4960.

(In reply to comment #4)

The problem was that the updates weren't using enough columns to ensure each update worked on a unique row. This new version uses *every* column, so we shouldn't have that problem now.

I also removed the print lines for each update, they really shouldn't have been in there in the first place.

> This doesn't quite work for me. First of all, I'm a little dubious about
> listing every single change. Second, the update for old_reserves set the
> reservenumber to 0 for the first 1078 reserves in my database, leaving only
> 8 with valid reservenumbers. This led to the following error:
> DBD::mysql::db do failed: Duplicate entry '0' for key 'PRIMARY' at
> installer/data/mysql/updatedatabase.pl line 4960.

Would this be safer as an updatedatabase approach:

alter table reserves add column reservenumber int(11) not null auto_increment primary key;
set @ai=(select max(reservenumber) from reserves);
alter table old_reserves add reservenumber int(11);
insert into reserves select * from old_reserves;
truncate old_reserves;
insert into old_reserves select * from reserves where reservenumber > @ai;
delete from reserves where reservenumber > @ai;
alter table old_reserves add primary key reservenumber;

I like it. Perhaps we can get some input from others. I do prefer reserve_id over reservenumber though, if only because it has fewer characters and is more readable imho.

(In reply to comment #11)
> Would this be safer as an updatedatabase approach:
> 
> alter table reserves add column reservenumber int(11) not null
> auto_increment primary key;
> set @ai=(select max(reservenumber) from reserves);
> alter table old_reserves add reservenumber int(11);
> insert into reserves select * from old_reserves;
> truncate old_reserves;
> insert into old_reserves select * from reserves where reservenumber > @ai;
> delete from reserves where reservenumber > @ai;
> alter table old_reserves add primary key reservenumber;

(In reply to comment #11)
> Would this be safer as an updatedatabase approach:
> 
> alter table reserves add column reservenumber int(11) not null
> auto_increment primary key;
> set @ai=(select max(reservenumber) from reserves);
> alter table old_reserves add reservenumber int(11);
> insert into reserves select * from old_reserves;
> truncate old_reserves;
> insert into old_reserves select * from reserves where reservenumber > @ai;
> delete from reserves where reservenumber > @ai;
> alter table old_reserves add primary key reservenumber;

I suggest a variant of this approach so that old reserves have lower IDs than current ones.  Note that this is *untested*:

create table tmp_reserves as select * from reserves limit 0;
alter table tmp_reserves add column reservenumber int(11) not null
auto_increment primary key;
insert into tmp_reserves select * from old_reserves order by reservedate;

truncate old_reserves;
alter table old_reserves add reservenumber int(11);
insert into old_reserves select * from tmp_reserves);
truncate tmp_reserves;

insert into tmp_reserves select * from reserves order by reservedate;
alter table reserves add reservenumber int(11) not null auto_increment primary_key;
truncate reserves;
insert into reserves select * from tmp_reserves);
drop table tmp_reserves;

And a more tested version:

create table tmp_reserves as select * from reserves limit 0;
alter table tmp_reserves add column reservenumber int(11) not null
auto_increment primary key;
insert into tmp_reserves (borrowernumber, reservedate, biblionumber, constrainttype, branchcode, notificationdate, reminderdate, cancellationdate, reservenotes, priority, found, timestamp, itemnumber, waitingdate, expirationdate, lowestPriority) select borrowernumber, reservedate, biblionumber, constrainttype, branchcode, notificationdate, reminderdate, cancellationdate, reservenotes, priority, found, timestamp, itemnumber, waitingdate, expirationdate, lowestPriority from old_reserves order by reservedate;

set @ai=(select max(reservenumber) from tmp_reserves);

truncate old_reserves;
alter table old_reserves add reservenumber int(11);
insert into old_reserves select * from tmp_reserves where reservenumber <= @ai;

insert into tmp_reserves (borrowernumber, reservedate, biblionumber, constrainttype, branchcode, notificationdate, reminderdate, cancellationdate, reservenotes, priority, found, timestamp, itemnumber, waitingdate, expirationdate, lowestPriority) select borrowernumber, reservedate, biblionumber, constrainttype, branchcode, notificationdate, reminderdate, cancellationdate, reservenotes, priority, found, timestamp, itemnumber, waitingdate, expirationdate, lowestPriority from reserves order by reservedate;

alter table reserves add reservenumber int(11) not null auto_increment primary_key;
truncate reserves;
insert into reserves select * from tmp_reserves where reservenumber > @ai;
drop table tmp_reserves;

QA comment: DB change change only.
However, follow-up(s) will be needed to continue the work, because this patch only does not change anything. Kyle, do you plan to work on those follow-ups ?

Yes. This bug is a blocker for many hold related bugs, some of which I will be working on.

(In reply to comment #18)
> QA comment: DB change change only.
> However, follow-up(s) will be needed to continue the work, because this
> patch only does not change anything. Kyle, do you plan to work on those
> follow-ups ?

I've started a discussion on koha-devel about naming of primary keys. You're welcomed to join the discussion !

(I keep passed QA until we've found

As the _id proposal seems to make most ppl happy, I'll push the patch

Failed QA, and a tricky one !!!

When tmp_reserve is created, it is from reserves, so with 
borrowernumber being NOT NULL default 0

When old_reserves is copied to tmp_reserves ... all NULL borrowernumbers are transformed to 0 ... and when it's copied back to old_reserves ... booom, contraint failed:

[Sun Jun 10 18:28:14 2012] updatedatabase.pl: DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`lecannetdesmaures`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE SET NULL) at /home/paul/koha.dev/koha-community/installer/data/mysql/updatedatabase.pl line 5395.


That's a common situation to have NULL in old_reserves, when a borrower is removed from the database but we want to keep the history of the hold "someone placed a hold on yyyy-mm-dd"

I think the solution would be to have tmp_reserve be created from old_reserve, to enable the NULL field !

Created attachment 10250 [details] [review]
[SIGNED-OFF] Bug 7065 - reserves table needs a primary key

Ok, tmp_reserve is created from old_reserve like said in Paul's comment.
Still works as expected, signed off.

Looks good to me. Last QA comment has been incorporated.
Passed QA