Bug 13790 - Add unique id issue_id to issues and old_issues tables
Summary: Add unique id issue_id to issues and old_issues tables
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 13743 (view as bug list)
Depends on:
Blocks: 6427 9011 9303 14069 15462 16496 16534
  Show dependency treegraph
 
Reported: 2015-03-04 09:08 UTC by Kyle M Hall
Modified: 2016-12-05 21:27 UTC (History)
7 users (show)

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


Attachments
Bug 13790 - Add unique id issue_id to issues and old_issues tables (4.02 KB, patch)
2015-03-04 09:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 - Update Koha Schema (3.05 KB, patch)
2015-03-04 09:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 - Add unique id issue_id to issues and old_issues tables (4.73 KB, patch)
2015-03-23 11:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 - Update Koha Schema (3.10 KB, patch)
2015-03-23 11:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13790 - Add unique id issue_id to issues and old_issues tables (5.42 KB, patch)
2015-04-12 20:44 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13790 - Update Koha Schema (3.16 KB, patch)
2015-04-12 20:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13790 - Update Koha Schema (2.37 KB, patch)
2015-04-17 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13790 [QA Followup] (1.57 KB, patch)
2015-04-17 15:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 [QA Followup] (1.53 KB, patch)
2015-04-17 15:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 - Add unique id issue_id to issues and old_issues tables (5.51 KB, patch)
2015-04-20 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13790 - Update Koha Schema (2.42 KB, patch)
2015-04-20 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13790 [QA Followup] (1.62 KB, patch)
2015-04-20 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13790: [QA Follow-up] Relocation of db revision in updatedatabase (2.33 KB, patch)
2015-04-20 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13790 [QA Followup] - Disable auto increment on old_issues after pk values are created (887 bytes, patch)
2015-04-20 12:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 [QA Followup] - Disable auto increment on old_issues after pk values are created (969 bytes, patch)
2015-04-20 12:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13790 - Add unique id issue_id to issues and old_issues tables (5.47 KB, patch)
2015-04-22 17:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 - Add unique id issue_id to issues and old_issues tables (5.47 KB, patch)
2015-04-22 17:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 - Update Koha Schema (2.40 KB, patch)
2015-04-22 17:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 [QA Followup] (1.58 KB, patch)
2015-04-22 17:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790: [QA Follow-up] Relocation of db revision in updatedatabase (2.28 KB, patch)
2015-04-22 17:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13790 [QA Followup] - Disable auto increment on old_issues after pk values are created (944 bytes, patch)
2015-04-22 17:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
The script we use to update the schema didn't like the checksum (3.06 KB, patch)
2015-04-23 16:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2015-03-04 09:08:35 UTC

    
Comment 1 Kyle M Hall 2015-03-04 09:30:10 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-03-04 09:30:59 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2015-03-22 06:36:37 UTC
Hi Kyle, Olli has another patch for this on bug 13743, also this doesn't seem to take into account the changes we made after testing on bug 9303.
I am wondering about the auto_increment on old_issues and the renumbering of the old entries especially. Could you try and figure this out together?
Comment 4 Kyle M Hall 2015-03-23 11:23:44 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2015-03-23 11:23:50 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2015-03-23 11:24:44 UTC
*** Bug 13743 has been marked as a duplicate of this bug. ***
Comment 7 Nick Clemens 2015-04-12 20:44:59 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2015-04-12 20:45:07 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens 2015-04-12 20:47:35 UTC
I cleared a simple conflict in updatedatabase (or attempted at least) and tested and all worked as expected.  Hope it was okay to clear and sign off in one step.
Comment 10 Marcel de Rooy 2015-04-17 14:04:37 UTC
QA: Looking here now..
Comment 11 Marcel de Rooy 2015-04-17 14:24:52 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2015-04-17 14:25:15 UTC
That was just a trivial rebase.
Comment 13 Marcel de Rooy 2015-04-17 14:43:39 UTC
QA Comment:
Being somewhat strict now :) I have some hesitations about your db revision:

It makes Koha::Database appear now in mysql/updatedatabase.pl. Well, it must be introduced one day. That is true, but maybe it needs a little bit more thought? The DBIx project is not moving so much these days..
The $schema should not be in a far-away if statement btw.

Secondly, you combine the old way with $dbh from C4 with some DBIx statements. That does not look good to me.

And what about these lines:
+    my $max_issue_id = $schema->resultset('Issue')->get_column('issue_id')->max();
+    $max_issue_id ||= $schema->resultset('OldIssue')->get_column('issue_id')->max();
+    $max_issue_id ||= 0;
+    $max_issue_id++;
+    $dbh->do(qq{
+        ALTER TABLE issues AUTO_INCREMENT = $max_issue_id}
That is somewhat strange. It will work, but you should just pick max from issues. If there are no records, you are ready. Otherwise +1.

All by all, I recommend adjusting your patch.
Comment 14 Kyle M Hall 2015-04-17 15:01:06 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-04-17 15:05:42 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2015-04-20 07:59:03 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2015-04-20 07:59:08 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2015-04-20 07:59:12 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2015-04-20 07:59:15 UTC Comment hidden (obsolete)
Comment 20 Olli-Antti Kivilahti 2015-04-20 10:55:15 UTC
Thinking about Katrin's comment on Bug 13743 regarding the AUTO_INCREMENT of old_issues.

Isnt it a bit dangerous to have the old_issues.issue_id as AUTO_INCREMENT?
This feature works as well without it, and prevents accidentally adding old_issues from hypothetical other sources without a issue_id known.

When issues.issue_id and old_issues.issue_id get out of sync,
that would cause serious and hard-to-see issues when old_issues-table is not populated on check-ins.

Nobody should touch the old_issues -table from anywhere but the C4::Circulation::AddReturn(), but dropping the AUTO_INCREMENT will make it a bit harder.

I don't think this issue is a blocker, but I would feel better.

BTW. Separating issues and old_issues is really awkward, but I guess we need to just live with it. Thinking from a statistical point of view.
Comment 21 Katrin Fischer 2015-04-20 11:21:18 UTC
In the kohastructure.sql the old_issues is not auto increment (first patch), but in the database update it seems to be (could only look at the patches, not apply them). Can someone confirm?

I am also in favor of not auto_incrementing old_issues and being consistent!
Comment 22 Marcel de Rooy 2015-04-20 11:25:36 UTC
Kyle: Please adjust !
Comment 23 Kyle M Hall 2015-04-20 11:48:55 UTC
(In reply to Katrin Fischer from comment #21)
> In the kohastructure.sql the old_issues is not auto increment (first patch),
> but in the database update it seems to be (could only look at the patches,
> not apply them). Can someone confirm?
> 
> I am also in favor of not auto_incrementing old_issues and being consistent!

Agreed, old_issues should no autoincrement! I'll take a look!
Comment 24 Kyle M Hall 2015-04-20 12:21:40 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2015-04-20 12:27:12 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2015-04-22 17:57:56 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2015-04-22 17:58:24 UTC
Created attachment 38350 [details] [review]
Bug 13790 - Add unique id issue_id to issues and old_issues tables

Test Plan:
1) Use a database with existing issues and old issues
2) Apply this patch
3) Run updatedatabase
4) Inspect the database
   a) old_issues should now have a column issue_id starting with 1
   b) issues should now have a column issue_id starting with the number
      of rows in the old_issues table plus one
5) Perform a checkout, note it is written to the database
6) Check in the checked out item, note it is moved to the old_issues table

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Kyle M Hall 2015-04-22 17:58:53 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2015-04-22 17:58:56 UTC
Created attachment 38352 [details] [review]
Bug 13790 [QA Followup]

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Kyle M Hall 2015-04-22 17:58:59 UTC
Created attachment 38353 [details] [review]
Bug 13790: [QA Follow-up] Relocation of db revision in updatedatabase

The db rev lines should not be under the bug 13068 sandbox section.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Kyle M Hall 2015-04-22 17:59:02 UTC
Created attachment 38354 [details] [review]
Bug 13790 [QA Followup] - Disable auto increment on old_issues after pk values are created

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Tomás Cohen Arazi 2015-04-23 16:07:21 UTC
Created attachment 38429 [details] [review]
The script we use to update the schema didn't like the checksum

of the provided patch. So I obsolete it and regenerate properly.
Comment 33 Tomás Cohen Arazi 2015-04-23 16:14:56 UTC
Patches pushed to master.

Thanks Kyle!
Comment 34 Dobrica Pavlinusic 2015-04-27 12:24:40 UTC
I have problem with this patch. When applied to our database it generates following errors:


    [Mon Apr 27 12:33:40 2015] updatedatabase.pl: DBD::mysql::db do failed: Multiple primary key defined at /srv/koha/installer/data/mysql/updatedatabase.pl line 10166.
    [Mon Apr 27 12:33:40 2015] updatedatabase.pl: DBD::mysql::db do failed: Unknown column 'issue_id' in 'order clause' at /srv/koha/installer/data/mysql/updatedatabase.pl line 10170.
    [Mon Apr 27 12:33:40 2015] updatedatabase.pl: DBD::mysql::st execute failed: Unknown column 'me.issue_id' in 'field list' at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1481.
    DBIx::Class::ResultSetColumn::max(): Unknown column 'me.issue_id' in 'field list' at /srv/koha/installer/data/mysql/updatedatabase.pl line 10174


It seems that

ALTER TABLE issues ADD issue_id INT( 11 ) NOT NULL AUTO_INCREMENT PRIMARY KEY FIRST

doesn't work because we already have itemnumber defined as primary key. Should we drop it before this alter table?
Comment 35 Tomás Cohen Arazi 2015-04-27 13:29:59 UTC
(In reply to Dobrica Pavlinusic from comment #34)

> doesn't work because we already have itemnumber defined as primary key.
> Should we drop it before this alter table?

Dobrica, you mean you altered the original DB definition?
Comment 36 Julian Maurice 2015-04-28 09:45:18 UTC
Same problem here.
It seems that primary key was introduced by Bug 4002 (version 3.05.00.009 - d0c9592592c0bd6c529915a9fbc04476f9b52851), and it was removed *only from kohastructure.sql* by Bug 6716 (version 3.05.00.011 - 91b5a09c6cef398d9ad39ca1ced7d6569a87456b)

So the problem will happen to those who started with Koha 3.4 or less.
Comment 37 Julian Maurice 2015-04-28 09:47:56 UTC
To make the updatedatabase process to finish correctly I had to run the following query:
  ALTER TABLE issues DROP PRIMARY KEY
Comment 38 Katrin Fischer 2015-04-28 09:51:07 UTC
Hi Julian, thx for investigating. I think we need to fix this prior to release.
Comment 39 Katrin Fischer 2015-04-28 09:54:38 UTC
I have filed a separate 'blocker' bug 14069.
Comment 40 Tomás Cohen Arazi 2015-04-28 12:29:26 UTC
(In reply to Katrin Fischer from comment #39)
> I have filed a separate 'blocker' bug 14069.

Great job!