Bug 7065 - reserves table needs a primary key
Summary: reserves table needs a primary key
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 7711 7710
Blocks: 5609 5696 7717
  Show dependency treegraph
 
Reported: 2011-10-19 16:09 UTC by Ian Walls
Modified: 2014-05-26 21:04 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7065 - reserves table needs a primary key (5.63 KB, patch)
2012-02-21 17:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (5.31 KB, patch)
2012-03-06 15:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (9.67 KB, patch)
2012-03-19 15:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (9.67 KB, patch)
2012-03-19 16:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (9.71 KB, patch)
2012-03-19 16:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (9.53 KB, patch)
2012-05-02 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (9.28 KB, patch)
2012-05-02 14:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (5.86 KB, patch)
2012-05-03 15:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (5.88 KB, patch)
2012-05-03 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (5.89 KB, patch)
2012-05-04 10:45 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
[PASSED QA] Bug 7065 - reserves table needs a primary key (6.01 KB, patch)
2012-05-23 13:46 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 7065 - reserves table needs a primary key (6.00 KB, patch)
2012-06-10 19:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7065 - reserves table needs a primary key (6.02 KB, patch)
2012-06-12 13:43 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Walls 2011-10-19 16:09:21 UTC
The reserves (and old_reserves) tables desperately need a primary key.  reservernumber should be used for consistency sake.  This will allow the possibility of multiple holds per biblio/patron combination, and a host of other improvements.

This is included in bug 5872, but needs to be broken out into it's own enhancement for independent review.

One cavaet: the values must be unique across the union of reserves and old_reserves.  While this is easy enough to achieve in a new installation, adding the values into an existing install gets trickier.
Comment 1 Kyle M Hall 2012-02-21 17:37:11 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-03-03 19:38:04 UTC
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
Comment 3 Kyle M Hall 2012-03-06 15:54:08 UTC Comment hidden (obsolete)
Comment 4 Jared Camins-Esakov 2012-03-17 10:18:05 UTC
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.
Comment 5 Kyle M Hall 2012-03-19 15:28:18 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-03-19 15:31:12 UTC
(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.
Comment 7 Kyle M Hall 2012-03-19 16:01:08 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2012-03-19 16:01:51 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-05-02 14:05:30 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-05-02 14:06:36 UTC Comment hidden (obsolete)
Comment 11 MJ Ray (software.coop) 2012-05-03 09:07:40 UTC
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;
Comment 12 Kyle M Hall 2012-05-03 14:06:30 UTC
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;
Comment 13 Galen Charlton 2012-05-03 14:14:27 UTC
(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;
Comment 14 Galen Charlton 2012-05-03 14:31:24 UTC
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;
Comment 15 Kyle M Hall 2012-05-03 15:27:37 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2012-05-03 15:29:16 UTC Comment hidden (obsolete)
Comment 17 MJ Ray (software.coop) 2012-05-04 10:45:48 UTC Comment hidden (obsolete)
Comment 18 Paul Poulain 2012-05-15 14:24:23 UTC
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 ?
Comment 19 Kyle M Hall 2012-05-17 10:30:28 UTC
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 ?
Comment 20 Ian Walls 2012-05-23 13:46:59 UTC Comment hidden (obsolete)
Comment 21 Paul Poulain 2012-05-25 16:04:07 UTC
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
Comment 22 Paul Poulain 2012-06-10 16:20:24 UTC
As the _id proposal seems to make most ppl happy, I'll push the patch
Comment 23 Paul Poulain 2012-06-10 16:38:41 UTC
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 !
Comment 24 Kyle M Hall 2012-06-10 19:25:35 UTC Comment hidden (obsolete)
Comment 25 Julian Maurice 2012-06-12 13:43:29 UTC
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.
Comment 26 Marcel de Rooy 2012-07-05 09:40:43 UTC
Looks good to me. Last QA comment has been incorporated.
Passed QA