[Koha-devel] Issues table and foreign keys in general

[Chris Cormack]

* Paul Poulain (paul.poulain at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: </pipermail/koha-devel/attachments/20110417/24efb288/attachment.pgp>