(Raised from bug 17782) DBIC needs a primary key to work correctly. We could be tempt to make borrowernumber a primary key, but a use case could bring thing. Indeed if a table is empty and the mysql (mariadb behaves identically) server restarted, the Auto Increment (AI) is reset. Problematic use case: 1. Install Koha 2. Create a patron (borrowers.borrowernumber = 1) 3. Delete it (deletedborrowers.borrowernumer = 1, and AI is set to 2 4. Restart mysql => borrowers is empty so AI is reset 5. Create a patron (borrowers.borrowernumber = 1) 6. Delete it => We will try to insert a new row with an existing borrowernumber (1) into the deletedborrowers table: BOOM Solution 1: Do not care about this problematic use case and prevent to delete the last patron. Solution 2: Add a AI primary key (id) to deletedborrowers => The 2 tables borrowers and deletedborrowers will differ, bugs will be expected Solution 3: Deal with that at code level: when a patron is created, guess the borrowernumber it will get and check if it does not already exist in the deletedborrowers table. If so, force it before inserting it Solution 4: Do not enforce this constraint at DB level but set the primary key to DBIC schema: Koha::Schema::Result::Deletedborrower: __PACKAGE__->set_primary_key("borrowernumber"); Problem: Koha::Database->new->schema->resultset('Deletedborrower')->find(42); will raise a warning "DBIx::Class::Storage::DBI::select_single(): Query returned more than one row. SQL that returns multiple rows is DEPRECATED for ->find and ->single" if 42 is duplicated, and the first matching row will be picked Another solution?
Created attachment 59616 [details] [review] Bug 18003: Example of why we would need a PK on deletedborrowers These tests do not pass: DBIx::Class::ResultSource::_pri_cols_or_die(): Operation requires a primary key to be declared on 'Deletedborrower' via set_primary_key at t/db_dependent/Koha/Patrons.t line 329
(In reply to Jonathan Druart from comment #0) > > Solution 1: > Do not care about this problematic use case and prevent to delete the last > patron. I do not like this - it escalates the internal technical problem to end user... > Solution 2: > Add a AI primary key (id) to deletedborrowers > => The 2 tables borrowers and deletedborrowers will differ, bugs will be > expected It could be possible I think, but probably not easy to make it > Solution 3: > Deal with that at code level: when a patron is created, guess the > borrowernumber it will get and check if it does not already exist in the > deletedborrowers table. If so, force it before inserting it > A bit complicated, but probably the best one > Solution 4: > Do not enforce this constraint at DB level but set the primary key to DBIC > schema: > Koha::Schema::Result::Deletedborrower: > __PACKAGE__->set_primary_key("borrowernumber"); > Problem: > Koha::Database->new->schema->resultset('Deletedborrower')->find(42); > will raise a warning "DBIx::Class::Storage::DBI::select_single(): Query > returned more than one row. SQL that returns multiple rows is DEPRECATED > for ->find and ->single" if 42 is duplicated, and the first matching row > will be picked I do not like this too > > Another solution? So I am for 2 or 3, 2 was what came first to my mind, but the 3 is better I think...
> Another solution? Merge borrowers and deletedborrowers to one table and mark the deleted one with a flag
Does the problem mentioned on top only occur on an empty borrowers table with a mysql restart? Supposing that this is the case and it is quite rare, can't we just use borrowernumber as the primary key on borrowers and deletedborrowers, obviously having no constraints on deletedborrowers. We could even add a check for borrowernumber when creating a patron, and update the borrowernumber if needed to not match deletedborrowers? Ugly, but fixing some problems.
We discussed if it also happens when you delete the last added borrower and then restart - not sure what the answer was.
(In reply to Katrin Fischer from comment #5) > We discussed if it also happens when you delete the last added borrower and > then restart - not sure what the answer was. Indeed this is a case that sucks as well.
See also "Merge borrowers and deletedborrowers tables" topic on koha-devel 1 year ago: http://lists.koha-community.org/pipermail/koha-devel/2016-January/042207.html
(In reply to Jonathan Druart from comment #7) > See also "Merge borrowers and deletedborrowers tables" topic on koha-devel 1 > year ago: > http://lists.koha-community.org/pipermail/koha-devel/2016-January/042207.html We don't provide a way to recover a deleted borrower, do we?
I'd +1 the merging of tables.. but can see the annoyance it would cause people who have written reports using said tables.. was that the reason it was not done a year ago?
(In reply to Martin Renvoize from comment #9) > I'd +1 the merging of tables.. but can see the annoyance it would cause > people who have written reports using said tables.. was that the reason it > was not done a year ago? Can't we creaste a view for those?
I like the idea of creating a view.. but the only method that would work seamlessly would be to create an entirely new 'borrowers' table with a new name and create a view for each of the old borrowers and deletedborrowers tables. Not a massive issue, but worth thinking about.. I kinda like the idea of 'doing it properly, and using views to maintain backwards compatibility with old reports'
(In reply to Tomás Cohen Arazi from comment #10) > (In reply to Martin Renvoize from comment #9) > > I'd +1 the merging of tables.. but can see the annoyance it would cause > > people who have written reports using said tables.. was that the reason it > > was not done a year ago? > > Can't we creaste a view for those? For the info: I have created a view on bug 17835 (different need, different problem, but it is worth noting).