Bug 14053 - Acquisition db tables are missing indexes
Summary: Acquisition db tables are missing indexes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin SOMERS
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 7923
  Show dependency treegraph
 
Reported: 2015-04-23 15:37 UTC by Fridolin SOMERS
Modified: 2016-12-05 21:23 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14053 - Acquisition db tables are missing indexes (5.83 KB, patch)
2015-04-27 15:15 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 14053 - Acquisition db tables are missing indexes - atomicupdates (5.26 KB, patch)
2015-04-27 15:15 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 14053 - Acquisition db tables are missing indexes (5.88 KB, patch)
2015-04-30 18:49 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14053 - Acquisition db tables are missing indexes - atomicupdates (5.31 KB, patch)
2015-04-30 18:49 UTC, M. Tompsett
Details | Diff | Splinter Review
[Signed-off] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates (5.48 KB, patch)
2015-05-04 12:08 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14053 - Acquisition db tables are missing indexes (4.04 KB, patch)
2015-05-13 14:17 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 14053 - Acquisition db tables are missing indexes - atomicupdates (3.35 KB, patch)
2015-05-13 14:17 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 14053 - Fix cut and paste mistake (1.58 KB, patch)
2015-05-13 16:48 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14053 - Acquisition db tables are missing indexes - atomicupdates (3.40 KB, patch)
2015-05-13 18:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14053 - Acquisition db tables are missing indexes (4.69 KB, patch)
2015-05-13 18:06 UTC, M. Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates (3.47 KB, patch)
2015-05-31 20:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14053 - Acquisition db tables are missing indexes (4.76 KB, patch)
2015-05-31 20:55 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2015-04-23 15:37:05 UTC
Acquisition db tables have a lot of relational constraints and are often missing an index to have performance queries.
Comment 1 Fridolin SOMERS 2015-04-27 15:15:35 UTC Comment hidden (obsolete)
Comment 2 Fridolin SOMERS 2015-04-27 15:15:49 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2015-04-29 04:54:26 UTC
Comment on attachment 38568 [details] [review]
Bug 14053 - Acquisition db tables are missing indexes

Review of attachment 38568 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/kohastructure.sql
@@ +2934,5 @@
> +  KEY `budget_parent_id` (`budget_parent_id`),
> +  KEY `budget_code` (`budget_code`),
> +  KEY `budget_branchcode` (`budget_branchcode`),
> +  KEY `budget_period_id` (`budget_period_id`),
> +  KEY `budget_owner_id` (`budget_owner_id`),

I believe this trailing comma is an issue.
I dropped koha_library. created it. then run web install.
Comment 4 M. Tompsett 2015-04-29 04:55:27 UTC
Comment on attachment 38569 [details] [review]
Bug 14053 - Acquisition db tables are missing indexes - atomicupdates

Review of attachment 38569 [details] [review]:
-----------------------------------------------------------------

Atomic updates worked fine.
Comment 5 M. Tompsett 2015-04-30 18:48:38 UTC
I checked the SQL syntax. I tweaked the SQL to not fail for me.

Current master needs:
sudo apt-get install libgraphics-colornames-perl
sudo apt-get install libimage-size-perl
sudo apt-get install libhtml-tidy-perl
cpan2deb PDF::Writer
sudo dpkg -i libpdf-writer-perl_0.06-1_all.deb
cpan2deb PDF::FromHTML
sudo dpkg -i libpdf-fromhtml-perl_0.31-1_all.deb

I did not check performance, though I doubt this would hinder it.
Comment 6 M. Tompsett 2015-04-30 18:49:10 UTC Comment hidden (obsolete)
Comment 7 M. Tompsett 2015-04-30 18:49:17 UTC Comment hidden (obsolete)
Comment 8 M. Tompsett 2015-04-30 18:52:01 UTC
Fridolin, could you please confirm my tweaks? See comment #5 about issues you may encounter. If it works for you, then feel free to set it to signed off.
Comment 9 Marc Véron 2015-05-04 12:08:50 UTC Comment hidden (obsolete)
Comment 10 Fridolin SOMERS 2015-05-04 13:13:46 UTC
I have to review those patches because I discovered that adding a foreign key constraint automatically adds an index
Comment 11 Fridolin SOMERS 2015-05-13 14:17:35 UTC Comment hidden (obsolete)
Comment 12 Fridolin SOMERS 2015-05-13 14:17:47 UTC Comment hidden (obsolete)
Comment 13 Fridolin SOMERS 2015-05-13 14:18:19 UTC
Patches updated
Comment 14 M. Tompsett 2015-05-13 16:36:44 UTC
Comment on attachment 39135 [details] [review]
Bug 14053 - Acquisition db tables are missing indexes

Review of attachment 39135 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/kohastructure.sql
@@ +2903,4 @@
>    PRIMARY KEY  (`id`),
>    KEY `listprice` (`listprice`),
>    KEY `invoiceprice` (`invoiceprice`),
> +  KEY `name` (`name`(255)),

Remove (255)
This causes a failure.
Comment 15 M. Tompsett 2015-05-13 16:48:54 UTC Comment hidden (obsolete)
Comment 16 M. Tompsett 2015-05-13 16:50:21 UTC
Comment on attachment 39153 [details] [review]
Bug 14053 - Fix cut and paste mistake

Nope that's not the solution. Sorry, my bad.
Comment 17 M. Tompsett 2015-05-13 17:13:06 UTC
Comment on attachment 39135 [details] [review]
Bug 14053 - Acquisition db tables are missing indexes

Review of attachment 39135 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/kohastructure.sql
@@ +2903,4 @@
>    PRIMARY KEY  (`id`),
>    KEY `listprice` (`listprice`),
>    KEY `invoiceprice` (`invoiceprice`),
> +  KEY `name` (`name`(255)),

This wasn't the problem.

@@ +2933,5 @@
> +  KEY `budget_parent_id` (`budget_parent_id`),
> +  KEY `budget_code` (`budget_code`),
> +  KEY `budget_branchcode` (`budget_branchcode`),
> +  KEY `budget_period_id` (`budget_period_id`),
> +  KEY `budget_owner_id` (`budget_owner_id`),

This comma is the problem! I'm going to remove it from your patch and sign off.
Comment 18 M. Tompsett 2015-05-13 17:14:31 UTC
(In reply to M. Tompsett from comment #3)
> Comment on attachment 38568 [details] [review] [review]
> Bug 14053 - Acquisition db tables are missing indexes
> 
> Review of attachment 38568 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: installer/data/mysql/kohastructure.sql
> @@ +2934,5 @@
> > +  KEY `budget_parent_id` (`budget_parent_id`),
> > +  KEY `budget_code` (`budget_code`),
> > +  KEY `budget_branchcode` (`budget_branchcode`),
> > +  KEY `budget_period_id` (`budget_period_id`),
> > +  KEY `budget_owner_id` (`budget_owner_id`),
> 
> I believe this trailing comma is an issue.
> I dropped koha_library. created it. then run web install.

Please fix your branch. This comma bug was introduced again.
Comment 19 M. Tompsett 2015-05-13 18:05:58 UTC Comment hidden (obsolete)
Comment 20 M. Tompsett 2015-05-13 18:06:03 UTC Comment hidden (obsolete)
Comment 21 M. Tompsett 2015-05-13 18:09:05 UTC
NOTE: I did not compare performance issues, though it conceptually makes sense. My sign offs are based on tweaked (I fixed the comma bug) patches and a more explicit test plan (to prevent the comma bug from returning).
Comment 22 Jonathan Druart 2015-05-26 15:51:59 UTC
(In reply to Fridolin SOMERS from comment #1)
>  - Try to compare performance after and before database update. I think
> query contained in C4::Acquistion::GetInvoices could be a good example

I only have ~20 invoices in my DB, not sure it will be enough.
Don't you have number you would have measured? In production for instance?
Comment 23 Katrin Fischer 2015-05-31 20:55:10 UTC
Created attachment 39719 [details] [review]
[PASSED QA] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2015-05-31 20:55:24 UTC
Created attachment 39720 [details] [review]
[PASSED QA] Bug 14053 - Acquisition db tables are missing indexes

Acquisition db tables are missing some indexes to have
performance queries.

This patch adds an index on some columns very often
used in search queries, such as aqbooksellers.name and
aqbudgets.budget_code.

Also adds an index on aqorders.orderstatus, very often used
with hardcoded value like 'cancelled', in various queries.

Test plan :
1) Back up database
2) $ git reset --hard origin/master
3) $ git bz apply 14053
4) In your mysql client
   > DROP DATABASE koha_library;
   > CREATE DATABASE koha_library;
   > QUIT;
   -- Obviously you may need to vary koha_library :)
5) Navigate to staff client
   -- should be able to set up the DB just fine.
   -- this will catch the comma bug that keeps coming in.
6) $ git reset --hard origin/master
7) Repeat step 4
8) Navigate to staff client
   -- nothing tested, but we need the DB set up.
9) $ ./installer/data/mysql/updatedatabase.pl
   -- atomic updates run without issue.
10) run koha qa test tools
11) Restore DB
12) Try to compare performance after and before
    database update. I think query contained in
    C4::Acquistion::GetInvoices could be a good example

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Tomás Cohen Arazi 2015-06-08 14:10:37 UTC
Patch pushed to master.

Thanks Frido!