Bug 13766 - Make biblioitems.ean longer and add index
Summary: Make biblioitems.ean longer and add index
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Katrin Fischer
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 18048 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-02-27 11:30 UTC by Frédéric Demians
Modified: 2017-10-09 19:17 UTC (History)
6 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 13766 Set biblioitems.ean type to MEDIUMTEXT (2.68 KB, patch)
2015-02-27 11:34 UTC, Frédéric Demians
Details | Diff | Splinter Review
[Signed-off] Bug 13766 Set biblioitems.ean type to MEDIUMTEXT (2.90 KB, patch)
2015-03-01 19:33 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13766 - Change *bibioitems.ean to mediumtext and add indexes (3.80 KB, patch)
2017-08-17 12:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Change biblioitems.ean to mediumtext and add index (3.87 KB, patch)
2017-08-17 13:14 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 13766 - Change *bibioitems.ean to mediumtext and add indexes (3.91 KB, patch)
2017-09-01 13:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13766 - Follow-up - use index_exists() (1.39 KB, patch)
2017-09-04 20:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13766 - Change *bibioitems.ean to mediumtext and add indexes (4.01 KB, patch)
2017-10-06 10:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13766 - Follow-up - use index_exists() (1.48 KB, patch)
2017-10-06 10:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Demians 2015-02-27 11:30:03 UTC
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.
Comment 1 Frédéric Demians 2015-02-27 11:34:06 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-03-01 19:33:00 UTC
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>
Comment 3 Jonathan Druart 2015-03-05 10:32:58 UTC
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.
Comment 4 Frédéric Demians 2015-03-05 14:28:57 UTC
(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?
Comment 5 Jonathan Druart 2015-03-05 14:36:27 UTC
(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.
Comment 6 Colin Campbell 2017-06-09 10:42:23 UTC
(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
Comment 7 Katrin Fischer 2017-08-17 12:01:05 UTC
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.
Comment 8 Katrin Fischer 2017-08-17 12:02:54 UTC
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.
Comment 9 Katrin Fischer 2017-08-17 12:04:55 UTC
*** Bug 18048 has been marked as a duplicate of this bug. ***
Comment 10 Colin Campbell 2017-08-17 13:14:24 UTC
Created attachment 66147 [details] [review]
Change biblioitems.ean to mediumtext and add index
Comment 11 Colin Campbell 2017-08-17 13:15:27 UTC
Signed off patch
Comment 12 Katrin Fischer 2017-08-17 13:17:17 UTC
Thx Colin!
Comment 13 Nick Clemens 2017-09-01 13:54:08 UTC
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>
Comment 14 Nick Clemens 2017-09-01 13:54:56 UTC
Looks good and worked for me. Wondered if we need to check for existing indexes on deletedbiblioitems but think this is okay
Comment 15 Jonathan Druart 2017-09-01 14:05:42 UTC
(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
Comment 16 Katrin Fischer 2017-09-01 18:13:32 UTC
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.
Comment 17 Katrin Fischer 2017-09-04 20:39:15 UTC
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.
Comment 18 Marcel de Rooy 2017-10-06 10:25:24 UTC
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>
Comment 19 Marcel de Rooy 2017-10-06 10:25:28 UTC
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>
Comment 20 Marcel de Rooy 2017-10-06 10:26:32 UTC
For another report: But do we really need all those indexes on deleted.. tables ?
Comment 21 Katrin Fischer 2017-10-06 11:32:39 UTC
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.
Comment 22 Marcel de Rooy 2017-10-09 07:11:23 UTC
(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.
Comment 23 Jonathan Druart 2017-10-09 19:17:01 UTC
Pushed to master for 17.11, thanks to everybody involved!