-
Notifications
You must be signed in to change notification settings - Fork 120
Removed slow subquery from albums view #300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I already tried to remove this ugly request, but you can't doing it this way. |
|
In fact the whole database schema is very ugly... We took a very bad decision at the early beginning of this project. |
|
Considering the improvement that this PR provides, I think it would be interesting to consider dropping PostgreSQL support (until it gets fixed). Now that I run Sonerezh with this commit, I just don't want to come back. |
|
I'll look into getting a postgres solution to work. In worst case the subquery could be postgres specific code. |
|
The issue seems to be that the code is not SQL-92 compatible @lGuillaume124. PostgreSQL is strict on this as opposed to earlier versions of MySQL. Either I could do a PR from my whole development-new branch to merge all newer changes (including this). See full list of changes from the original sonerezh development branch here: Otherwise I could either update this PR with the SQL-92 compliance changes or create a new PR. The first suggestion would be the easiest of course. |
|
PostgreSQL just verified to work fine with my development-new branch. |
|
Great! If it doesn't bother you I prefer you to update this PR :) |
|
Rebased and amended. Now works for MySQL, PostgreSQL & Sqlite. (removed the sqlite specific code since the same code works for all three db types) @lGuillaume124 Also, this includes the code in PR #304 (issue #270) since it isn't possible to exclude the group by band (SQL-92 compliance). |
|
bump :). Any reasons left not to merge ? On a large collection (~10Gb of music), the slow query gets the cpu to 100% and crashes the app |
|
Unfortunately it's now been close to 8 months since the last activity here. @JocelynDelalande My own branch/fork has a number of fixes (for this issue among others) & feature additions. Until the sonerezh devs have time available, I'd advise you to run this branch and report any issues there. |
|
Already merged as part of #339 |
Fix for issue #279
Opened new PR to change source branch. Old PR #283