Summary: | Count/pagination broken for reports with duplicated column names | ||
---|---|---|---|
Product: | Koha | Reporter: | Nick Clemens (kidclamp) <nick> |
Component: | Reports | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | major | ||
Priority: | P5 - low | CC: | jonathan.druart, kelly, martin.renvoize, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
19.11.00
|
|
Circulation function: | |||
Bug Depends on: | 23624 | ||
Bug Blocks: | |||
Attachments: |
Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table
Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table Bug 23982: Fix the failing test |
Description
Nick Clemens (kidclamp)
2019-11-06 20:01:52 UTC
It seems that we should revert commit bca4453c50d8b5b1d3c7029ded2b522a9a2bf868 Bug 23624: (QA follow-up) Optimize even more From https://bugs.mysql.com/bug.php?id=6709: """ This is in agreement with the SQL Standard, which says: [...] 11) No <column name> shall be specified more than once in a <derived column> list. """ This is horrible.. I really don't like the idea of reverting that followup as it fixes an important performance hole. Feels like MySQL is getting it wrong to me.. grr Contemplating if we have any other options. I think if we can make a sensible error back to the user about deduplicating the names we would be okay. Ideally we would also make basic pagination work, just assumign 10 pages, and again having a friendly error if they page out of bounds Created attachment 95479 [details] [review] Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table Caused by commit bca4453c50d8b5b1d3c7029ded2b522a9a2bf868 Bug 23624: (QA follow-up) Optimize even more A report like: SELECT * FROM issues JOIN borrowers USING (borrowernumber) will have two borrowernumber columns - SQL will give us there rsults, but if we try to wrap them in a SELECT COUNT(*) FROM (report) it throws a duplicated column error. This patch suggests to execute the query the old way if the derived table optimization failed. This impacts stable releases and will cause reports to fail. I think it's a critical one. Created attachment 95545 [details] [review] Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table Caused by commit bca4453c50d8b5b1d3c7029ded2b522a9a2bf868 Bug 23624: (QA follow-up) Optimize even more A report like: SELECT * FROM issues JOIN borrowers USING (borrowernumber) will have two borrowernumber columns - SQL will give us there rsults, but if we try to wrap them in a SELECT COUNT(*) FROM (report) it throws a duplicated column error. This patch suggests to execute the query the old way if the derived table optimization failed. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks for jumping on this one Jonathan, looks reasonable and in a quick test it works for me.. Signing off. Created attachment 95548 [details] [review] Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table Caused by commit bca4453c50d8b5b1d3c7029ded2b522a9a2bf868 Bug 23624: (QA follow-up) Optimize even more A report like: SELECT * FROM issues JOIN borrowers USING (borrowernumber) will have two borrowernumber columns - SQL will give us there rsults, but if we try to wrap them in a SELECT COUNT(*) FROM (report) it throws a duplicated column error. This patch suggests to execute the query the old way if the derived table optimization failed. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Nice work! Pushed to master for 19.11.00 Created attachment 95593 [details] [review] Bug 23982: Fix the failing test # Failed test 'nb_rows returns 0 on bad queries' # at t/db_dependent/Reports/Guided.t line 441. # got: undef # expected: '0' Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Test Fix pushed to master |