Navigation Menu

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

Piwik compatible with MySQL mode: ONLY_FULL_GROUP_BY #5124

Open
anonymous-matomo-user opened this issue May 9, 2014 · 26 comments
Open

Piwik compatible with MySQL mode: ONLY_FULL_GROUP_BY #5124

anonymous-matomo-user opened this issue May 9, 2014 · 26 comments
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.

Comments

@anonymous-matomo-user
Copy link

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

@mattab
Copy link
Member

mattab commented May 13, 2014

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:

  • Set ONLY_FULL_GROUP_BY during the core + integration + plugin tests, so that our Continuous integration server shows us problems as early as possible

@anonymous-matomo-user anonymous-matomo-user added this to the 2.5.0 - Piwik 2.5.0 milestone Jul 8, 2014
@lourdas
Copy link
Contributor

lourdas commented Aug 17, 2014

I'm attaching a screenshot of my piwik database where ONLY_FULL_GROUP_BY is enabled. This needs work and it might not be a simple case (from personal experience, we had to rewrite some sql queries for ONLY_FULL_GROUP_BY to work for a project of ours).
piwik_only_full_group_by_issues

@mattab
Copy link
Member

mattab commented Aug 20, 2014

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

@mattab mattab added Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. and removed Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. labels Sep 18, 2014
@mattab
Copy link
Member

mattab commented Sep 18, 2014

If you experience this issue with Piwik please comment in this thread! If more users comment we will schedule it.

@gcmartijn
Copy link

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.

@mattab
Copy link
Member

mattab commented Sep 19, 2014

That will help us for tickets like #2593 and #3914 Sqlite support and #500 Postgresql support

@mattab mattab added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. and removed Bug For errors / faults / flaws / inconsistencies etc. labels Sep 19, 2014
@mattab mattab added the c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. label Oct 12, 2014
@mattab
Copy link
Member

mattab commented Apr 7, 2015

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.

@mattab mattab changed the title MYSQL ONLY_FULL_GROUP_BY valid or catch Piwik compatible with MySQL mode: ONLY_FULL_GROUP_BY Apr 9, 2015
@mattab mattab modified the milestones: Mid term, Short term Apr 9, 2015
@rvullriede
Copy link

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/

@sander
Copy link

sander commented Aug 12, 2015

I’m experiencing this issue with MySQL 5.7. Most Piwik displays show:

Mysqli prepare error: Expression #1 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'withCounter.idaction' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by

Is there a workaround?

@lourdas
Copy link
Contributor

lourdas commented Aug 12, 2015

rant mode on
Welcome to the wonderful world of MySQL and non SQL strictness. Enjoy.
rant mode off

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.

@mattab
Copy link
Member

mattab commented Aug 13, 2015

@tsteur try what? do you mean adding the ONLY_FULL_GROUP_BY on CI so our tests run with it? or "disabling" this ONLY_FULL_GROUP_BY mode?

@tsteur
Copy link
Member

tsteur commented Aug 14, 2015

I meant "disabling" this ONLY_FULL_GROUP_BY. Not sure if it's a good idea though and whether we should actually do it. Maybe it would be a good temporary solution until we actually get to work on fixing it properly but we all know temporary solutions usually stay forever hehe

@lourdas
Copy link
Contributor

lourdas commented Aug 14, 2015

@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.

@mattab mattab modified the milestones: 2.15.0, Mid term Aug 14, 2015
@mattab
Copy link
Member

mattab commented Aug 14, 2015

Adding to 2.15.0 as this will be a quick fix, and will help prevent some issues.

@lourdas
Copy link
Contributor

lourdas commented Aug 14, 2015

Please consider against using a empty sql_mode directive. The one I use at my production server is

sql-mode = STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_AUTO_VALUE_ON_ZERO,NO_ENGINE_SUBSTITUTION,NO_ZERO_DATE,NO_ZERO_IN_DATE

Running the latest version of Piwik (and in total for almost 2 years now) with no issues at all.

@tsteur
Copy link
Member

tsteur commented Aug 17, 2015

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)

@tsteur
Copy link
Member

tsteur commented Aug 18, 2015

Is it possible that someone tries the manual setting of SQL mode?

One needs to patch core/Db.php see https://github.com/piwik/piwik/compare/5124?expand=1#diff-4410ab928da865a7fe6bbb3dea2e48c2R34 and either core/Db/Adapter/Pdo/Mysql.php if PDO is used https://github.com/piwik/piwik/compare/5124?expand=1#diff-3f803240d77a636be35f90d3813d9cf7L12 or core/Db/Adapter/Mysqli.php if Mysqli is used https://github.com/piwik/piwik/compare/5124?expand=1#diff-16483da56b87bf02bb53a48a732958b2L12

I did a quick performance test and it takes about 60micro seconds to set it, both Mysqli and Pdo

@tsteur
Copy link
Member

tsteur commented Aug 18, 2015

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 ONLY_FULL_GROUP_BY afterwards but would be still good to have at some point.

Actually the PR will be the issue, we just need to move this one to mid term afterwards

@lourdas
Copy link
Contributor

lourdas commented Aug 18, 2015

@tsteur I've applied the patch to my production Piwik installation (2.14.3), added ONLY_FULL_GROUP_BY to my.cnf, restarted MySQL and Piwik runs fine.

@lourdas
Copy link
Contributor

lourdas commented Aug 18, 2015

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)

@tsteur
Copy link
Member

tsteur commented Aug 19, 2015

Thanks for the feedback!

diosmosis added a commit that referenced this issue Aug 21, 2015
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.
@mattab mattab modified the milestones: 2.15.0, Mid term Oct 6, 2015
@mattab
Copy link
Member

mattab commented Oct 6, 2015

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.

@lourdas
Copy link
Contributor

lourdas commented Oct 6, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

No branches or pull requests

7 participants