-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Piwik compatible with MySQL mode: ONLY_FULL_GROUP_BY #5124
Comments
is something broken when ONLY_FULL_GROUP_BY is enabled? What is the actual error that you get please? I think the best course of action would be:
|
Tests are now running with ONLY_FULL_GROUP_BY but unfortunately no tests are failing. Maybe the set global sqlmode is not working as expected? UPDATE: Actually tests are failing! see https://travis-ci.org/piwik/piwik/jobs/33112799 |
If you experience this issue with Piwik please comment in this thread! If more users comment we will schedule it. |
Mysql has support for non-standard grouping. The fix is to group by all non-aggregate selected columns: GROUP BY [all columns] In mysql only (all other databases will raise an exception), grouping by fewer than all non-aggregated columns results in a random row for each unique combination of those columns that are grouped by. That's why to set ONLY_FULL_GROUP_BY to make valid database query's so it will work everywhere. And more important, don't get random results. |
The goal of this issue is to enable ONLY_FULL_GROUP_BY in our CI setup (revert 49c4863) and then implement the change in all SQL queries that make the build fail. |
With MySQL 5.7 ONLY_FULL_GROUP_BY will become the default and a lot more user will experience this: http://mysqlserverteam.com/mysql-5-7-only_full_group_by-improved-recognizing-functional-dependencies-enabled-by-default/ |
I’m experiencing this issue with MySQL 5.7. Most Piwik displays show:
Is there a workaround? |
rant mode on If by any chance Piwik is supposed to use in the future other database backends, like PostgreSQL or Oracle or MS SQL Server maybe, the Piwik developers must correct all these problematic SQL queries. IMHO, the fact that MySQL 5.7 will by default enable ONLY_FULL_GROUP_BY tells us nothing. Any serious database application developer should set their own sane (thus SQL strict) options by default. |
I meant |
@tsteur is right. While my MariaDB 10 configuration should include ONLY_FULL_GROUP_BY in sql_mode, it doesn't because Piwik throws a bunch of errors. Now it would be a good time for a patch to remove ONLY_FULL_GROUP_BY per session, so that any database admin would not remove ONLY_FULL_GROUP_BY from the global sql_mode variable and this would temporarily fix the upcoming change in defaults for MySQL 5.7. Then, the next logical step is to fix all queries with GROUP BY that are problematic. On a side note, ONLY_FULL_GROUP_BY is just one MySQL quirk. What about dates like 0000-00-00 which again depending on the server sql_mode are accepted or not...? I just hope Piwik doesn't use dates like that for a null date or something of a special semantics. |
Adding to 2.15.0 as this will be a quick fix, and will help prevent some issues. |
Please consider against using a empty
Running the latest version of Piwik (and in total for almost 2 years now) with no issues at all. |
Sounds good, might take a while to check for different modes but I reckon it will be helpful for now when knowing all Piwik installations will have the same SQL mode. We should also check re performance how long it takes to set this (should be very fast) |
Is it possible that someone tries the manual setting of SQL mode? One needs to patch I did a quick performance test and it takes about 60micro seconds to set it, both Mysqli and Pdo |
FYI: Once we created this issue, we should move this issue back to mid term or so and create a new issue just for the workaround as we won't be compatible with Actually the PR will be the issue, we just need to move this one to mid term afterwards |
@tsteur I've applied the patch to my production Piwik installation (2.14.3), added |
And link to documentation about MySQL SQL modes: https://dev.mysql.com/doc/refman/5.5/en/sql-mode.html#sql-mode-full (version 5.5) |
Thanks for the feedback! |
Refs #5124, for 2.15 LTS, disable ONLY_FULL_GROUP_BY Mysql mode when creating connection by using explicitly specified SQL mode. May be removed in 3.0.
Therefore could this issue be closed do you think? or do we need to actually add support for ONLY_FULL_GROUP_BY? So far i don't see a reason to work on this, since it should be enough to disable this mode in the current session. |
I consider disabling ONLY_FULL_GROUP_BY a workaround, a hack and not a real solution. As a temporary means to fix this ok, but in the long term, the developers should rewrite SQL queries to be SQL compliant. At least this would help in porting Piwik to use other DBMS, like maybe Postgresql or Oracle or whatever. It's a pity that such a wonderful piece of software is closely tight with only MySQL. |
What is the 'problem', well its mysql.
In short its about this
Our and other mysql users use the best (valid) sql modes to get the best query's etc.
One sql mode is ONLY_FULL_GROUP_BY
You need to add all columns you select/use in the GROUP BY
SELECT name, address, MAhot smileyage) FROM t GROUP BY name;
ERROR 1055 (42000): 't.address' isn't in GROUP BY
You do this to tackle some mysql problems.
http://dev.mysql.com/doc/refman/5.0/en/sql-mode.html#sqlmode_only_full_group_by
But piwik don't use this way so upgrading/installing failes.
To fix this we need manual add a sql command in
https://github.com/piwik/piwik/blob/master/core/Db/Adapter/Pdo/Mysql.php
public function getConnection()
{
if ($this->_connection) {
return $this->_connection;
}
$this->_connect();
$this->_connection->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);
SQL COMMAND -> "SET sql_mode='';"
return $this->_connection;
}
Maybe its possible to make this an option or something, the other option is to add all columns to your group by query's confused smiley
Forum: http://forum.piwik.org/read.php?3,115060
The text was updated successfully, but these errors were encountered: