table message_queue has a NOT NULL AUTO_INCREMENT key `message_id`, we should move to PRIMARY KEY
Created attachment 8489 [details] [review] proposed patch
Hmmm + PRIMARY KEY (`message_id`), KEY `message_id` (`message_id`), Maybe we should remove the second Key?
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.
Created attachment 11743 [details] [review] Bug 7793: Followup: Remove the message_queue.message_id KEY
Last patch removes the useless message_id KEY. I hope it was you want :)
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 ?
Yep, it is, but i switched it to in discussion as Jared's concern has not been addressed.
(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.
Created attachment 36145 [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.
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.
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.
Created attachment 36945 [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.
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.
Created attachment 36987 [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>
Thanks for the sign off Joubu!
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.
Created attachment 37027 [details] [review] [SIGNED-OFF] 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>
Created attachment 37028 [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.
Created attachment 39143 [details] [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 First patch signed. No errors
(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).
Created attachment 39525 [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>
Created attachment 39526 [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>
I amended the 2 patches to reorder the DB entry.
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.
(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.
So these patches need to be redone?
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
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>
(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.
Patches pushed to master. Thanks Olli et al!