Description
Fridolin Somers
2015-04-23 15:37:05 UTC
Created attachment 38568 [details] [review] Bug 14053 - Acquisition db tables are missing indexes Acquisition db tables have a lot of relational constraints and are often missing an index to have performance queries. This patch adds an index for all columns used in a forein key. Also adds an index on some columns very often used in search queries : 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 : - Check SQL syntax is correct - Try to compare performance after and before database update. I think query contained in C4::Acquistion::GetInvoices could be a good example Created attachment 38569 [details] [review] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates 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 on attachment 38569 [details] [review] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates Review of attachment 38569 [details] [review]: ----------------------------------------------------------------- Atomic updates worked fine. 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. Created attachment 38728 [details] [review] Bug 14053 - Acquisition db tables are missing indexes Acquisition db tables have a lot of relational constraints and are often missing an index to have performance queries. This patch adds an index for all columns used in a forein key. Also adds an index on some columns very often used in search queries : 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 : - Check SQL syntax is correct - 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> Created attachment 38729 [details] [review] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates Signed-off-by: Mark Tompsett <mtompset@hotmail.com> 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. Created attachment 38808 [details] [review] [Signed-off] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Atomic updates worked OK Signed-off-by: Marc Véron <veron@veron.ch> I have to review those patches because I discovered that adding a foreign key constraint automatically adds an index Created attachment 39135 [details] [review] 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 : - Check SQL syntax is correct - Try to compare performance after and before database update. I think query contained in C4::Acquistion::GetInvoices could be a good example Created attachment 39136 [details] [review] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates Patches updated 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. Created attachment 39153 [details] [review] Bug 14053 - Fix cut and paste mistake (255) does not belong in the key definition. TEST PLAN ---------- 1) Back up db. 2) $ git reset --hard origin/master 3) $ git bz apply 14053 -- do not apply this patch. 4) in your mysql client: > DROP DATABASE koha_library; > CREATE DATABASE koha_library; > QUIT; 5) Navigate to staff client -- blows up in step 3 6) $ git bz apply 14053 -- apply just this final patch 7) repeat step 4. 8) Navigate to staff client -- everything proceeds smoothly. 9) run koha qa test tools Comment on attachment 39153 [details] [review] Bug 14053 - Fix cut and paste mistake Nope that's not the solution. Sorry, my bad. 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. (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. Created attachment 39154 [details] [review] Bug 14053 - Acquisition db tables are missing indexes - atomicupdates Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 39155 [details] [review] 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> 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). (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? 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> 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> Patch pushed to master. Thanks Frido! |