biblioitems.ean field is too short. It's a VARCHAR(13) type. EAN can be longer, especially if it's repeated in the biblio record. When biblio records are imported, and 073 (Unimarc) or 024 (MARC21) field is repeated, their content can't be stored in biblioitems.ean field : it stopped the biblio record import. This field should be aligned to biblioitems.isbn type: MEDIUMTEXT.
Created attachment 36233 [details] [review] Bug 13766 Set biblioitems.ean type to MEDIUMTEXT biblioitems.ean field is too short. It's a VARCHAR(13) type. EAN can be longer, especially if it's repeated in the biblio record. When biblio records are imported, and 073 (Unimarc) or 024 (MARC21) field is repeated, their content can't be stored in biblioitems.ean field: it stops the biblio record import. This field should be aligned to biblioitems.isbn type: MEDIUMTEXT.
Created attachment 36251 [details] [review] [Signed-off] Bug 13766 Set biblioitems.ean type to MEDIUMTEXT biblioitems.ean field is too short. It's a VARCHAR(13) type. EAN can be longer, especially if it's repeated in the biblio record. When biblio records are imported, and 073 (Unimarc) or 024 (MARC21) field is repeated, their content can't be stored in biblioitems.ean field: it stops the biblio record import. This field should be aligned to biblioitems.isbn type: MEDIUMTEXT. Verified that tables biblioitems and deletedbiblioitems are changed. Signed-off-by: Marc Véron <veron@veron.ch>
Frédéric, I got an error on executing the updatedb entry: DBD::mysql::db do failed: BLOB/TEXT column 'ean' used in key specification without a key length at installer/data/mysql/updatedatabase.pl line 9798.
(In reply to Jonathan Druart from comment #3) > Frédéric, I got an error on executing the updatedb entry: > > DBD::mysql::db do failed: BLOB/TEXT column 'ean' used in key specification > without a key length at installer/data/mysql/updatedatabase.pl line 9798. Thanks testing. I also can get this error. I don't have it on recent DB. I get it on older. In updatedatebase.pl, line 5323: $dbh->do("CREATE INDEX `ean` ON biblioitems (`ean`) "); So a DB which was on Koha 3.8, and earlier, has an index on biblioitems.ean. A fresh DB, hasn't such an index. kohastructre.sql, line 186 and after, there are index defined for isbn but not for ean. Do you think that an index is required on ean field?
(In reply to Frédéric Demians from comment #4) > Do you think that an index is required on ean field? It has been added by bug 5337, but the patch does not update the kohastructure.sql file. I think it's an oversight and should exist.
(In reply to Frédéric Demians from comment #4) > (In reply to Jonathan Druart from comment #3) > > Frédéric, I got an error on executing the updatedb entry: > > > > DBD::mysql::db do failed: BLOB/TEXT column 'ean' used in key specification > > without a key length at installer/data/mysql/updatedatabase.pl line 9798. > > Thanks testing. I also can get this error. I don't have it on recent DB. I > get it on older. > > In updatedatebase.pl, line 5323: > > $dbh->do("CREATE INDEX `ean` ON biblioitems (`ean`) "); > > So a DB which was on Koha 3.8, and earlier, has an index on biblioitems.ean. > > A fresh DB, hasn't such an index. kohastructre.sql, line 186 and after, > there are index defined for isbn but not for ean. > > Do you think that an index is required on ean field? Yes it should, like isbn, issn be indexed. It could be an important field if the library (like most publics) has a large dvd collection. The short size and lack of key is a problem if trying to do checks against this field
Created attachment 66146 [details] [review] Bug 13766 - Change *bibioitems.ean to mediumtext and add indexes - biblioitems.ean and deleteditems.ean are changed to mediumtext to match issn and isbn. - An index is added for ean on both tables. Patch cleans up inconsistencies caused by bug 5337 that causes some older databases to have an existing index on biblioitems.ean.
Hi Frederic, I hope it's ok that I adopt this bug. I was not sure how best to do a follow up on yours with the need to also move the changes to an atomicupdate, so I have written a new patch from scratch.
*** Bug 18048 has been marked as a duplicate of this bug. ***
Created attachment 66147 [details] [review] Change biblioitems.ean to mediumtext and add index
Signed off patch
Thx Colin!
Created attachment 66744 [details] [review] Bug 13766 - Change *bibioitems.ean to mediumtext and add indexes - biblioitems.ean and deleteditems.ean are changed to mediumtext to match issn and isbn. - An index is added for ean on both tables. Patch cleans up inconsistencies caused by bug 5337 that causes some older databases to have an existing index on biblioitems.ean. Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Looks good and worked for me. Wondered if we need to check for existing indexes on deletedbiblioitems but think this is okay
(In reply to Nick Clemens from comment #14) > Looks good and worked for me. Wondered if we need to check for existing > indexes on deletedbiblioitems but think this is okay Yes, please. Check all of them, and use C4::Installer::index_exists
We can check both - I didn't to it initially, because we only had one on biblioitems in the past, but it makes sense for backporting. I will look for an example for index_exists.
Created attachment 66825 [details] [review] Bug 13766 - Follow-up - use index_exists() Changes check for existing index to use index_exists. Also checks deletedbiblioitems for an existing ean index and drops it if one is found.
Created attachment 67687 [details] [review] Bug 13766 - Change *bibioitems.ean to mediumtext and add indexes - biblioitems.ean and deleteditems.ean are changed to mediumtext to match issn and isbn. - An index is added for ean on both tables. Patch cleans up inconsistencies caused by bug 5337 that causes some older databases to have an existing index on biblioitems.ean. Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67688 [details] [review] Bug 13766 - Follow-up - use index_exists() Changes check for existing index to use index_exists. Also checks deletedbiblioitems for an existing ean index and drops it if one is found. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
For another report: But do we really need all those indexes on deleted.. tables ?
I think the most common use case is using deleted* in a union with the undeleted table. But I am not sure if the index has a positive effect then or not.
(In reply to Katrin Fischer from comment #21) > I think the most common use case is using deleted* in a union with the > undeleted table. But I am not sure if the index has a positive effect then > or not. Depends on use in ORDER BY or WHERE etc.
Pushed to master for 17.11, thanks to everybody involved!