Issues table and foreign keys in general
13 Apr
2011
13 Apr
'11
2:38 a.m.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, I have noticed some things: 1. issues table has nullable borrowernumber and itemnumber; foreign key constraints are consequently declared as "ON DELETE SET NULL ON UPDATE SET NULL" 2. That is not an isolated issue, there are many cases where foreign key field/constraints are declared like that Can anyone shed more light, that looksvery wrong to me. Thanks, Srdjan -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk2k/pcACgkQX6p/D9UE7dy2fQCfZP/QeVQSb7VNNMJvA5yYWdBj AvAAn0fRuZ1KinfIiIIWCR+EgXhmYBu8 =2WSQ -----END PGP SIGNATURE-----
13 Apr
13 Apr
2:40 p.m.
New subject: Issues table and foreign keys in general
Hi, On Tue, Apr 12, 2011 at 9:38 PM, Srdjan <srdjan@catalyst.net.nz> wrote:
1. issues table has nullable borrowernumber and itemnumber; foreign key constraints are consequently declared as "ON DELETE SET NULL ON UPDATE SET NULL"
2. That is not an isolated issue, there are many cases where foreign key field/constraints are declared like that
Can anyone shed more light, that looksvery wrong to me.
And it is very wrong; this should be tightened up. As to why? Reasons best hidden in the mists of time, no doubt. In the case of issues, I think it's pretty clear that both itemnumber and borrowernumber should be non-NULL and have foreign key constraints. I suggest ON DELETE RESTRICT ON UPDATE CASCADE; if you are deleting an item record or a patron, the application logic needs to do the right thing if for whatever reason a library really wants to delete a record that has an outstanding loan on it. Regards, Galen -- Galen Charlton gmcharlt@gmail.com
16 Apr
16 Apr
2:56 p.m.
New subject: Issues table and foreign keys in general
Le 13/04/2011 15:40, Galen Charlton a écrit : > Hi, > > On Tue, Apr 12, 2011 at 9:38 PM, Srdjan <srdjan@catalyst.net.nz> wrote: >> 1. issues table has nullable borrowernumber and itemnumber; foreign >> key constraints are consequently declared as "ON DELETE SET NULL ON >> UPDATE SET NULL" >> >> 2. That is not an isolated issue, there are many cases where foreign >> key field/constraints are declared like that >> >> Can anyone shed more light, that looksvery wrong to me. > And it is very wrong; this should be tightened up. As to why? > Reasons best hidden in the mists of time, no doubt. no, no, they are hidden in my memory, I explain: koha 2.2 used myISAM tables, so no contraints. one of the first thing I did for 3.0 was to switch to innoDB and add constraints. When I did that, all issues were in issues table (including issues already returned) Then someone from LibLime added the oldissues table, and wrote the code to move issues to oldissues were applicable. The SET NULL *MUST BE KEPT* for statistic purposes on checked-in items : if you delete the item, you still can have usefull informations ! On issues, I agree that we have a problem now = it contains only checked-out items, that must not be deleted, otherwise the issue will never be checked-out. So we have 3 options : * use a trigger on item deletion, to move the issues to oldissues if needed. But with mySQL this is not possible * switch to ON DELETE CASCADE on item deletion, but that will loose some information for stats (could we accept this, considering deleting an item that has not been checked-in is very un-common ?) * add business logic to Koha to move the issue when the item is deleted. Isn't this already the case ? if it's already the case, then we don't care of the constraint on mySQL schema, as there won't be anything in issues, so the case will never happend. About > 2. That is not an isolated issue, there are many cases where foreign > key field/constraints are declared like that I wrote many of them with a reason that I think needed. Please tell me which you consider as a problem, i'll tell you : * why I choose this option if it's me * that I was not the author of this choice, the author made a mistake (and if git blame show you i'm the author of a buggy/wrong choice, i'll argue that someone stole my identity just for this mistake :D ) HTH -- Paul POULAIN http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc Tel : (33) 4 91 81 35 08
7:41 p.m.
New subject: Issues table and foreign keys in general
* Paul Poulain (paul.poulain@biblibre.com) wrote: > Le 13/04/2011 15:40, Galen Charlton a écrit : > > Hi, > > > > On Tue, Apr 12, 2011 at 9:38 PM, Srdjan <srdjan@catalyst.net.nz> wrote: > >> 1. issues table has nullable borrowernumber and itemnumber; foreign > >> key constraints are consequently declared as "ON DELETE SET NULL ON > >> UPDATE SET NULL" > >> > >> 2. That is not an isolated issue, there are many cases where foreign > >> key field/constraints are declared like that > >> > >> Can anyone shed more light, that looksvery wrong to me. > > And it is very wrong; this should be tightened up. As to why? > > Reasons best hidden in the mists of time, no doubt. > no, no, they are hidden in my memory, I explain: > koha 2.2 used myISAM tables, so no contraints. > one of the first thing I did for 3.0 was to switch to innoDB and add > constraints. When I did that, all issues were in issues table (including > issues already returned) > Then someone from LibLime added the oldissues table, and wrote the code > to move issues to oldissues were applicable. > > The SET NULL *MUST BE KEPT* for statistic purposes on checked-in items : > if you delete the item, you still can have usefull informations ! > On issues, I agree that we have a problem now = it contains only > checked-out items, that must not be deleted, otherwise the issue will > never be checked-out. > So we have 3 options : > * use a trigger on item deletion, to move the issues to oldissues if > needed. But with mySQL this is not possible > * switch to ON DELETE CASCADE on item deletion, but that will loose some > information for stats (could we accept this, considering deleting an > item that has not been checked-in is very un-common ?) > * add business logic to Koha to move the issue when the item is deleted. > Isn't this already the case ? if it's already the case, then we don't > care of the constraint on mySQL schema, as there won't be anything in > issues, so the case will never happend. I disagree, option 4 is to make the change suggested by Srdjan, in which the database will enforce integrity and make it impossible to delete an item that is checked out (has a row in issues). This covers us from mistakes in code, and from people making mistakes in the database. DELETE CASCADE is dangerous and should be avoided in this instance. It means if we make a mistake anywhere deleting an item that is checked out, the mistake will be compounded as we now delete the row in issues. The set NULL is nearly as bad, as we now null the column and still have no idea what it was linked to. > > About > > 2. That is not an isolated issue, there are many cases where foreign > > key field/constraints are declared like that > I wrote many of them with a reason that I think needed. Please tell me > which you consider as a problem, i'll tell you : > * why I choose this option if it's me > * that I was not the author of this choice, the author made a mistake > (and if git blame show you i'm the author of a buggy/wrong choice, i'll > argue that someone stole my identity just for this mistake :D ) > I think the database should never do this, I think our code should do it if it is needed. The database should stop us deleting things that are referred to elsewhere. We should clean up those references before trying to delete. The database should be the failsafe for us making mistakes, not allow us to make bigger mistakes :) Chris -- Chris Cormack Catalyst IT Ltd. +64 4 803 2238 PO Box 11-053, Manners St, Wellington 6142, New Zealand
19 Apr
19 Apr
1:41 p.m.
New subject: Issues table and foreign keys in general
Le 16/04/2011 20:41, Chris Cormack a écrit : > I disagree, option 4 is to make the change suggested by Srdjan, in which > the database will enforce integrity and make it impossible to delete an > item that is checked out (has a row in issues). This covers us from > mistakes in code, and from people making mistakes in the database. > DELETE CASCADE is dangerous and should be avoided in this instance. It > means if we make a mistake anywhere deleting an item that is checked > out, the mistake will be compounded as we now delete the row in issues. The set > NULL is nearly as bad, as we now null the column and still have no idea > what it was linked to. This problem is a functionnal problem: it's impossible to have an item that is checked out AND deleted. So we must find the best way for librarians to deal with the case they try to do this. I agree that today situation is wrong. I'm sure librarians will say : "hey, if I delete the item, it means no one has it anymore, why does not Koha just check-in the pending issue?" It's the same when you check-out an item that is already checked out : koha says "item is checked out for XXX, do the check-in before checking out to YYY?" I think we should do the same : "you're about to delete an item checked-out to XXX, do you confirm the check-in before deleting the item?" >> About >>> 2. That is not an isolated issue, there are many cases where foreign >>> key field/constraints are declared like that >> I wrote many of them with a reason that I think needed. Please tell me >> which you consider as a problem, i'll tell you : >> * why I choose this option if it's me >> * that I was not the author of this choice, the author made a mistake >> (and if git blame show you i'm the author of a buggy/wrong choice, i'll >> argue that someone stole my identity just for this mistake :D ) > I think the database should never do this, I think our code should do it if it > is needed. The database should stop us deleting things that are referred > to elsewhere. We should clean up those references before trying to > delete. The database should be the failsafe for us making mistakes, not > allow us to make bigger mistakes :) I'm not sure I understand what you mean here, so I explain what I wanted to achieve with the constraints. I just used DELETE CASCADE or SET NULL where it's logical. Like, when you delete a auth_tag_structure you delete the subfields. I used SET NULL only when I thought it was relevant to keep the entries (like anonymised issues) -- Paul POULAIN http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc Tel : (33) 4 91 81 35 08
5566
Age (days ago)
5572
Last active (days ago)
4 comments
4 participants
participants (4)
-
Chris Cormack -
Galen Charlton -
Paul Poulain -
Srdjan