Bug 14053

Summary: Acquisition db tables are missing indexes
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: DatabaseAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, mtompset, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14132
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7923    
Attachments: Bug 14053 - Acquisition db tables are missing indexes
Bug 14053 - Acquisition db tables are missing indexes - atomicupdates
Bug 14053 - Acquisition db tables are missing indexes
Bug 14053 - Acquisition db tables are missing indexes - atomicupdates
[Signed-off] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates
Bug 14053 - Acquisition db tables are missing indexes
Bug 14053 - Acquisition db tables are missing indexes - atomicupdates
Bug 14053 - Fix cut and paste mistake
Bug 14053 - Acquisition db tables are missing indexes - atomicupdates
Bug 14053 - Acquisition db tables are missing indexes
[PASSED QA] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates
[PASSED QA] Bug 14053 - Acquisition db tables are missing indexes

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 Mark 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 Mark 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 Mark 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 Mark Tompsett 2015-04-30 18:49:10 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2015-04-30 18:49:17 UTC Comment hidden (obsolete)
Comment 8 Mark 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 Mark 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 Mark Tompsett 2015-05-13 16:48:54 UTC Comment hidden (obsolete)
Comment 16 Mark 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 Mark 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 Mark 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 Mark Tompsett 2015-05-13 18:05:58 UTC Comment hidden (obsolete)
Comment 20 Mark Tompsett 2015-05-13 18:06:03 UTC Comment hidden (obsolete)
Comment 21 Mark 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!