There's no need to have a marc column in the auth_header table anymore. We'll need to change at least the following files: ./installer/data/mysql/kohastructure.sql ./C4/AuthoritiesMarc.pm (AddAuthority, ) t/db_dependent/Linker_FirstMatch.t We'll also need to drop the column using updatedatabase.pl: ./installer/data/mysql/updatedatabase.pl
Actually, we'll also need to re-generate Koha/Schema/Result/AuthHeader.pm
Created attachment 111670 [details] [review] Bug 26684: Remove marc from auth_header for new installs
Created attachment 111671 [details] [review] Bug 26684: Remove marc column from existing databases
Created attachment 111672 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader
Created attachment 112016 [details] [review] Bug 26684: Remove marc from auth_header for new installs Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Created attachment 112018 [details] [review] Bug 26684: Remove marc from auth_header for new installs Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Created attachment 112019 [details] [review] Bug 26684: Remove marc column from existing databases Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Qa tools complaints: FAIL installer/data/mysql/atomicupdate/001-26684-drop_marc_column_from_auth_header.pl FAIL critic Code before strictures are enabled at line 1, column 1. See page 429 of PBP. FAIL file permissions File must have the exec flag FAIL valid Name "main::dbh" used only once: possible typo if you move the file to 'perl' many disappear, however, the bug number is missing from the update so it fails right now, also we should check the existence of the column before droppping
Created attachment 112087 [details] [review] Bug 26684: Remove marc from auth_header for new installs
Created attachment 112088 [details] [review] Bug 26684: Remove marc column from existing databases
Created attachment 112089 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader
Created attachment 112090 [details] [review] Bug 26684: Fix atomic update
QA scripts and updatedatabase.pl should be happy now
Patch no longer applies 8-(.. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 26684: Remove marc from auth_header for new installs Using index info to reconstruct a base tree... M C4/AuthoritiesMarc.pm M installer/data/mysql/kohastructure.sql Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/kohastructure.sql CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql Auto-merging C4/AuthoritiesMarc.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 26684: Remove marc from auth_header for new installs Also, is there a test plan? I have no idea how to test this one...
Created attachment 177445 [details] [review] Bug 26684: Remove marc from auth_header for new installs
Created attachment 177446 [details] [review] Bug 26684: Remove marc column from existing databases
Created attachment 177447 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader
Created attachment 177448 [details] [review] Bug 26684: Fix atomic update
The auth_header.marc column should definitely be remover. It is outdated, never used (only stored), and we have removed marc from biblio long time ago. Morover, it is of size blob (64K), which means it cannot properly host large ISO 2709 records (up to 99K). While saving large records we get warning: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'marc' at row 1 at /kohadevbox/koha/Koha/Object.pm line 172 and creation / modification is not properly performed.
Created attachment 177449 [details] [review] Bug 26684: Remove marc from Koha::Authority::to_api_mapping
Created attachment 177450 [details] [review] Bug 26684: Remove misc/exportauth.pl as a consequence of removing auth_header.marc Since we have misc/export_records.pl and we want to remove auth_header.marc, the ancient script misc/exportauth.pl can be removed as well (it is relied upon auth_header.marc).
Created attachment 177455 [details] [review] Bug 26684: Remove marc from auth_header for new installs Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177456 [details] [review] Bug 26684: Remove marc column from existing databases Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177457 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177458 [details] [review] Bug 26684: Fix atomic update Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177459 [details] [review] Bug 26684: Remove marc from Koha::Authority::to_api_mapping Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177460 [details] [review] Bug 26684: Remove misc/exportauth.pl as a consequence of removing auth_header.marc Since we have misc/export_records.pl and we want to remove auth_header.marc, the ancient script misc/exportauth.pl can be removed as well (it is relied upon auth_header.marc). Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Please add a test plan, and I'd be happy to test. (Replicate the issue (and what the issue is), apply patches, demonstrate that the issue is fixed.)
Sorry, I thought this was waiting for sign off (has now been signed off). A test plan would be ideal for the QA step, and a release note would be great to explain what this fixes and why.
Created attachment 177461 [details] Test data (one auth record) of size 92771 B
(In reply to David Nind from comment #29) > A test plan would be ideal for the QA step, and a release note would be > great to explain what this fixes and why. It is hard to say about a strict test plan. One aspect of problems causing by this column can be illustrates by the following sequence of steps. 1. Take the provided test auth record (100 Baggins, Bilbo) with lots od 670 fields, of size 92771 B: still in the limits of ISO 2709, but significantly bigger than blob size (64 KB). 2. Try to import the record into ktd installation with: misc/migration_tools/bulkmarcimport.pl -a -d -file Baggins_Bilbo.mrc -fk You should see a series of warnings: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'marc' at row 1 at /kohadevbox/koha/Koha/Object.pm line 172 Use of uninitialized value $originalid in concatenation (.) or string at ./misc/migration_tools/bulkmarcimport.pl line 520, <GEN3> line 1. ERROR: Insert authority failed: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'marc' at row 1 at /kohadevbox/koha/Koha/Object.pm line 172 3. Control in ktd --dbshell with: select * from auth_header; You should see one row with no data in marc nor in marcxml. 4. Apply the patch ; updatedatabase. 5. Repeat p. 2 and p. 3. You should get no warnings and expected (huge) result from MySQL. Other aspects should be evaluated by code reviewers. The koha_header.marc column should not be used in the code anymore with this patch.
Thanks Janusz!
Created attachment 177622 [details] [review] Bug 26684: Remove marc from auth_header for new installs Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 177623 [details] [review] Bug 26684: Remove marc column from existing databases Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 177624 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 177625 [details] [review] Bug 26684: Fix atomic update Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 177626 [details] [review] Bug 26684: Remove marc from Koha::Authority::to_api_mapping Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 177627 [details] [review] Bug 26684: Remove misc/exportauth.pl as a consequence of removing auth_header.marc Since we have misc/export_records.pl and we want to remove auth_header.marc, the ancient script misc/exportauth.pl can be removed as well (it is relied upon auth_header.marc). Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I agree about removing the column, but I don't believe this is a bug fix. Setting to enhancement (moving back in queue a little). Also: setting Assignee and QA contact...
(In reply to Katrin Fischer from comment #39) > I agree about removing the column, but I don't believe this is a bug fix. > Setting to enhancement (moving back in queue a little). > > Also: setting Assignee and QA contact... I wouldn't agree that it is just enhancement. IMO it is a real and quite serious bug. Please see comments # 19 and # 31. The limit of the size of blob makes auth_table and the whole authorities infrastructure fail in case of authority records of the size >= 64 KB (rare but real cases).
It's still quite a big change, so I am not sure it should be backported.
(In reply to Katrin Fischer from comment #41) > It's still quite a big change, so I am not sure it should be backported. I get the point. I do not insist. It seems serious and it is but it shouldn't hurt anybody. On the contrary, it could save maybe not the life, but data ;)
Applying: Bug 26684: Remove misc/exportauth.pl as a consequence of removing auth_header.marc Using index info to reconstruct a base tree... M misc/exportauth.pl Falling back to patching base and 3-way merge... CONFLICT (modify/delete): misc/exportauth.pl deleted in Bug 26684: Remove misc/exportauth.pl as a consequence of removing auth_header.marc and modified in HEAD. Version HEAD of misc/exportauth.pl left in tree. error: Failed to merge in the changes. Please rebase. I need to rebase another one on top..
Created attachment 178277 [details] [review] Bug 26684: Remove marc from auth_header for new installs Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 178278 [details] [review] Bug 26684: Remove marc column from existing databases Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 178279 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 178280 [details] [review] Bug 26684: Fix atomic update Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 178281 [details] [review] Bug 26684: Remove marc from Koha::Authority::to_api_mapping Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 178282 [details] [review] Bug 26684: Remove misc/exportauth.pl as a consequence of removing auth_header.marc Since we have misc/export_records.pl and we want to remove auth_header.marc, the ancient script misc/exportauth.pl can be removed as well (it is relied upon auth_header.marc). Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Well, it was a trivial rebase after all
DBIx::Class::Schema::Loader::make_schema_at(): Checksum mismatch in './/Koha/Schema/Result/AuthHeader.pm', the auto-generated part of the file has been modified outside of this loader. Aborting. Please run misc/devel/update_dbix_class_files.pl to prevent that when generating the new schema file.
Created attachment 178285 [details] [review] Bug 26684: {DBIx] Schema changes for AuthHeader.pm
I can see these are really old patches still using the old atomicupdate template. I will port them, but if you see such things, please help and save some RM time.
Pushed for 25.05! Well done everyone, thank you!
This change requires a change to our sample data used by ktd (reset_all) and Jenkins: auth_header.sql I've filed: https://gitlab.com/koha-community/koha-misc4dev/-/issues/86 We need an urgent fix please.