Bug 7793 - redefine the field message_id as PRIMARY KEY of message_queue
Summary: redefine the field message_id as PRIMARY KEY of message_queue
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Jonathan Druart
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-22 10:39 UTC by stephane.delaune
Modified: 2016-12-05 21:23 UTC (History)
7 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:


Attachments
proposed patch (1.63 KB, patch)
2012-03-22 10:44 UTC, stephane.delaune
Details | Diff | Splinter Review
Bug 7793: Followup: Remove the message_queue.message_id KEY (1.67 KB, patch)
2012-08-22 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7793 - redefine the field message_id as PRIMARY KEY of message_queue (5.55 KB, patch)
2015-02-24 17:02 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 7793 - redefine the field message_id as PRIMARY KEY of message_queue (5.77 KB, patch)
2015-03-17 12:08 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 7793 - redefine the field message_id as PRIMARY KEY of message_queue (5.82 KB, patch)
2015-03-18 14:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7793 - redefine the field message_id as PRIMARY KEY of message_queue (5.89 KB, patch)
2015-03-20 12:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7793 - Simplify db update (3.39 KB, patch)
2015-03-20 12:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7793: Simplify db update (4.39 KB, patch)
2015-05-13 15:14 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 7793 - redefine the field message_id as PRIMARY KEY of message_queue (6.03 KB, patch)
2015-05-26 14:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7793 - Simplify db update (3.52 KB, patch)
2015-05-26 14:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7793 - redefine the field message_id as PRIMARY KEY of message_queue (4.60 KB, patch)
2015-06-16 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7793 - Simplify db update (3.52 KB, patch)
2015-06-16 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description stephane.delaune 2012-03-22 10:39:33 UTC
table message_queue has a NOT NULL AUTO_INCREMENT key `message_id`, we should move to PRIMARY KEY
Comment 1 stephane.delaune 2012-03-22 10:44:05 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-04-09 09:12:02 UTC
Hmmm

+  PRIMARY KEY (`message_id`),
   KEY `message_id` (`message_id`),

Maybe we should remove the second Key?
Comment 3 Jared Camins-Esakov 2012-05-13 15:22:07 UTC
I'm concerned about this patch because there is nothing to prevent two rows having duplicate message_ids. The existing index is KEY and not UNIQUE.
Comment 4 Jonathan Druart 2012-08-22 12:47:31 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2012-08-22 12:48:31 UTC
Last patch removes the useless message_id KEY.
I hope it was you want :)
Comment 6 Jonathan Druart 2012-08-23 07:11:17 UTC
Chris,
Why do you change the status to "In discussion" ?
The last patch removes the second key on message_id. It is not what you wanted ?
Comment 7 Chris Cormack 2012-08-23 07:29:03 UTC
Yep, it is, but i switched it to in discussion as Jared's concern has not been addressed.
Comment 8 Galen Charlton 2013-07-03 20:19:35 UTC
(In reply to Jared Camins-Esakov from comment #3)
> I'm concerned about this patch because there is nothing to prevent two rows
> having duplicate message_ids. The existing index is KEY and not UNIQUE.

Since the column is AUTO_INCREMENT, I question to what extent this would be a problem in practice unless folks have been in the habit of munging message_id.

Regardless, a follow-up to the schema update should be straightforward: check to see if there are duplicates, and if so, renumber the affected rows.  I don't think that there is any code that cares about any particular values in that column.
Comment 9 Olli-Antti Kivilahti 2015-02-24 17:02:45 UTC Comment hidden (obsolete)
Comment 10 Olli-Antti Kivilahti 2015-02-24 17:06:20 UTC
Sorry to steal your bug.
Here is everything inside one commit + the rare case of duplicate PKs.
No discussion for this one.
It is a no-brainer to have a PRIMARY KEY, especially since DBIx-operations fail without it.
Comment 11 Jonathan Druart 2015-03-17 11:49:45 UTC
Olli,
Could you
- move the updatedb entry at the end of the updatedatabase.pl file.
- make the change more clear, it's not easily readable

And I think there is a problem:
Before: 2 id=42
After: 1 id=43 and 1 id=44
I would expect: 1 id=42 and 1 id=43.
Comment 12 Olli-Antti Kivilahti 2015-03-17 12:08:16 UTC Comment hidden (obsolete)
Comment 13 Olli-Antti Kivilahti 2015-03-17 12:09:54 UTC
Fixed Jonathan's good bug.
Thanks for it.

Won't move the updatedatabase-subroutine "down" because it will just conflict so badly with all future modifications.
Comment 14 Jonathan Druart 2015-03-18 14:01:19 UTC Comment hidden (obsolete)
Comment 15 Olli-Antti Kivilahti 2015-03-18 14:07:49 UTC
Thanks for the sign off Joubu!
Comment 16 Kyle M Hall 2015-03-20 12:18:25 UTC
I got this error when I tried to update my db:
DBD::mysql::db do failed: ALTER TABLE causes auto_increment resequencing, resulting in duplicate entry '9' for key 'PRIMARY' at installer/data/mysql/updatedatabase.pl line 6750.
Comment 17 Kyle M Hall 2015-03-20 12:19:20 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2015-03-20 12:19:29 UTC Comment hidden (obsolete)
Comment 19 Bernardo Gonzalez Kriegel 2015-05-13 15:14:59 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-05-26 13:48:03 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #19)
> Created attachment 39143 [details] [review] [review]
> [SIGNED-OFF] Bug 7793: Simplify db update
> 
> Since message_id is not linked to anything else in the database, we
> can ensure the message_id's are unique by simply dropping the column
> and recreating it.
> 
> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
> 
> Moved updatedatabase to atomicupdate

atomicupdate only works for sql queries (and 1 per file).
Comment 21 Jonathan Druart 2015-05-26 14:01:35 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2015-05-26 14:01:40 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2015-05-26 14:02:17 UTC
I amended the 2 patches to reorder the DB entry.
Comment 24 Tomás Cohen Arazi 2015-05-27 17:31:48 UTC
Guys, can u verify this? I ran the dbix upgrade script just in case on top of this, and found that the generated Koha/Schema/Result/MessageQueue.pm differs from the one shipped with this patch set.

I always regenerate them, because the hash on them usually is invalid and makes the upgrade script fail.

I mark this Failed-QA until you tell me which is wrong: the kohastructure.sql change or the Koha/Schema/Result/MessageQueue.pm change.
Comment 25 Jonathan Druart 2015-06-03 08:25:42 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> Guys, can u verify this? I ran the dbix upgrade script just in case on top
> of this, and found that the generated Koha/Schema/Result/MessageQueue.pm
> differs from the one shipped with this patch set.
> 
> I always regenerate them, because the hash on them usually is invalid and
> makes the upgrade script fail.
> 
> I mark this Failed-QA until you tell me which is wrong: the
> kohastructure.sql change or the Koha/Schema/Result/MessageQueue.pm change.

Yes, the file provided in the first patch is wrong.
I think the changes to Koha/Schema should not be provided unless it is needed to make the dev/fix work.
It always should be provided in a separate patch.
Comment 26 Katrin Fischer 2015-06-06 11:16:27 UTC
So these patches need to be redone?
Comment 27 Jonathan Druart 2015-06-16 10:46:12 UTC
Created attachment 40172 [details] [review]
Bug 7793 - redefine the field message_id as PRIMARY KEY of message_queue

Making message_queue work nicely with DBIx.

DROP the existing KEY and replace with an PRIMARY KEY.
If somebody has managed to get duplicate message_ids in his/her
message_queue-table, this patch takes care of them as well, but givin
all duplicate message_id's a new id.

TEST INSTRUCTIONS: in the patch.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Amended patch: Remove change to Koha/Schema/Result/MessageQueue.pm, will
be done by the RM
Comment 28 Jonathan Druart 2015-06-16 10:46:17 UTC
Created attachment 40173 [details] [review]
Bug 7793 - Simplify db update

Since message_id is not linked to anything else in the database, we
can ensure the message_id's are unique by simply dropping the column
and recreating it.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 29 Jonathan Druart 2015-06-16 10:46:56 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> Guys, can u verify this? I ran the dbix upgrade script just in case on top
> of this, and found that the generated Koha/Schema/Result/MessageQueue.pm
> differs from the one shipped with this patch set.
> 
> I always regenerate them, because the hash on them usually is invalid and
> makes the upgrade script fail.
> 
> I mark this Failed-QA until you tell me which is wrong: the
> kohastructure.sql change or the Koha/Schema/Result/MessageQueue.pm change.

I have amended the first patch to remove the changes to this file.
Comment 30 Tomás Cohen Arazi 2015-06-22 20:08:46 UTC
Patches pushed to master.

Thanks Olli et al!