Bug 24741 - Recent creation of unique index on library_groups erroneously removes rows
Summary: Recent creation of unique index on library_groups erroneously removes rows
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P2 critical with 10 votes (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 21674
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-27 11:46 UTC by Stefan Berndtsson
Modified: 2020-03-04 14:54 UTC (History)
2 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 24741: (bug 21674 follow-up) Fix destructive update DB 19.12.00.017 (1.64 KB, patch)
2020-03-03 08:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24741: (bug 21674 follow-up) Fix destructive update DB 19.12.00.017 (1.86 KB, patch)
2020-03-03 15:37 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 24741: (bug 21674 follow-up) Fix destructive update DB 19.12.00.017 (1.92 KB, patch)
2020-03-04 10:51 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Berndtsson 2020-02-27 11:46:22 UTC
Bug 21674 introduces a new unique index on library_groups. Before this is done, removal of duplicate entries is done. This check for duplicates is wrong and removes non-duplicate entries as well.

The query being executed is:

DELETE FROM library_groups
WHERE id NOT IN (
  SELECT MIN(id)
  FROM ( SELECT * FROM library_groups ) AS lg
  WHERE parent_id IS NOT NULL
  GROUP BY parent_id, branchcode
)
AND parent_id IS NOT NULL;


To test:

Use Administration/Library groups, add two group on the same level. Add a library or two in each of them, then run the above query.

The second of the two groups, including all its libraries will now be removed.
Comment 1 David Gustafsson 2020-02-28 09:51:32 UTC
I'm not 100%, but think a correct query to remove duplicates could be written like:
 
DELETE FROM library_groups
WHERE id NOT IN (
  SELECT MIN(id)
  FROM ( SELECT * FROM library_groups ) AS lg
  GROUP BY parent_id, branchcode
)
AND NOT(parent_id IS NULL OR branchcode IS NULL);
Comment 2 Jonathan Druart 2020-03-02 10:09:54 UTC
I tried again.
For me it does not remove non-duplicate entries, but it removed an empty subgroup:

Before: https://snipboard.io/20aTY9.jpg
After : https://snipboard.io/lKzaeE.jpg

Subgroup 'yyyy' has been removed (not correct), however 'FPL' has not been removed from the first 'xx' group.

Did you notice another behavior?
Comment 3 Jonathan Druart 2020-03-02 10:15:48 UTC
(In reply to David Gustafsson from comment #1)
> I'm not 100%, but think a correct query to remove duplicates could be
> written like:
>  
> DELETE FROM library_groups
> WHERE id NOT IN (
>   SELECT MIN(id)
>   FROM ( SELECT * FROM library_groups ) AS lg
>   GROUP BY parent_id, branchcode
> )
> AND NOT(parent_id IS NULL OR branchcode IS NULL);

This query looks correct to me. Could you submit a patch?
Comment 4 Stefan Berndtsson 2020-03-02 11:56:56 UTC
(In reply to Jonathan Druart from comment #2)
> I tried again.
> For me it does not remove non-duplicate entries, but it removed an empty
> subgroup:
> 
> Before: https://snipboard.io/20aTY9.jpg
> After : https://snipboard.io/lKzaeE.jpg
> 
> Subgroup 'yyyy' has been removed (not correct), however 'FPL' has not been
> removed from the first 'xx' group.
> 
> Did you notice another behavior?

Yes.

In our case we had:

https://snipboard.io/cLhEY7.jpg

Since I've restored the data, I don't have an after-image, but after the migration ACQ was the only one left under __SEARCH_GROUPS__. All the others had been removed, with their entire substructure.

ACQ is the one with the lowest "id", and MIN(id) therefor kept that one.
Comment 5 Jonathan Druart 2020-03-03 08:59:33 UTC
Created attachment 99978 [details] [review]
Bug 24741: (bug 21674 follow-up) Fix destructive update DB 19.12.00.017

Previous version of the SQL query did not deal correctly with subgroup.

Test plan:
checkout the following commit to be just before 017
  commit 715da06db557edc4b1baa51cc278fdac362c01c4
  Bug 22868: DBRev 19.12.00.016
reset_all # recreate the DB
Create a complex tree of library groups, with several groups, subgroups,
etc.
When you add a new library, refresh the page to add it several time.
When ready, git checkout master, restart all the things, execute the
updatedatabase script, then go to the library groups page.
You should see that the duplicated entries have been removed, but
subgroups have not been deleted
Comment 6 Bernardo Gonzalez Kriegel 2020-03-03 15:37:22 UTC
Created attachment 100041 [details] [review]
Bug 24741: (bug 21674 follow-up) Fix destructive update DB 19.12.00.017

Previous version of the SQL query did not deal correctly with subgroup.

Test plan:
checkout the following commit to be just before 017
  commit 715da06db557edc4b1baa51cc278fdac362c01c4
  Bug 22868: DBRev 19.12.00.016
reset_all # recreate the DB
Create a complex tree of library groups, with several groups, subgroups,
etc.
When you add a new library, refresh the page to add it several time.
When ready, git checkout master, restart all the things, execute the
updatedatabase script, then go to the library groups page.
You should see that the duplicated entries have been removed, but
subgroups have not been deleted

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested with duplicated entries on groups and subgroups,
going back to the right commit and doing clean install.
Patch works, subgroups are preserved. No errors.
Comment 7 Nick Clemens 2020-03-04 10:51:25 UTC
Created attachment 100094 [details] [review]
Bug 24741: (bug 21674 follow-up) Fix destructive update DB 19.12.00.017

Previous version of the SQL query did not deal correctly with subgroup.

Test plan:
checkout the following commit to be just before 017
  commit 715da06db557edc4b1baa51cc278fdac362c01c4
  Bug 22868: DBRev 19.12.00.016
reset_all # recreate the DB
Create a complex tree of library groups, with several groups, subgroups,
etc.
When you add a new library, refresh the page to add it several time.
When ready, git checkout master, restart all the things, execute the
updatedatabase script, then go to the library groups page.
You should see that the duplicated entries have been removed, but
subgroups have not been deleted

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested with duplicated entries on groups and subgroups,
going back to the right commit and doing clean install.
Patch works, subgroups are preserved. No errors.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Martin Renvoize 2020-03-04 14:54:39 UTC
Nice work everyone!

Pushed to master for 20.05