Bug 21901 - Foreign keys are missing on the serials and subscriptions tables
Summary: Foreign keys are missing on the serials and subscriptions tables
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 24213
Blocks: 5334
  Show dependency treegraph
 
Reported: 2018-11-28 15:09 UTC by Frank Hansen
Modified: 2020-05-07 20:45 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 21901: Add FK on subscription and serial tables (13.41 KB, patch)
2019-12-11 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21901: Add FK on subscription and serial tables (13.54 KB, patch)
2020-01-30 16:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21901: (QA follow-up) Ensure consistent data before adding keys (3.61 KB, patch)
2020-01-31 13:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21901: (QA follow-up) Ensure consistent data before adding keys (3.61 KB, patch)
2020-01-31 13:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21901: Add a warning if we removed entries (3.64 KB, patch)
2020-03-09 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21901: Add FK on subscription and serial tables (13.54 KB, patch)
2020-03-11 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21901: (QA follow-up) Ensure consistent data before adding keys (3.67 KB, patch)
2020-03-11 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21901: Add a warning if we removed entries (3.70 KB, patch)
2020-03-11 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21901: Add FK on subscription and serial tables (13.57 KB, patch)
2020-04-17 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21901: (QA follow-up) Ensure consistent data before adding keys (3.80 KB, patch)
2020-04-17 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21901: Add a warning if we removed entries (3.81 KB, patch)
2020-04-17 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21901: (QA follow-up) Add a few NOT NULL constraints in dbrev (1.46 KB, patch)
2020-04-17 08:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21901: (RM follow-up) Use TestBuilder to fix tests (9.25 KB, patch)
2020-04-17 12:37 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Hansen 2018-11-28 15:09:50 UTC
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.
Comment 1 Jonathan Druart 2019-12-11 08:47:34 UTC
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
Comment 2 Martin Renvoize 2020-01-30 16:11:38 UTC
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>
Comment 3 Martin Renvoize 2020-01-30 16:12:15 UTC
I like seeing foreign keys added.. works as expected.. Signing off.
Comment 4 Marcel de Rooy 2020-01-31 06:50:16 UTC
QAing
Comment 5 Marcel de Rooy 2020-01-31 06:56:26 UTC
(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.
Comment 6 Marcel de Rooy 2020-01-31 07:00:13 UTC
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 ;)
Comment 7 Jonathan Druart 2020-01-31 10:11:30 UTC
(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.
Comment 8 Marcel de Rooy 2020-01-31 10:16:00 UTC
(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..
Comment 9 Jonathan Druart 2020-01-31 10:25:59 UTC
What do you suggest then?
Comment 10 Marcel de Rooy 2020-01-31 10:48:45 UTC
<marcelr> ashimema: did you test on a large production DB ?
Comment 11 Martin Renvoize 2020-01-31 12:31:22 UTC
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.
Comment 12 Martin Renvoize 2020-01-31 13:22:34 UTC
Created attachment 98229 [details] [review]
Bug 21901: (QA follow-up) Ensure consistent data before adding keys
Comment 13 Martin Renvoize 2020-01-31 13:24:22 UTC
Created attachment 98231 [details] [review]
Bug 21901: (QA follow-up) Ensure consistent data before adding keys
Comment 14 Martin Renvoize 2020-01-31 13:25:26 UTC
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.
Comment 15 Jonathan Druart 2020-01-31 13:50:21 UTC
As I said this morning on #koha, I do not think we should silently delete those rows.
Comment 16 Marcel de Rooy 2020-02-03 08:01:22 UTC
(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.
Comment 17 Martin Renvoize 2020-02-03 08:35:15 UTC
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 :'(.
Comment 18 Katrin Fischer 2020-02-03 08:46:16 UTC
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?
Comment 19 Frank Hansen 2020-03-09 13:28:02 UTC
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.
Comment 20 Jonathan Druart 2020-03-09 14:26:25 UTC
Created attachment 100384 [details] [review]
Bug 21901: Add a warning if we removed entries
Comment 21 Jonathan Druart 2020-03-09 14:27:05 UTC
I have added a follow-up to display a warning message if entries are going to be deleted.
Comment 22 Martin Renvoize 2020-03-11 11:51:49 UTC
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>
Comment 23 Martin Renvoize 2020-03-11 11:51:52 UTC
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>
Comment 24 Martin Renvoize 2020-03-11 11:51:55 UTC
Created attachment 100547 [details] [review]
Bug 21901: Add a warning if we removed entries

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2020-03-11 11:52:36 UTC
This seems to cover all the bases to me now.. Signing off
Comment 26 Marcel de Rooy 2020-04-17 08:16:36 UTC
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.
Comment 27 Marcel de Rooy 2020-04-17 08:17:36 UTC
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>
Comment 28 Marcel de Rooy 2020-04-17 08:17:41 UTC
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>
Comment 29 Marcel de Rooy 2020-04-17 08:17:46 UTC
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>
Comment 30 Marcel de Rooy 2020-04-17 08:17:52 UTC
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>
Comment 31 Martin Renvoize 2020-04-17 09:22:05 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 32 Martin Renvoize 2020-04-17 12:37:48 UTC
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>
Comment 33 Martin Renvoize 2020-04-17 12:38:04 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 34 Joy Nelson 2020-05-07 20:45:24 UTC
not backported to 19.11.x