Bug 28977 - Most-circulated items (cat_issues_top.pl) is failing with SQL Mode ONLY_FULL_GROUP_BY
Summary: Most-circulated items (cat_issues_top.pl) is failing with SQL Mode ONLY_FULL_...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Michael Hafen
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-08 20:54 UTC by Michael Hafen
Modified: 2022-02-24 23:37 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes an error that causes the most circulated items report to fail when run on a database with SQL mode ONLY_FULL_GROUP_BY and in strict SQL mode.
Version(s) released in:
22.05.00,21.11.03,21.05.11


Attachments
Proposed patch (2.84 KB, patch)
2021-09-08 21:03 UTC, Michael Hafen
Details | Diff | Splinter Review
Proposed patch (3.28 KB, patch)
2021-09-21 21:40 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 28977 - Add GROUP_CONCAT to some of the selected columns (3.28 KB, patch)
2021-09-23 16:16 UTC, David Nind
Details | Diff | Splinter Review
Bug 28977: Add GROUP_CONCAT to some of the selected columns (3.34 KB, patch)
2021-12-17 16:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28977: Add GROUP_CONCAT to some of the selected columns (3.08 KB, patch)
2022-01-14 18:37 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 28977: Add GROUP_CONCAT to some of the selected columns (3.09 KB, patch)
2022-01-30 17:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 28977: Fix cat_issues_top.pl report with strict SQL modes (3.20 KB, patch)
2022-02-02 10:47 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Hafen 2021-09-08 20:54:44 UTC
The Most circulated items report gives an error when run on a database that has ONLY_FULL_GROUP_BY in it's SQL_MODE.
Patch to follow shortly.
Comment 1 Michael Hafen 2021-09-08 21:03:06 UTC
Created attachment 124664 [details] [review]
Proposed patch

Test plan:

Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF )
Go to Reports -> Most circulated items
submit the form.
Comment 2 David Nind 2021-09-11 22:36:34 UTC
I had a go at testing with koha-testing-docker (KTD), but was unable to get this to work.

- KTD = started with ku
- Strict mode already set in /etc/koha/sites/kohadev/koha-conf.xml

Error before patch applied, no change after (with a flush_memcached and restart_all):

CGI::Compile::ROOT::kohadevbox_koha_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: 'koha_kohadev.biblio.title' isn't in GROUP BY at /kohadevbox/koha/reports/cat_issues_top.pl line 64
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

I think I will leave the testing to someone who understands how this all works a bit better than I do!
Comment 3 Michael Hafen 2021-09-13 17:46:43 UTC
(In reply to David Nind from comment #2)
> I had a go at testing with koha-testing-docker (KTD), but was unable to get
> this to work.
> 
> - KTD = started with ku
> - Strict mode already set in /etc/koha/sites/kohadev/koha-conf.xml
> 
> Error before patch applied, no change after (with a flush_memcached and
> restart_all):
> 
> CGI::Compile::ROOT::kohadevbox_koha_reports_cat_issues_top_2epl::calculate():
> DBI Exception: DBD::mysql::st execute failed: 'koha_kohadev.biblio.title'
> isn't in GROUP BY at /kohadevbox/koha/reports/cat_issues_top.pl line 64
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77
> 
> I think I will leave the testing to someone who understands how this all
> works a bit better than I do!

What options did you use to generate that error?  I'm not able to reproduce it.

I didn't test this report thoroughly, so I'm sure there is more work to be done here.  Now that I'm playing with it a bit more the "distinct" is causing trouble too with Mysql 8.
Comment 4 David Nind 2021-09-13 19:53:08 UTC
> What options did you use to generate that error?  I'm not able to reproduce
> it.

This was with and without selecting any date ranges, 

With koha-testing-docker (started with ku) it is using MariaDB 10.1 with Debian 9.
Comment 5 Michael Hafen 2021-09-21 21:40:06 UTC
Created attachment 125124 [details] [review]
Proposed patch

Fresh patch with fixes for MariaDB in Debian 9.
Comment 6 David Nind 2021-09-23 16:16:08 UTC
Created attachment 125208 [details] [review]
Bug 28977 - Add GROUP_CONCAT to some of the selected columns

To get the 'Most circulated items' report to run in ONLY_FULL_GROUP_BY
Sql mode.

Test plan:

Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF )
Go to Reports -> Most circulated items
submit the form.

Without the patch you get an error like:
CGI::Compile::ROOT::usr_local_koha_master_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: Expression #4 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'koha_main_v4.items.itemcallnumber' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by [for Statement "SELECT DISTINCT biblio.title, COUNT(biblio.biblionumber) AS `RANK`, biblio.biblionumber AS ID, itemcallnumber as CALLNUM, ccode as CCODE, location as LOC FROM `old_issues`
                  LEFT JOIN items USING(itemnumber)
                  LEFT JOIN biblio USING(biblionumber)
                  LEFT JOIN biblioitems USING(biblionumber)
                  LEFT JOIN borrowers USING(borrowernumber)
                  WHERE 1 AND old_issues.issuedate > '2020-08-03' AND old_issues.issuedate < '2021-05-17' AND old_issues.returndate > '2020-08-27' AND old_issues.returndate < '2021-06-01' AND old_issues.branchcode like '403' AND items.itype like 'F' AND ccode like 'FIC' AND borrowers.categorycode like 'ST' group by biblio.biblionumber order by `RANK` DESC"] at /usr/local/koha_master/reports/cat_issues_top.pl line 67
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

With the patch you will see the report results view.

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2021-09-23 16:19:16 UTC
Testing notes (koha-testing-docker):
- tested on environments available by starting KTD with ku, ku-mdb, ku-md9, ku-md10, ky-my8
- check out and check in an item (so that there was some data for the report)
Comment 8 Tomás Cohen Arazi 2021-12-17 16:01:34 UTC
Created attachment 128684 [details] [review]
Bug 28977: Add GROUP_CONCAT to some of the selected columns

To get the 'Most circulated items' report to run in ONLY_FULL_GROUP_BY
Sql mode.

Test plan:

Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF )
Go to Reports -> Most circulated items
submit the form.

Without the patch you get an error like:
CGI::Compile::ROOT::usr_local_koha_master_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: Expression #4 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'koha_main_v4.items.itemcallnumber' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by [for Statement "SELECT DISTINCT biblio.title, COUNT(biblio.biblionumber) AS `RANK`, biblio.biblionumber AS ID, itemcallnumber as CALLNUM, ccode as CCODE, location as LOC FROM `old_issues`
                  LEFT JOIN items USING(itemnumber)
                  LEFT JOIN biblio USING(biblionumber)
                  LEFT JOIN biblioitems USING(biblionumber)
                  LEFT JOIN borrowers USING(borrowernumber)
                  WHERE 1 AND old_issues.issuedate > '2020-08-03' AND old_issues.issuedate < '2021-05-17' AND old_issues.returndate > '2020-08-27' AND old_issues.returndate < '2021-06-01' AND old_issues.branchcode like '403' AND items.itype like 'F' AND ccode like 'FIC' AND borrowers.categorycode like 'ST' group by biblio.biblionumber order by `RANK` DESC"] at /usr/local/koha_master/reports/cat_issues_top.pl line 67
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

With the patch you will see the report results view.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Fridolin Somers 2022-01-10 22:47:53 UTC
Do we still need "DISTINCT" on biblio.title ?
Comment 10 Michael Hafen 2022-01-11 17:10:12 UTC
(In reply to Fridolin Somers from comment #9)
> Do we still need "DISTINCT" on biblio.title ?

Maybe so; depends on how likely it is that separate biblios will have the same title, and whether the library would want those separate records grouped together.
In my case, we'd probably want the records grouped together.  I tried the query in MySQL 8.0 with and without the 'DISTINCT', and there was 1 row difference.  Though my case is probably not typical, being a school district with a bunch of schools that migrated from separate databases into one instance of Koha.
Comment 11 Fridolin Somers 2022-01-11 21:42:21 UTC
(In reply to Michael Hafen from comment #10)
> (In reply to Fridolin Somers from comment #9)
> > Do we still need "DISTINCT" on biblio.title ?
> 
> Maybe so; depends on how likely it is that separate biblios will have the
> same title, and whether the library would want those separate records
> grouped together.
> In my case, we'd probably want the records grouped together.  I tried the
> query in MySQL 8.0 with and without the 'DISTINCT', and there was 1 row
> difference.  Though my case is probably not typical, being a school district
> with a bunch of schools that migrated from separate databases into one
> instance of Koha.

Is there then a need to group by biblionumber ?
Comment 12 Fridolin Somers 2022-01-11 21:43:04 UTC
Ah that is strange.
Looks like callnumber is not displayed in result table.
This has been added by Bug 10154 as a filter, but looks there there is no need to add in the SELECT.
Can someone confirm ?
Comment 13 Michael Hafen 2022-01-12 17:46:20 UTC
(In reply to Fridolin Somers from comment #12)
> Ah that is strange.
> Looks like callnumber is not displayed in result table.
> This has been added by Bug 10154 as a filter, but looks there there is no
> need to add in the SELECT.
> Can someone confirm ?

I would guess that that is intended.  The call number shows in the filters, and probably isn't necessary in the results table.

If someone wants it there though, it can be added.  Here or in an enhancement bug.
Comment 14 Fridolin Somers 2022-01-13 00:10:33 UTC
(In reply to Michael Hafen from comment #13)
> (In reply to Fridolin Somers from comment #12)
> > Ah that is strange.
> > Looks like callnumber is not displayed in result table.
> > This has been added by Bug 10154 as a filter, but looks there there is no
> > need to add in the SELECT.
> > Can someone confirm ?
> 
> I would guess that that is intended.  The call number shows in the filters,
> and probably isn't necessary in the results table.
> 
> If someone wants it there though, it can be added.  Here or in an
> enhancement bug.

I propose removing from select here, instead of group_concat
Comment 15 Michael Hafen 2022-01-14 18:33:20 UTC
Looking at things again, only the title, rank, id and $colfield columns reach the template.  All the other columns in the SELECT clause (all the GROUP_CONCATs except $colfield) could probably be dropped.  Also, the $colfield column should probably be pulled out of the GROUP_CONCAT since it's also added to the GROUP BY clause.
Also, grouping by biblionumber is required, because the report contains a link to the biblio record.
Coming back to the question about DISTINCT.  I had to look up how it works; it seems I didn't understand it completely.  It works like the GROUP BY clause for all the columns in the SELECT clause.  Given that it should probably be dropped for the sake of performance.
I tried running the query again, with and without the DISTINCT, and there was no difference in the number or rows returned.  I don't know what I was doing before, but I think the previous runs were flawed; this result makes more sense.

This amounts to a big change in the patch, and the status should probably be moved back to Needs Signoff if the patch is updated.  Which I think it should be at this point; I really didn't understand this report completely when I started this.  So I'm going to go and work up another patch now.
Comment 16 Michael Hafen 2022-01-14 18:37:26 UTC
Created attachment 129510 [details] [review]
Bug 28977: Add GROUP_CONCAT to some of the selected columns
Comment 17 Jonathan Druart 2022-01-27 15:34:59 UTC
Asking for a signoff.

Could you adjust the patch's title?
Comment 18 David Nind 2022-01-30 17:51:34 UTC
Created attachment 129982 [details] [review]
Bug 28977: Add GROUP_CONCAT to some of the selected columns

To get the 'Most circulated items' report to run in ONLY_FULL_GROUP_BY
Sql mode.

Test plan:

Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF )
Go to Reports -> Most circulated items
submit the form.

Without the patch you get an error like:
CGI::Compile::ROOT::usr_local_koha_master_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: Expression #4 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'koha_main_v4.items.itemcallnumber' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by [for Statement "SELECT DISTINCT biblio.title, COUNT(biblio.biblionumber) AS `RANK`, biblio.biblionumber AS ID, itemcallnumber as CALLNUM, ccode as CCODE, location as LOC FROM `old_issues`
                  LEFT JOIN items USING(itemnumber)
                  LEFT JOIN biblio USING(biblionumber)
                  LEFT JOIN biblioitems USING(biblionumber)
                  LEFT JOIN borrowers USING(borrowernumber)
                  WHERE 1 AND old_issues.issuedate > '2020-08-03' AND old_issues.issuedate < '2021-05-17' AND old_issues.returndate > '2020-08-27' AND old_issues.returndate < '2021-06-01' AND old_issues.branchcode like '403' AND items.itype like 'F' AND ccode like 'FIC' AND borrowers.categorycode like 'ST' group by biblio.biblionumber order by `RANK` DESC"] at /usr/local/koha_master/reports/cat_issues_top.pl line 67
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

With the patch you will see the report results view.

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 Jonathan Druart 2022-02-02 10:47:46 UTC
Created attachment 130081 [details] [review]
Bug 28977: Fix cat_issues_top.pl report with strict SQL modes

To get the 'Most circulated items' report to run in ONLY_FULL_GROUP_BY
Sql mode.

Test plan:

Turn on strict_sql_modes ( make sure <strict_sql_modes> is 1 in KOHA_CONF )
Go to Reports -> Most circulated items
submit the form.

Without the patch you get an error like:
CGI::Compile::ROOT::usr_local_koha_master_reports_cat_issues_top_2epl::calculate(): DBI Exception: DBD::mysql::st execute failed: Expression #4 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'koha_main_v4.items.itemcallnumber' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by [for Statement "SELECT DISTINCT biblio.title, COUNT(biblio.biblionumber) AS `RANK`, biblio.biblionumber AS ID, itemcallnumber as CALLNUM, ccode as CCODE, location as LOC FROM `old_issues`
                  LEFT JOIN items USING(itemnumber)
                  LEFT JOIN biblio USING(biblionumber)
                  LEFT JOIN biblioitems USING(biblionumber)
                  LEFT JOIN borrowers USING(borrowernumber)
                  WHERE 1 AND old_issues.issuedate > '2020-08-03' AND old_issues.issuedate < '2021-05-17' AND old_issues.returndate > '2020-08-27' AND old_issues.returndate < '2021-06-01' AND old_issues.branchcode like '403' AND items.itype like 'F' AND ccode like 'FIC' AND borrowers.categorycode like 'ST' group by biblio.biblionumber order by `RANK` DESC"] at /usr/local/koha_master/reports/cat_issues_top.pl line 67
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

With the patch you will see the report results view.

Signed-off-by: David Nind <david@davidnind.com>

JD Amended patch - adjust commit's title

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Fridolin Somers 2022-02-10 08:03:02 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 21 Kyle M Hall 2022-02-11 11:37:30 UTC
Pushed to 21.11.x for 21.11.03
Comment 22 Andrew Fuerste-Henry 2022-02-21 16:32:42 UTC
Pushed to 21.05.x for 21.05.11
Comment 23 Victor Grousset/tuxayo 2022-02-24 23:37:54 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.