Skip to content
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

[database] Move join subquery into separate view for ancient MySQL #15235

Merged
merged 2 commits into from Jan 12, 2019

Conversation

@pkerling
Copy link
Member

commented Jan 12, 2019

So it seems MySQL < 5.7 cannot perform subqueries in SQL FROM. As a
workaround, put the subquery into a separate view.

See #15200

@pkerling pkerling requested review from phate89 and fritsch Jan 12, 2019
@pkerling pkerling self-assigned this Jan 12, 2019
@pkerling pkerling added this to the Leia 18.0-rc5 milestone Jan 12, 2019
So it seems MySQL < 5.7 cannot perform subqueries in SQL FROM. As a
workaround, put the subquery into a separate view.
@pkerling pkerling force-pushed the pkerling:mysql55-fix branch from 6f63631 to feaeaec Jan 12, 2019
@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

jenkins build this please

Copy link
Member

left a comment

Looks good - let's see if it works.

@pkerling pkerling referenced this pull request Jan 12, 2019
3 of 13 tasks complete
@Tolriq

This comment has been minimized.

Copy link

commented on feaeaec Jan 12, 2019

@pkerling please use explain query plan :) Views do not have index, this will kills performance in most Mysql versions and sqlite. Views in views is an aberration in sqlite and that's what 99% of users use.
This is already a fix of a fix from a bad fix for another problem. Maybe trying to had tons of quick hacks are not the solution and a proper fix for the first issue would be better.

This comment has been minimized.

Copy link
Owner Author

replied Jan 12, 2019

Everyone wants a proper fix, but for 18.0 it's not going to happen. If you have a suggestion to fix the query that does not involve changing the UI and skins and lots of other places, do tell.

This comment has been minimized.

Copy link
Owner Author

replied Jan 12, 2019

Of course it would be a valid solution to revert everything and say MySQL >= 5.7 is not supported. That way we wouldn't regress at least.

This comment has been minimized.

Copy link

replied Jan 12, 2019

I no more have a running Mysql and seeing the situation and the ban I can't pass time to do the fix, I already did to have a proper CVE fix. It's quite complicated to continue to help a project when you are banned for trying to help ;)

Anyway I'm just warning about speed because this for sure will have impacts, and hard to measure at that point in the release. That show view is used in a tons of places.
So whatever you choose be sure to at least test for speed regression and choose a limit between a fix and x% speed lost. (A few percent may not matters but maybe 30 or 40% on rpi with large database could)

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

Can someone with a large DB of TV series on MySQL do a speed comparison? I can only do SQLite.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

I've tested with a small 58 tv show libary on MySQL 5.5.21 and can confirm that upgrading from 114 to 116 now works, many thanks! According to MySQL Workbench the new 116 tvshows_view seems to perform a little better than it did with 113 and 114 (116: 0.078 sec, 114: 0.234 sec, 113: 0.374 sec, 85 rows total).

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

SQLite time difference between 113 and 116 is below measurement accuracy (185 shows)

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

Actually this whole story shouldn't even have happened - as @MilhouseVH pointed out to me, we theoretically disable ONLY_FULL_GROUP_BY (#8393), so @wildcat2020 should never have experienced the view failing. Curious.

@wildcat2020

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

Actually not if you ever want to install database schema on Oracle RDBMS...
There is no disable full group by there...

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

I can assure you that we don't want to do that - we don't even have someone maintaining MySQL ;-)

And I can't really see Kodi on Oracle becoming "a thing"...

So @wildcat2020 does that mean you never actually encountered the error in Kodi on a MySQL installation?

@wildcat2020

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

No, I did experience the error on MySQL 5.7 (today just upgraded to 8.0)...
So all this work for sure it was not for nothing...
But I really would like to see data schema to be truly generic so that it will be able to run anywhere...and that means no particular SQlite or MySQL stuff enabled...
I can help working on that....just bare with me some time till I understand the whole schema logic....

@wildcat2020

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

I will be having for a long time MySQL as it serves other stuff so if you need me to test stuff, I can surely do...

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

But I really would like to see data schema to be truly generic so that it will be able to run anywhere...and that means no particular SQlite or MySQL stuff enabled...

Yes of course, and also be able to run efficiently on the many lower power processors that Kodi is installed on. Sacrificing speed on a local SQLite db for genericity and multiple RDBMS is not acceptable IMO.

I can help working on that....just bare with me some time till I understand the whole schema logic....

Contributions are very welcome, but do keep in mind that those that came before us were not idiots, many things are like they are for a good reason (even if we no longer know what that reason was)

@phate89

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

About performance the query is slower but it's negligible. It's a query on tv shows so a big database might have 200/300 elements top. episode_view with tvshowcounts is way slower already.
but I didn't know we override the setting so we probably should have looked at that for 18...

@wildcat2020

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

Agreed, always old stuff are there for a reason, and for sure whoever coded that stuff was not an idiot, never meant something like that..
We must understand what that is so that we are able to move things forward and add new functionality (that supports AI for instance and is able to answer to questions like for instance, what's new this week...) without breaking the current one...
Functionality always drives the data design, and if you are aiming for max integration capabilities with various web services, simplicity and generality is a key...
That's all I'm saying...
I hope that makes sense...

@wildcat2020

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

Just an idea for the sub query, instead of select max wouldn't it be faster to just select and add LIMIT 1?

@fritsch

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

From the benchmarks: No real difference. Let's not overoptimize the workaround for a design flaw.

@graysky2

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

So it seems MySQL < 5.7 cannot perform subqueries in SQL FROM.

I get the same on MariaDB 10.1.37 (released only 3 months ago). It could be that the 10.1.x branch is based on MYSQL <5.7, I don't know. I speak-up to let you guys know this fact and to potentially add scope to the bug.

@fritsch

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

@graysky2 yes - same problem. The 10.1.x branch is old as hell and only receives "maintenance" updates ... I ran into the same issue on Ubuntu 18.04 when testing this. Adding a ppa and upgrading to 10.3 solved.

But: with the above change it should work again for the old version.

@graysky2

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

@fritsch - Thanks for the info. I patched rc5 with this PR and can confirm expected behavior without error when updating from RC4 --> this version. Good job, all 👍

@fritsch

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

Build failure not related. Works for testers.

@fritsch fritsch merged commit 0a0147f into xbmc:master Jan 12, 2019
1 check failed
1 check failed
default Sorry, building this PR failed. Please check the logs for the errors.
Details
@pkerling pkerling deleted the pkerling:mysql55-fix branch Jan 13, 2019
MartijnKaijser added a commit to MartijnKaijser/xbmc that referenced this pull request Jan 22, 2019
[database] Move join subquery into separate view for ancient MySQL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.