Use ONLY_FULL_GROUP_BY for MySQL

Background: ANSI SQL requires that you GROUP BY every field that is not an aggregate if you have any aggregates. MySQL, as usual, doesn't care and tries to guess what you mean anyway. PostgreSQL does not.

MySQL of course has a flag to change that behavior and require ANSI behavior.

HOWEVER!

MySQL 5.0.0-5.0.3 includes that flag as part of STRICT. From 5.0.4 on, it does not. Drupal 7 supposedly supports 5.0.0 and later, so right now whether or not we require a full GROUP BY in MySQL varies with your MySQL version. Doubleplusungood.

This patch forces MySQL into full group by mode in all versions, which means it behaves in the SQL-standard way, which is also the way PostgreSQL behaves. So, let's do.

I like. Committed to CVS HEAD. Thanks.

Stopped working for me during a clean install with the current Drupal CVS HEAD.

The error message is:

SQLSTATE[42000]: Syntax error or access violation: 1140 Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause 

I'm running OpenSuse 11.1, Mysql version string is:

mysql  Ver 14.12 Distrib 5.0.67, for suse-linux-gnu (i686) using readline 5.2

I've managed to track it to a MySQL bug:
http://bugs.mysql.com/bug.php?id=39656

MySQL version 5.0.67

Confirmed here too. I had a fresh checkout right after that patch, and the error happens on MySQL 5.1.30-community-log. The same checkout is also broken on SQLite, but I'm not sure this is related.

Reverting this specific patch repaired the install for MySQL.

I rolled back this patch so we can continue to investigate and brainstorm without breaking half the world.

Reverting the patch also repaired the SQLite install.

Explanation: As we figured out at DrupalCon Paris, there's a bug (again) in some versions of MySQL that breaks completely with this patch. Of course, that version happens to be the version that ships with several major Linux distros. MySQL's QA Process Fail!

Eventually we'll be able to require a new enough MySQL version that we won't have that bug, I hope, but until then...

Well, I should have done this a year ago, but maybe it can still get in 7.x ?? Feel free to kick it back to 8.x, though.

It safely reverts back to current behavior in the offending MySQL versions while working OK with the fixed versions.. From the bug report in http://bugs.mysql.com/bug.php?id=39656 (and my experience a year ago), it is known to be broken in 5.0.67 and fixed in 5.0.74 (for 5.1 it's broken between 5.1.28 and 5.1.31). Unfortunately, I don't know the exact versions where the bug first appeared.

I had generated the patch from inside includes/database/mysql. I've now set the full path starting from Drupal root.

It passed! It would be nice to know the MySQL version of test client #33 (and even if it indeed running MySQL and not pgSQL).

Is there anyway to force a test on a machine running MySQL 5.0.67?

João

PS: There's a list of "official" MySQL releases at http://downloads.mysql.com/archives.php?p=mysql-5.0&o=other

Apparently the test bots are all running MariaDB to begin with, so none of them (I presume) will have this bug.

Drupal now requires MySQL 5.0.15, too, so the original MySQL inconsistency is no longer an issue. The folks at MySQL actually want us to increase the MySQL version requirement anyway, but we need to investigate what distributions ship what before we can do that.

Also, this patch makes the MySQL support inconsistent. It means it's possible to write a query that sometimes works in MySQL and sometimes does not, and it won't be clear to the module developer why. I think that's worse than writing a query that works in MySQL and not Postgres; the vast majority of our user base is still MySQL, remember.

Also, the PHP version_compare() function would probably be a cleaner implementation anyway.

We really need to gather some statistics on what distributions ship what as far as MySQL goes. That will help us decide how big of an impact patches like this will have.

Indeed, version_compare() improves it a lot :) Didn't know such a thing existed.

I've re-rolled the patch.

Maybe the drupal core update function could be used to provide some statistics about that (database type, version of the database, etc.) back to d.o. That way you could see when the majority of the users stops using 5.0.67.

João

This should now be failing since #878198: Regression: add back a method to the DB driver interface that reports the server version. got committed.

Some further notes about MySQL < 5.1.31 also in #875674: Report DB version in Status report..

Anyway, new patch without my version of version().

I patched Drupal 7.8 the opposite way.
Add automaticly FIRST() aggregate funtions when ORDER BY is not empty and field is not within ORDER BY.
To use ONLY_FULL_GROUP_BY in mysql cause that you doent have a FIRST() aggregate funktion in mysql but what you will then?

One of #21 and #22 needs to be rerolled for core/. :)

In MySQL 5.7, it has been proposed that ONLY_FULL_GROUP_BY may be enabled by default. I have a blog on this here:
http://www.tocker.ca/2014/01/24/proposal-to-enable-sql-mode-only-full-gr...

Woops, component changed incorrectly there. Setting it to 'mysql db driver'.

An update: MySQL 5.7 will have ONLY_FULL_GROUP_BY enabled by default. It has also been improved to be more SQL standards compliant:
http://mysqlserverteam.com/mysql-5-7-only_full_group_by-improved-recogni...

OK, re-rolling #21 to D8, and because of #26, I'm bumping it to major, as we should be prepared for MySQL 5.7 in 8.0.x. Note that this issue is dependant on #2473301: Raise MySQL requirement to 5.5.3, as I'm assuming that will go in as well. It's pointless to raise the minimum MySQL version to the now long-dead 5.1.31, so I've removed that from the patch. I'm also skipping the interdiff, as it's a one liner.

@greenrover33: #22 is so different that I fail to see what it's trying to do. Among others, this issue is about improving MySQL's standards compliance, but the patch you wrote applies only to the pgsql driver? Whoever creates the final commit message, please remove greenrover33 from the credits line.

PS: I worked on this issue in DC Paris 2009 and DC Copenhagen 2010. I can't believe it's still open!

Bot's happy, let's just do it.

Only after #2473301: Raise MySQL requirement to 5.5.3, please. It does depend on MySQL >= 5.1.31, which is currently not the case.

Marking explicitly postponed on that one.

No longer blocked. Before the commit, check my comments regarding @greenrover33 on #27.

Wow, this is an exciting issue history. :) Thanks @Crell for the signoff.

With the new version requirement for MySQL, the MySQL bugs that caused this to be reverted more than 5 years ago (!) presumably are no longer an issue. Ideally this will also reduce the risk of regressing Postgres support with group by while we're at it!

In #6 and #8 it mentions that this change previously broke SQLite somehow. I'm mystified as to how since it only modifies the MySQL driver, not any general database thing, but to be safe can we do an SQLite test run with this patch confirm that this doesn't introduce any new failures there? And might as well put a Postgres run in there too to be safe.

How does one request a patch run against SQLite or Postgres these days? Since neither of those pass all tests right now anyway, I assume "doesn't fail anything new" would be all we need?

Or since both of those are broken right now anyway, and this affects on y MySQL (unless there's some dark magic happening), we can just say screw it and merge and if somehow SQLite or Postgres are affected we figure it out as part of those efforts. I favor that option, frankly.

Or since both of those are broken right now anyway, and this affects on y MySQL (unless there's some dark magic happening), we can just say screw it and merge and if somehow SQLite or Postgres are affected we figure it out as part of those efforts. I favor that option, frankly.

Commit, wait a day, look at
http://d8sqlitebot.erwanderbar.de/ and http://d8pgbot.erwanderbar.de/

As per #33, #35 and #36, it seems that we neeed to see the results of this patch on the non-MySQL bots, but this is a lot easier to do if this is committed.

Also, I don't really believe in black magic, 6 years ago @fgm complained that this broke SQLite support, and that the patch revert in #7 fixed it. The only way to actually see why is to check any possible diffs between the test bots, and then try to understand and fix the problem. Honestly, based on Occam's Razor, I'd lean to the fact that @fgm was actually running MySQL in those setups as well.

Yes sqlite issues seems very unlikely and we can always roll back (again).

Committed/pushed to 8.0.x, thanks!

@jcnventura : actually, in https://www.drupal.org/node/567148#comment-2008856 I mentioned the patch broke both (not just SQLite), and reverting it repaired both. Anyway, things have changed so much in the meantime that any current situation will not be conclusive either way. Let's just see.

we can just say screw it and merge and if somehow SQLite or Postgres are affected we figure it out as part of those efforts. I favor that option, frankly.

No, let's please not do that. Anything that introduces critical regressions on SQLite or PostgreSQL should be reverted, as both have critical issues to pass for 8.0.0. Edit: And both are very close to ready, actually.

Yes I committed this on the assumption that it wouldn't affect sqlite, but agreed if it somehow did, we should just roll back.

@catch: thanks!

@xjm: agree fully.
HOWEVER, if a fix to the MySQL driver breaks the SQLite or PostgreSQL tests, it's my belief that we'll need to exorcise Drupal in addition to reverting this patch.

The results are in, and this is the diff from the previous run:
- SQLite: -504 passes (bad), -2 fails (good), +4 exceptions (bad) http://d8sqlitebot.erwanderbar.de/archive/results_2015_04_28.txt
- PostgreSQL: +8 passes (good), -2 fails (good), 0 exceptions (good) http://d8sqlitebot.erwanderbar.de/archive/results_2015_04_27.txt

The patch may indeed have had an effect on the SQLite tests, most likely it did not, and these 4 exceptions are the result of something else. Some exceptions that happenned on the previous day reoccurred, some disappeared, and a few new ones popped up.

Given the variability of these results, and the fact that we actually had less fails, these exceptions should be looked at in their own right outside of this issue.

SQLite Drupal 8 Testbot Latest Results (http://d8sqlitebot.erwanderbar.de/)
Last Run - Tue Apr 28 7:12:07 CEST 2015
Passes: 87942 Change: (-504)
Fails: 24 Change: (-2)
Exceptions: 9 Change: (+4)

PostgreSQL Drupal 8 Testbot Latest Results (http://d8pgsqlbot.erwanderbar.de/)
Last Run - Tue Apr 28 3:44:28 CEST 2015
Passes: 87300 Change: (+8)
Fails: 23 Change: (-2)
Exceptions: 90 Change: (-0)

The 2 less fails on SQLite were brought by #2475177: SQLite: Fix system\Tests\Database\FetchTest and I can't reproduce any of the new exceptions when running those tests locally so they're probably caused by the DrupalCI test runner and they'll need to be investigated as part of that initiative :/

One concluding note from today's SQLite tests:

SQLite Drupal 8 Testbot Latest Results
Last Run - Wed Apr 29 7:11:57 CEST 2015
Passes: 88800 Change: (+858)
Fails: 24 Change: (+/-0)
Exceptions: 3 Change: (-6)
http://d8sqlitebot.erwanderbar.de/archive/results_2015_04_29.txt

These values are respectively:
- The highest ever number of passes,
- The lowest ever number of fails and exceptions

I can finally say goodbye to this issue that ruined my first attempt at sprinting at a Drupalcon.

Sorry to jump in late and reopen here, but just ran into an issue from someone running Drupal on MySQL 5.7.x.

5.7.x may not be release ready yet however, I am getting a MySQL error that seems to stem from Drupal, I cannot find it in any of the code we wrote or Views (there is not a lot).

PDOException: SQLSTATE[HY000]: General error: 3065 Expression #1 of ORDER BY clause is not in SELECT list, references column 'phcpi_drupal.node.title' which is not in SELECT list; this is incompatible with DISTINCT: SELECT DISTINCT field_data_field_tags0.entity_type AS entity_type, field_data_field_tags0.entity_id AS entity_id, field_data_field_tags0.revision_id AS revision_id, field_data_field_tags0.bundle AS bundle FROM {field_data_field_tags} field_data_field_tags0 INNER JOIN {node} node ON node.nid = field_data_field_tags0.entity_id WHERE (field_data_field_tags0.field_tags_tid = :db_condition_placeholder_0) AND (field_data_field_tags0.deleted = :db_condition_placeholder_1) AND (node.type = :db_condition_placeholder_2) AND (node.status = :db_condition_placeholder_3) AND (node.language = :db_condition_placeholder_4) AND (field_data_field_tags0.entity_type = :db_condition_placeholder_5) ORDER BY node.title ASC; Array ( [:db_condition_placeholder_0] => 21 [:db_condition_placeholder_1] => 0 [:db_condition_placeholder_2] => country [:db_condition_placeholder_3] => 1 [:db_condition_placeholder_4] => en [:db_condition_placeholder_5] => node ) in field_sql_storage_field_storage_query() (line 652 of D:\Projects\R4D-PHCPI-Drupal\docroot\modules\field\modules\field_sql_storage\field_sql_storage.module).

We have 6 developers running MySQL 5.6 per a Vagrant LAMP setup, the only one running a Windows IIS MySQL setup got ahold of 5.7 somehow, and hits this error. No Views are running DISTINCT as far as I can see, and no custom queries are either.

It took us a while to track it down, but it is definitely related to https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sqlmode_only_full_... as the Drupal 8 issue linked to earlier in my post seems to address.

Is there another issue or patch for Drupal 7 that will also address this?

@kevinquillen For context, can you report if 5.6 with ONLY_FULL_GROUP_BY enabled also produces this error?

5.7 actually has a less strict only-full-group-by than 5.6 (respecting functional dependencies), but it is enabled by default.

5.6 does not throw the error.

And does this patch actually make any difference there? Can you try removing the new ONLY_FULL_GROUP_BY flag in core/lib/Drupal/Core/Database/Driver/mysql/Connection.php?

Can you also provide the exact version of MySQL that is being used so others can try to replicate the error? Can you also try with MySQL 5.7.8 (released today)?

From the dev:

c:\Program Files\MySQL\MySQL Server 5.7\bin\mysql.exe Ver 14.14 Distrib 5.7.7-rc, for Win64 (x86_64)

v 5.7.7

@kevinquillen 5.6 does not throw the error, or 5.6 does not throw the error when ONLY_FULL_GROUP_BY is enabled in mysql configuration?

The difference is important, because it would imply that 5.7 is more strict in this case.

I did both ways on my machine (5.6) - no error was thrown.

@kevinquillen: Thanks for sharing that it was 5.7.7. Can the developer also confirm that the problem goes away on 5.7.7 if he removes the changes introduced by this patch, as I described in #54 ("Can you try removing the new ONLY_FULL_GROUP_BY flag in core/lib/Drupal/Core/Database/Driver/mysql/Connection.php")

I'm leaning in to this being more a views bug (if any) than any problem with this patch. This is caused by the fact that you have a DISTINCT on a field that is not being used in the SELECT. In this case, it's ordering by "node.title", but it's not part of the select. This has been seen before in #2395767: Sort error on clicking header label in column, 8 months ago in Drupal 7 which doesn't include this patch.

I will check. The odd thing is I am not using DISTINCT anywhere, I only have 4 basic Views, none of which have sort or distinct on them. The only place I am using a sort are some proertyOrderBy on simple EFQ queries, that sort by title.

edit: yes, he did try adding and removing the mode from sql_mode parameter. No dice.

@kevinquillen, can you open a followup issue for this and reference it here? Thanks!

Created: #2545480: Don't use the ANSI SQL mode since it has different meanings for different MySQL versions (and breaks MySQL 5.7 support in Drupal 7)

I've investigated #2545480 and reported on why it breaks. It looks like a very easy patch, but I am not sure I am aware of all of the nuances of #344575 (original issue referenced in code comment).

@DamienTournoud: Did you mind taking a look?

Oops, I posted this in the wrong place. See #2545480: Don't use the ANSI SQL mode since it has different meanings for different MySQL versions (and breaks MySQL 5.7 support in Drupal 7).

I might be late to the party, but I am running MySQL 5.7.12 and experienced an issue with with a search_api-driven View displaying 0 results when sorted by a field, and it narrowed down to the forcing of ANSI and ONLY_FULL_GROUP_BY in the core MySQL driver. Removing those two gives the expected output of the View. I saw in #2545480 that someone had success downgrading their mysql version to 5.6, so I will test to see if that helps solve it.

Edit: 5.6 works just fine. For now, it might be good to recommend that people stick with 5.6? I'm unsure the full implications here, but 5.6 seems to work as expected in my case.