|
Description
Laura Escamilla
2023-07-06 20:38:50 UTC
The solution I found for this at the time was prevented, I believe, by ONLY_FULL_GROUP_BY in https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/Database.pm#L106 There is currently a FIXME in https://github.com/Koha-Community/Koha/blob/master/Koha/Illbackend.pm This FIXME is for the samne issue reported by this bug. Need help with this. Created attachment 153289 [details] [review] Bug 34223: PROOF OF CONCEPT The following command will (hopefully) reset your ILL data and create 10k fake ILL requests (run this in DEV KTD only). 1) On an empty k-t-d, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-data.sh) 2) Pet a cat 3) Visit /cgi-bin/koha/ill/ill-requests.pl and select a backend on the left side filters 4) Notice how the status filter takes a while (3-5 secs) to load 5) Apply patch and koha-plack --restart kohadev 6) Repeat 3, notice how the status filter now loads fast The patch is just a proof of concept detailing the solution I had at the time of developing this but was ultimately prevented by the ONLY_FULL_GROUP_BY. The patch is NOT the fix, as I don't think we want to change Database.pm for this. It's just demonstrating how it should work. Created attachment 153953 [details] [review] Bug 34223: Fix filter performance We are now only fetching the existing statuses for the provided backend. Then, we fetch 1 illrequest object to make use of ->strings_map. Before, we were fetching all illrequests in the database and removing redundant afterwards @Laura Escamilla Can you test my most recent patch? I've tested it myself with 10k fake requests and it fixed the issue. Found an issue with the patch. Trying to fix. Created attachment 153956 [details] [review] Bug 34223: Fix filter performance We are now only fetching the existing statuses for the provided backend. Then, we fetch 1 illrequest object to make use of ->strings_map. Before, we were fetching all illrequests in the database and removing redundant afterwards Created attachment 153957 [details] [review] Bug 34223: Work around ONLY_FULL_GROUP_BY in Illbackend This patch re-works the query in the existing_statuses method to remove the FIXME and improve performance. We pass an SQL literal into the query to make it explicit which illrequest_id we're looking for (which dampens the SQL warning) even though we really don't mind which request is returned here. Test plan: The following command will (hopefully) reset your ILL data and create 10k fake ILL requests (run this in DEV KTD only). 1) On an empty k-t-d, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-data.sh) 2) Pet a cat 3) Visit /cgi-bin/koha/ill/ill-requests.pl and select a backend on the left side filters 4) Notice how the status filter takes a while (3-5 secs) to load 5) Apply patch and koha-plack --restart kohadev 6) Repeat 3, notice how the status filter now loads fast Pedro and I had another look at this and I managed to come up with a slightly cleaner and I believe more performant alternative now attached above. We're not getting just the status's we want in just one DB hit.. it's a fun work around for the `ONLY_FULL_GROUP_BY` problem. Please test as per Pedro's test plan.. now copied into my submission too. Issue spotted.. Just merging parts of Pedro's patch and parts of my own and we'll cross-sign each others. Also.. there appears to be no unit tests for this method.. so I'm going to have a look at writing one. Created attachment 153961 [details] [review] Bug 34223: (follow-up) Restore status_alias handling By reducing the original call to all distinct 'status' we also removed the status_alias combinations. This patch adds an additional distinct query to fetch all 'status_alias' and add them to the returned data structure. Created attachment 153962 [details] [review] Bug 34223: Work around ONLY_FULL_GROUP_BY in Illbackend This patch re-works the query in the existing_statuses method to remove the FIXME and improve performance. We pass an SQL literal into the query to make it explicit which illrequest_id we're looking for (which dampens the SQL warning) even though we really don't mind which request is returned here. Test plan: The following command will (hopefully) reset your ILL data and create 10k fake ILL requests (run this in DEV KTD only). 1) On an empty k-t-d, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-data.sh) 2) Pet a cat 3) Visit /cgi-bin/koha/ill/ill-requests.pl and select a backend on the left side filters 4) Notice how the status filter takes a while (3-5 secs) to load 5) Apply patch and koha-plack --restart kohadev 6) Repeat 3, notice how the status filter now loads fast Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 153963 [details] [review] Bug 34223: (follow-up) Restore status_alias handling By reducing the original call to all distinct 'status' we also removed the status_alias combinations. This patch adds an additional distinct query to fetch all 'status_alias' and add them to the returned data structure. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153968 [details] [review] Bug 34223: Add unit test for existing_statuses This patch adds a unit test for the 'existing_statuses' method. Created attachment 153973 [details] [review] Bug 34223: Add unit test for existing_statuses This patch adds a unit test for the 'existing_statuses' method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153978 [details] [review] Bug 34223: (follow-up) Mock backend in tests Add a new request with a backend specific status Created attachment 153979 [details] [review] Bug 34223: Work around ONLY_FULL_GROUP_BY in Illbackend This patch re-works the query in the existing_statuses method to remove the FIXME and improve performance. We pass an SQL literal into the query to make it explicit which illrequest_id we're looking for (which dampens the SQL warning) even though we really don't mind which request is returned here. Test plan: The following command will (hopefully) reset your ILL data and create 10k fake ILL requests (run this in DEV KTD only). 1) On an empty k-t-d, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-data.sh) 2) Pet a cat 3) Visit /cgi-bin/koha/ill/ill-requests.pl and select a backend on the left side filters 4) Notice how the status filter takes a while (3-5 secs) to load 5) Apply patch and koha-plack --restart kohadev 6) Repeat 3, notice how the status filter now loads fast Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 153980 [details] [review] Bug 34223: (follow-up) Restore status_alias handling By reducing the original call to all distinct 'status' we also removed the status_alias combinations. This patch adds an additional distinct query to fetch all 'status_alias' and add them to the returned data structure. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 153981 [details] [review] Bug 34223: Add unit test for existing_statuses This patch adds a unit test for the 'existing_statuses' method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 153989 [details] [review] Bug 34223: (follow-up) Mock backend in tests Add a new request with a backend specific status Created attachment 153990 [details] [review] Bug 34223: (follow-up) Mock backend in tests Add a new request with a backend specific status Created attachment 154026 [details] [review] Bug 34223: Work around ONLY_FULL_GROUP_BY in Illbackend This patch re-works the query in the existing_statuses method to remove the FIXME and improve performance. We pass an SQL literal into the query to make it explicit which illrequest_id we're looking for (which dampens the SQL warning) even though we really don't mind which request is returned here. Test plan: The following command will (hopefully) reset your ILL data and create 10k fake ILL requests (run this in DEV KTD only). 1) On an empty k-t-d, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-data.sh) 2) Pet a cat 3) Visit /cgi-bin/koha/ill/ill-requests.pl and select a backend on the left side filters 4) Notice how the status filter takes a while (3-5 secs) to load 5) Apply patch and koha-plack --restart kohadev 6) Repeat 3, notice how the status filter now loads fast Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 154027 [details] [review] Bug 34223: (follow-up) Restore status_alias handling By reducing the original call to all distinct 'status' we also removed the status_alias combinations. This patch adds an additional distinct query to fetch all 'status_alias' and add them to the returned data structure. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 154028 [details] [review] Bug 34223: Add unit test for existing_statuses This patch adds a unit test for the 'existing_statuses' method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 154029 [details] [review] Bug 34223: (follow-up) Mock backend in tests Add a new request with a backend specific status Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> It appears something went wrong with patch "Add unit test for existing_statuses" upon Laura's sign-off, it currently includes all other patches AND Illbackend.t (which is the only file it should contain). Created attachment 154030 [details] [review] Bug 34223: Work around ONLY_FULL_GROUP_BY in Illbackend This patch re-works the query in the existing_statuses method to remove the FIXME and improve performance. We pass an SQL literal into the query to make it explicit which illrequest_id we're looking for (which dampens the SQL warning) even though we really don't mind which request is returned here. Test plan: The following command will (hopefully) reset your ILL data and create 10k fake ILL requests (run this in DEV KTD only). 1) On an empty k-t-d, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-data.sh) 2) Pet a cat 3) Visit /cgi-bin/koha/ill/ill-requests.pl and select a backend on the left side filters 4) Notice how the status filter takes a while (3-5 secs) to load 5) Apply patch and koha-plack --restart kohadev 6) Repeat 3, notice how the status filter now loads fast Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 154031 [details] [review] Bug 34223: (follow-up) Restore status_alias handling By reducing the original call to all distinct 'status' we also removed the status_alias combinations. This patch adds an additional distinct query to fetch all 'status_alias' and add them to the returned data structure. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 154032 [details] [review] Bug 34223: Add unit test for existing_statuses This patch adds a unit test for the 'existing_statuses' method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 154033 [details] [review] Bug 34223: (follow-up) Mock backend in tests Add a new request with a backend specific status Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 154068 [details] [review] Bug 34223: Work around ONLY_FULL_GROUP_BY in Illbackend This patch re-works the query in the existing_statuses method to remove the FIXME and improve performance. We pass an SQL literal into the query to make it explicit which illrequest_id we're looking for (which dampens the SQL warning) even though we really don't mind which request is returned here. Test plan: The following command will (hopefully) reset your ILL data and create 10k fake ILL requests (run this in DEV KTD only). 1) On an empty k-t-d, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-data.sh) 2) Pet a cat 3) Visit /cgi-bin/koha/ill/ill-requests.pl and select a backend on the left side filters 4) Notice how the status filter takes a while (3-5 secs) to load 5) Apply patch and koha-plack --restart kohadev 6) Repeat 3, notice how the status filter now loads fast Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Created attachment 154069 [details] [review] Bug 34223: (follow-up) Restore status_alias handling By reducing the original call to all distinct 'status' we also removed the status_alias combinations. This patch adds an additional distinct query to fetch all 'status_alias' and add them to the returned data structure. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 154070 [details] [review] Bug 34223: Add unit test for existing_statuses This patch adds a unit test for the 'existing_statuses' method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 154071 [details] [review] Bug 34223: (follow-up) Mock backend in tests Add a new request with a backend specific status Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I almost got stuck on step 2... no cat! Pushed to master for 23.11. Nice work everyone, thanks! Created attachment 155280 [details] [review] Bug 34223: (QA follow-up) Fix file permissions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to 23.05.x for 23.05.04 Created attachment 155493 [details] [review] Bug 34223: (follow-up) Ensure consistent ordering of statuses This patch adds an order_by clause to ensure consistent ordering of the returned status and status_alias statuses from the database between MySQL 8 and other DB engines. This fixes the failing tests introducd in bug 34223. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Follow-up pushed to master. Please backport. Follow-up pushed to 23.05.x for 23.05.04 Nice work everyone! Pushed to oldstable for 22.11.x |