Description
Mason James
2013-06-13 03:11:43 UTC
The data might be the same, but the format is different, because biblioitems.marc is iso2709, right? I seem to remember someone saying that we should keep the iso2709 in biblioitems.marc, because if you want to export e.g. 500k records in iso2709 format, transforming all those records from marcxml on the fly would be *really* heavy. But I might be wrong... (In reply to Magnus Enger from comment #1) > > I seem to remember someone saying that we should keep the iso2709 in > biblioitems.marc, because if you want to export e.g. 500k records in iso2709 > format, transforming all those records from marcxml on the fly would be > *really* heavy. But I might be wrong... I'd think not having to take care of twice the data would weigh more than possibly having a hypothetical situation of exporting 500k iso2709 records... (In reply to Magnus Enger from comment #1) > I seem to remember someone saying that we should keep the iso2709 in > biblioitems.marc, because if you want to export e.g. 500k records in iso2709 > format, transforming all those records from marcxml on the fly would be > *really* heavy. But I might be wrong... Sure, if you want ISO2709 and are starting from MARCXML, converting to ISO2709 for an export will consume a bit more CPU than dumping pre-existing ISO2709 directly... but (a) the cost of conversion is not that much, (b) exports are relatively infrequent operations, (c) and as such, better to pay the cost during exports than to (admittedly slightly) slow down the operation of saving or modifying a bib record. I'm taking of removing the "In Discussion" status in the hopes that somebody takes up writing a patch. biblioitems.marc is used by misc/cronjobs/MARC21_parse_test.pl. Does this script still used? (In reply to Jonathan Druart from comment #4) > biblioitems.marc is used by misc/cronjobs/MARC21_parse_test.pl. Does this > script still used? I've never even heard of this script before, so I don't think that we use it. However, I suppose it's possible that someone uses it? Could we just remove the references to biblioitems.marc in this one script? (In reply to David Cook from comment #5) > (In reply to Jonathan Druart from comment #4) > > biblioitems.marc is used by misc/cronjobs/MARC21_parse_test.pl. Does this > > script still used? > > I've never even heard of this script before, so I don't think that we use > it. However, I suppose it's possible that someone uses it? > > Could we just remove the references to biblioitems.marc in this one script? Yes we could, if nobody uses the --fix option: 243 -f --fix Replace biblioitems.marcxml from data in marc field, default OFF (In reply to Jonathan Druart from comment #6) > Yes we could, if nobody uses the --fix option: > > 243 -f --fix Replace biblioitems.marcxml from data in marc field, > default OFF The script was added in 2009 and not touched since: http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=84ff3747b80d42914a8d570b6b4ff5667eab663b From the commit message: "When marcxml cannot be parsed into a MARC::Record object, the biblio is undisplayable and it obviously breaks many features in Koha. This script can test to parse every marcxml, and alert on failures. Optionally, the marcxml can be replaced from the marc field." Didn't we at some point switch from treating marc (ISO2709) as the authoritative format, to treating marcxml as the authoritative format? I'd say loosing the ability to recreate marcxml from marc is no big deal. I tend to agree with Magnus - I am not sure that this script makes that much sense. Just noticed that http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11084 is using biblioitems.marc as a string... that would block this bug... (In reply to Jonathan Druart from comment #6) > (In reply to David Cook from comment #5) > > (In reply to Jonathan Druart from comment #4) > > > biblioitems.marc is used by misc/cronjobs/MARC21_parse_test.pl. Does this > > > script still used? > > > > I've never even heard of this script before, so I don't think that we use > > it. However, I suppose it's possible that someone uses it? > > > > Could we just remove the references to biblioitems.marc in this one script? > > Yes we could, if nobody uses the --fix option: > > 243 -f --fix Replace biblioitems.marcxml from data in marc field, > default OFF I have just used it right now. For testing purpose I have removed the marcxml field. Using --fix, I was able to repopulate it from the marc. (In reply to Jonathan Druart from comment #10) > I have just used it right now. > For testing purpose I have removed the marcxml field. > Using --fix, I was able to repopulate it from the marc. But why did you remove the marcxml field at all? :p (In reply to David Cook from comment #11) > (In reply to Jonathan Druart from comment #10) > > I have just used it right now. > > For testing purpose I have removed the marcxml field. > > Using --fix, I was able to repopulate it from the marc. > > But why did you remove the marcxml field at all? :p I have no idea, really :D As of a topic on the ML (http://lists.koha-community.org/pipermail/koha-devel/2016-July/042821.html) removing this column would bring performance gains. Created attachment 54844 [details] [review] Bug 10455: Remove biblioitems.marc - DB changes Created attachment 54845 [details] [review] Bug 10455: Remove biblioitems.marc Any discussions about biblioitems.marc bring to the same conclusion: This field is useless and should be removed. We are storing MARC data into 2 different fields, that does not make sense. Test plan: Add / update / export / import /delete records should work as before Created attachment 54846 [details] [review] Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl This script seems to be unused and it won't be of any usefulness after the removal of biblioitems.marc Created attachment 54847 [details] [review] Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl Recently added, delete_records_via_leader.pl reads biblioitems.marc as a text field and search for record to delete regarding the leader 5. This can be acchieve doing the same thing on biblioitems.marcxml (will certainly be slower) waiting for a patch on bug 15537. Test plan: Confirm that this script works as before, to do so the easiest way would be to dump your DB before executing the update DB entry, execute the script to delete records, reinsert the DB, execute the udpate DB entry (remove biblioitems.marc), execute the script to delete records. You should get the same number of records deleted. Created attachment 54849 [details] [review] Bug 10455: (follow-up) Remove biblioitems.marc Created attachment 54850 [details] [review] Bug 10455: (follow-up) Remove biblioitems.marc Created attachment 54855 [details] [review] Bug 10455: (follow-up 2) Remove biblioitems.marc Created attachment 54856 [details] [review] Bug 10455: (follow-up 2) Remove biblioitems.marc Created attachment 54857 [details] [review] Bug 10455: (follow-up 2) Remove biblioitems.marc Created attachment 54858 [details] [review] Bug 10455: (follow-up 2) Remove biblioitems.marc Created attachment 54859 [details] [review] Bug 10455: (follow-up 3) Remove biblioitems.marc Created attachment 54860 [details] [review] Bug 10455: (follow-up 3) Remove biblioitems.marc Created attachment 54875 [details] [review] Bug 10455: Remove MARC21_utf8_flag_fix.pl Is this script still in use? It uses the biblioitems.marc field so if it's still useful it will need to be rewritten. Created attachment 54881 [details] [review] Bug 10455: Fix tests Created attachment 55401 [details] [review] Bug 10455: Fix tests Signed-off-by: Mason James <mtj@kohaaloha.com> Created attachment 55402 [details] [review] Bug 10455: Remove biblioitems.marc - DB changes Signed-off-by: Mason James <mtj@kohaaloha.com> Created attachment 55403 [details] [review] Bug 10455: Remove biblioitems.marc Any discussions about biblioitems.marc bring to the same conclusion: This field is useless and should be removed. We are storing MARC data into 2 different fields, that does not make sense. Test plan: Add / update / export / import /delete records should work as before Signed-off-by: Mason James <mtj@kohaaloha.com> Created attachment 55404 [details] [review] Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl This script seems to be unused and it won't be of any usefulness after the removal of biblioitems.marc Signed-off-by: Mason James <mtj@kohaaloha.com> Created attachment 55405 [details] [review] Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl Recently added, delete_records_via_leader.pl reads biblioitems.marc as a text field and search for record to delete regarding the leader 5. This can be acchieve doing the same thing on biblioitems.marcxml (will certainly be slower) waiting for a patch on bug 15537. Test plan: Confirm that this script works as before, to do so the easiest way would be to dump your DB before executing the update DB entry, execute the script to delete records, reinsert the DB, execute the udpate DB entry (remove biblioitems.marc), execute the script to delete records. You should get the same number of records deleted. Signed-off-by: Mason James <mtj@kohaaloha.com> Created attachment 55406 [details] [review] Bug 10455: (follow-up) Remove biblioitems.marc Signed-off-by: Mason James <mtj@kohaaloha.com> Created attachment 55407 [details] [review] Bug 10455: (follow-up 2) Remove biblioitems.marc Signed-off-by: Mason James <mtj@kohaaloha.com> Created attachment 55408 [details] [review] Bug 10455: (follow-up 3) Remove biblioitems.marc Signed-off-by: Mason James <mtj@kohaaloha.com> Created attachment 55409 [details] [review] Bug 10455: Remove MARC21_utf8_flag_fix.pl Is this script still in use? It uses the biblioitems.marc field so if it's still useful it will need to be rewritten. Signed-off-by: Mason James <mtj@kohaaloha.com> Created attachment 55410 [details] [review] Bug 10455: Fix tests Signed-off-by: Mason James <mtj@kohaaloha.com> I've tested Jonathan's 9 patches. Everything seems to work as expected, and the patches all look good to me I used the misc/devel/update_dbix_class_files.pl script to build the Koha/Scheme/Results/* files, before testing delete_records_via_leader.pl thanks Jonathan! :0) In: misc/cronjobs/delete_records_via_leader.pl I think you need: $schema->resultset('Biblioitem')->search( { marcxml => { LIKE => '%<leader>_____d%' } } ); Instead of: $schema->resultset('Biblioitem')->search( { marcxml => { LIKE => '<leader>_____d%' } } ); Otherwise seems to be working well Created attachment 55844 [details] [review] Bug 10455: Remove biblioitems.marc - DB changes Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 55845 [details] [review] Bug 10455: Remove biblioitems.marc Any discussions about biblioitems.marc bring to the same conclusion: This field is useless and should be removed. We are storing MARC data into 2 different fields, that does not make sense. Test plan: Add / update / export / import /delete records should work as before Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 55846 [details] [review] Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl This script seems to be unused and it won't be of any usefulness after the removal of biblioitems.marc Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 55847 [details] [review] Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl Recently added, delete_records_via_leader.pl reads biblioitems.marc as a text field and search for record to delete regarding the leader 5. This can be acchieve doing the same thing on biblioitems.marcxml (will certainly be slower) waiting for a patch on bug 15537. Test plan: Confirm that this script works as before, to do so the easiest way would be to dump your DB before executing the update DB entry, execute the script to delete records, reinsert the DB, execute the udpate DB entry (remove biblioitems.marc), execute the script to delete records. You should get the same number of records deleted. Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 55848 [details] [review] Bug 10455: (follow-up) Remove biblioitems.marc Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 55849 [details] [review] Bug 10455: (follow-up 2) Remove biblioitems.marc Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 55850 [details] [review] Bug 10455: (follow-up 3) Remove biblioitems.marc Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 55851 [details] [review] Bug 10455: Remove MARC21_utf8_flag_fix.pl Is this script still in use? It uses the biblioitems.marc field so if it's still useful it will need to be rewritten. Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 55852 [details] [review] Bug 10455: Fix tests Signed-off-by: Mason James <mtj@kohaaloha.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 55853 [details] [review] Bug 10455 (QA Followup) Fix record matching in misc/cronjobs/delete_records_via_leader.pl Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Woo team! Pushed to master for 16.11, thanks Jonathan! Great. Rather an 'enhancement' than a 'normal' bug for me. 16.11 is stable now, but this was pushed pre-release and won't be backported. This change has interesting side-effects ! - $sth = $dbh->prepare("UPDATE biblioitems SET marc=?,marcxml=? WHERE biblionumber=?"); - $sth->execute( $record->as_usmarc(), $record->as_xml_record($encoding), $biblionumber ); + $sth = $dbh->prepare("UPDATE biblioitems SET marcxml=? WHERE biblionumber=?"); + $sth->execute( $record->as_xml_record($encoding), $biblionumber ); It seems that since we do no longer call the sub as_usmarc, the automatic field lengths in the marc are not calculated anymore. This needs our attention. Will open a new report now! (In reply to Marcel de Rooy from comment #54) > This change has interesting side-effects ! > > - $sth = $dbh->prepare("UPDATE biblioitems SET marc=?,marcxml=? WHERE > biblionumber=?"); > - $sth->execute( $record->as_usmarc(), $record->as_xml_record($encoding), > $biblionumber ); > + $sth = $dbh->prepare("UPDATE biblioitems SET marcxml=? WHERE > biblionumber=?"); > + $sth->execute( $record->as_xml_record($encoding), $biblionumber ); > > It seems that since we do no longer call the sub as_usmarc, the automatic > field lengths in the marc are not calculated anymore. > This needs our attention. Will open a new report now! Trivial fix submitted. Please have a look and sign off. |