Because two indexes are missing, the SQL searches in C4::Serials perform very slow. Since theres no indexes for subscription.biblionumber and serial.subscriptionid, the SQL server will do a full table scans on both tables, which takes a very long time, especially if you have 100,000 subscriptions and approximately one million series registered. To improve response time, at least two indexes are required. I will therefore suggest these two indexes to be built: alter table serial add index idx_serial_subscriptionid using btree (subscriptionid); alter table subscription add index idx_subscription_biblionumber (biblionumber); It's also strange that the subscriptionid in serial is a varchar(100). In subscriptions table, subscriptionid is an int. I think subscriptionid in serial should be an int for safe and for best performance.
Created attachment 96181 [details] [review] Bug 21901: Add FK on subscription and serial tables In order to improve performance in the serial modules and add DB constraints, this patch is going to add missing foreign key on the following columns: * serial.biblionumber * serial.subscription * subscriptionhistory.biblionumber * subscriptionhistory.subscriptionid * subscription.biblionumber Once done, some code can be removed from the Del* subroutines, as the ON CASCASE clause will make the RDBMS handles the deletions. Test plan: 0/ It would be useful to test the update DB entry on a big and old production DB, to make sure the constraints will be added correctly. We could remove the entries before creating the constraints, but it can be unecessary 1/ Make sure you can recreate a fresh install with the kohastructure.sql from this patch 2/ Make sure you can upgrade from a master install 3/ Create a subscription, serial, etc. and delete the biblio => The subscription/serials should have been deleted from the DB 4/ Create a subscription, serial, etc. and delete the subscription => The serials should have been deleted from the DB
Created attachment 98177 [details] [review] Bug 21901: Add FK on subscription and serial tables In order to improve performance in the serial modules and add DB constraints, this patch is going to add missing foreign key on the following columns: * serial.biblionumber * serial.subscription * subscriptionhistory.biblionumber * subscriptionhistory.subscriptionid * subscription.biblionumber Once done, some code can be removed from the Del* subroutines, as the ON CASCASE clause will make the RDBMS handles the deletions. Test plan: 0/ It would be useful to test the update DB entry on a big and old production DB, to make sure the constraints will be added correctly. We could remove the entries before creating the constraints, but it can be unecessary 1/ Make sure you can recreate a fresh install with the kohastructure.sql from this patch 2/ Make sure you can upgrade from a master install 3/ Create a subscription, serial, etc. and delete the biblio => The subscription/serials should have been deleted from the DB 4/ Create a subscription, serial, etc. and delete the subscription => The serials should have been deleted from the DB Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I like seeing foreign keys added.. works as expected.. Signing off.
QAing
(In reply to Jonathan Druart from comment #1) > 0/ It would be useful to test the update DB entry on a big and old > production DB, to make sure the constraints will be added correctly. > We could remove the entries before creating the constraints, but it can > be unecessary Since it is theoretically possible that a delete was not completely executed somehow leaving some bad keys in the table, it would be useful to remove the entries and not assume they are not. Cannot be verified by one or two tests.
Inclined to think that could be a blocker. We dont want large installs fail on this step. Feel free to change the status again, well argumented ;)
(In reply to Marcel de Rooy from comment #6) > Inclined to think that could be a blocker. We dont want large installs fail > on this step. > Feel free to change the status again, well argumented ;) I let that kind of comment to get help on it, not to see people asking me to unlock the issue. We need to test it on a old and big production DB, as I said. I do not have one to try.
(In reply to Jonathan Druart from comment #7) > (In reply to Marcel de Rooy from comment #6) > > Inclined to think that could be a blocker. We dont want large installs fail > > on this step. > > Feel free to change the status again, well argumented ;) > > I let that kind of comment to get help on it, not to see people asking me to > unlock the issue. > We need to test it on a old and big production DB, as I said. I do not have > one to try. Yes, but what I am saying is that one such a test does not prove anything. If it does not contain referential errors, it goes fine. Does not prove that nobody else has such problems..
What do you suggest then?
<marcelr> ashimema: did you test on a large production DB ?
I tested on 4 large clones of production Databases.. none of them failed in the way you are suggesting.. but it may not be a terrible idea to add a line for each foreign key addition to 'clean' the bad data. I did find that in a fair few cases that malformed date fields would crash the update... I'm not sure whether to fold in a check and clean for such fields into this patch or open another bug to be honest.. I think I'm inclined to fold it in here to ensure the DB update will actually run.
Created attachment 98229 [details] [review] Bug 21901: (QA follow-up) Ensure consistent data before adding keys
Created attachment 98231 [details] [review] Bug 21901: (QA follow-up) Ensure consistent data before adding keys
Run against a series of large databases.. adding the additional security of the followup did not result in any discernible performance degradation of the update so I think it's a worthwhile addition.
As I said this morning on #koha, I do not think we should silently delete those rows.
(In reply to Jonathan Druart from comment #15) > As I said this morning on #koha, I do not think we should silently delete > those rows. Instead of deleting we could look at setting to NULL. Not sure if it would be very valuable? We could dump the original serial tables to a backup file allowing a local developer to restore information; not sure if anyone would.
Setting to NULL leaves us with broken data and worse an avenue to new broken data being input. Personally, as I couldn't' find any cases where a delete would actually take place (and I went back to some koha versions many years old) and the data being deleted would lead to errors in the client as relations would be broken I feel it's justified to leave the delete in place... open to being countered as ever but I'd rather this didn't end up stuck in perpetuity as so many FK bugs do :'(.
If I undertand this correctly the problem might be cases where there is incomplete data? like a subscription without subscription history? Or a serial entry with a link to a nonexisting subscription? We store the user's name in subscription... so if we are very strict we might not want to dump it to the file system to be forgotten about. If we set to null, we need to make the db structure more lenient... I guess dumping might be the best option. How likely is it those cases exist?
Hi, > 0/ It would be useful to test the update DB entry on a big and old > production DB, to make sure the constraints will be added correctly. Here in Lund we have tested your fix on a clone of our production DB with over 90000 subscriptions and the short story is that we are satisfied of what the fix accomplish. > 2/ Make sure you can upgrade from a master install This step went well, no errors. I think it is good that orphaned entries is removed before creating the FK constraints. It turned out in our case there were data that were migrated from our previous system by mistake. So the subscription was actually deleted in the old system. So, we didn't lose any important data anyway. > 3/ Create a subscription, serial, etc. and delete the biblio > => The subscription/serials should have been deleted from the DB The fix does what it should. And the performance has also improved significantly which is important from my perspective. > 4/ Create a subscription, serial, etc. and delete the subscription > => The serials should have been deleted from the DB The fix does what it should. Works fine.
Created attachment 100384 [details] [review] Bug 21901: Add a warning if we removed entries
I have added a follow-up to display a warning message if entries are going to be deleted.
Created attachment 100545 [details] [review] Bug 21901: Add FK on subscription and serial tables In order to improve performance in the serial modules and add DB constraints, this patch is going to add missing foreign key on the following columns: * serial.biblionumber * serial.subscription * subscriptionhistory.biblionumber * subscriptionhistory.subscriptionid * subscription.biblionumber Once done, some code can be removed from the Del* subroutines, as the ON CASCASE clause will make the RDBMS handles the deletions. Test plan: 0/ It would be useful to test the update DB entry on a big and old production DB, to make sure the constraints will be added correctly. We could remove the entries before creating the constraints, but it can be unecessary 1/ Make sure you can recreate a fresh install with the kohastructure.sql from this patch 2/ Make sure you can upgrade from a master install 3/ Create a subscription, serial, etc. and delete the biblio => The subscription/serials should have been deleted from the DB 4/ Create a subscription, serial, etc. and delete the subscription => The serials should have been deleted from the DB Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 100546 [details] [review] Bug 21901: (QA follow-up) Ensure consistent data before adding keys Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 100547 [details] [review] Bug 21901: Add a warning if we removed entries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This seems to cover all the bases to me now.. Signing off
DELETE FROM serial WHERE biblionumber NOT IN (SELECT biblionumber FROM biblio) Wouldnt it be better to left join biblio here and check biblionumber is null in the where condition ? Same for another one.
Created attachment 103122 [details] [review] Bug 21901: Add FK on subscription and serial tables In order to improve performance in the serial modules and add DB constraints, this patch is going to add missing foreign key on the following columns: * serial.biblionumber * serial.subscription * subscriptionhistory.biblionumber * subscriptionhistory.subscriptionid * subscription.biblionumber Once done, some code can be removed from the Del* subroutines, as the ON CASCASE clause will make the RDBMS handles the deletions. Test plan: 0/ It would be useful to test the update DB entry on a big and old production DB, to make sure the constraints will be added correctly. We could remove the entries before creating the constraints, but it can be unecessary 1/ Make sure you can recreate a fresh install with the kohastructure.sql from this patch 2/ Make sure you can upgrade from a master install 3/ Create a subscription, serial, etc. and delete the biblio => The subscription/serials should have been deleted from the DB 4/ Create a subscription, serial, etc. and delete the subscription => The serials should have been deleted from the DB Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 103123 [details] [review] Bug 21901: (QA follow-up) Ensure consistent data before adding keys Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 103124 [details] [review] Bug 21901: Add a warning if we removed entries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 103125 [details] [review] Bug 21901: (QA follow-up) Add a few NOT NULL constraints in dbrev We need to specify NOT NULL on subscription and -history as well. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Nice work everyone! Pushed to master for 20.05
Created attachment 103151 [details] [review] Bug 21901: (RM follow-up) Use TestBuilder to fix tests A few tests failed due to missing values for foreign key relationships. Switching to TestBuilder for data mocking resolves this elegantly. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
not backported to 19.11.x
See bug 27842 - this potentially causes data loss during upgrades