-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
(dev/core/68) Fix DB Error on 'Find Participant' page when MySQL FULL_GROUP_BY_MODE is enabled #11996
Conversation
This is working as expected. Using MySQL 5.7 with FULL_GROUP_BY_MODE you can now use Find Participants. |
I'm feeling really stung by regressions related to full group by fixes. I am aware of at least one unfixed regression from a full group by fix (https://issues.civicrm.org/jira/browse/CRM-21699) & one that I merged just this week (#10988) . I think we need to figure out how to lift the bar on these. I don't think we should merge any more full group by fixes without @seamuslee001 & I discussed over on #11954 a bit |
This seems to be baulked on the lack of a unit test |
I think it's baulked on more than that from my pov - it's a really central change & it's baulked on us having an appropriate way to work with full group by & on someone being able to confirm they have been running this in production on more than one site for several months |
* @param array $orderBys - ORDER BY sub-clauses | ||
* | ||
*/ | ||
public static function getGroupByFromOrderBy(&$groupBy, $orderBys) { | ||
if (!CRM_Utils_SQL::disableFullGroupByMode()) { | ||
foreach ($orderBys as $orderBy) { | ||
$orderBy = str_replace(array(' DESC', ' ASC', '`'), '', $orderBy); // remove sort syntax from ORDER BY clauses if present | ||
$orderBy = str_ireplace(array(' DESC', ' ASC', '`'), '', $orderBy); // remove sort syntax from ORDER BY clauses if present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton so this change is about making it so that its not case sensitive iirc
* @param array $orderBys - ORDER BY sub-clauses | ||
* | ||
*/ | ||
public static function getGroupByFromOrderBy(&$groupBy, $orderBys) { | ||
if (!CRM_Utils_SQL::disableFullGroupByMode()) { | ||
foreach ($orderBys as $orderBy) { | ||
$orderBy = str_replace(array(' DESC', ' ASC', '`'), '', $orderBy); // remove sort syntax from ORDER BY clauses if present | ||
$orderBy = str_ireplace(array(' DESC', ' ASC', '`'), '', $orderBy); // remove sort syntax from ORDER BY clauses if present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so - this seems totally safe IMHO - if it's OK for non sensitive then case sensitive is ok
CRM/Contact/BAO/Query.php
Outdated
// if ORDER BY column is not present in GROUP BY then append it to end | ||
if (preg_match('/(MAX|MIN)\(/i', trim($orderBy)) !== 1 && !strstr($groupBy, $orderBy)) { | ||
$groupBy .= ", {$orderBy}"; | ||
if (preg_match('/(MAX|MIN)\(/i', trim($orderBy)) !== 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so change is coping with possibility of non-string $groupBy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the purpose of this if is to find any columns that are aggregated in the select and not add them to teh group by right?
CRM/Contact/BAO/Query.php
Outdated
return !strstr($var, $orderBy); | ||
}); | ||
} | ||
elseif (!strstr($groupBy, $orderBy)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is unchanged
CRM/Contact/BAO/Query.php
Outdated
$groupBy .= ", {$orderBy}"; | ||
if (preg_match('/(MAX|MIN)\(/i', trim($orderBy)) !== 1) { | ||
if (is_array($groupBy)) { | ||
// retrieve and add all the ORDER BY columns which are not in GROUP BY list of columns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the change is this - the possibility of groupBy being an array & it now being handled if it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if its not present in the array add it otherwise don't worry
CRM/Contact/BAO/Query.php
Outdated
@@ -4898,16 +4907,16 @@ public function searchQuery( | |||
list($select, $from, $where, $having) = $this->query($count, $sortByChar, $groupContacts, $onlyDeleted); | |||
|
|||
if (!empty($groupByCols)) { | |||
if (!empty($order)) { | |||
// retrieve order by columns from ORDER BY clause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this re-ordering - not sure the reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the reasoning is that if any of the orderBy Columns are not already in the Select statement then they would be added to the GroupBy but not Select because of L4917
so - looking at this with @seamuslee001 - I have a generalised high-level of concern about FGB fixes based on the experience we have had. However, I note that this PR is not adding FGB handling but tweaking existing handling - to the point of handling a different format for $groupBy in this function, and that @seamuslee001 has added a test. I also confirm per @seamuslee001 & @lcdservices that this does fix a problem. So, if we assume the FGB handling is generally working then extending it to an area that is known to be broken on FGB sites should be safe-ish (or at least more so than adding it to new code areas.....) |
Hmm I guess it DOES add fgb handling to more areas by catching more places |
So - my understanding is that what this does is IF a column is in the order by but NOT the group by then it will be added to the group by clause - at the mysql level this is a similar change to which caused this regression https://issues.civicrm.org/jira/browse/CRM-21699?filter=24614 We have a situation where the GROUP BY clause is generally 'correct' because when people have been writing the queries that was their focus. Order bys are a bit more hit & miss - but now we are changing Order by to match Group by through this & this PR DOES extend it. So, I think we maybe need to find a bunch of places where this is called with $groupBy as an array and actually get the before & after query output to test against data |
(I take back my 'safeish' & 'low-risk' comments after more thought) |
Here is the original query
And with FGB support added
WITH FGB + this patch I'm getting this:
Which seems OK - but I feel the point of the patch is to add |
@eileenmcnaughton @monishdeb so i'm putting this through its paces on a php5.6 MySQL 5.7 test box by running ALL the tests It has detected one legit failure which is a regression it seems https://gist.github.com/seamuslee001/586c83c01098cd74c4c55c7ebdc72d22 |
@eileenmcnaughton @seamuslee001 let me explain my fix in points:
|
@seamuslee001 the failure https://gist.github.com/seamuslee001/586c83c01098cd74c4c55c7ebdc72d22 is strange because it should have added the ORDER BY columns in GROUP BY as per the fix :( |
@monishdeb so the thing that is most likely to turn a query that returns the right result, but does not meet the full group by standard is to do what we did in #10926 - ie we added a column to the GROUP BY column because it was in the ORDER BY - thus changing the actual outcome in an unpredictable way. So as we hone in on the risks on FGB patches - it seems anything that adds to the GROUP BY clause is at high risk of taking a working query and messing with the results. Given this risk - is the hypothetical benefit of a 'better' query worth the risk is it actually worse due to using a GROUP BY that was not intended when the query was written? |
Hmm right at one hand it will satisfy the FGB but on the otherhand doing so will keep adding GROUP BY columns if there are more such columns present in ORDER BY. So in order to fulfil FGB but not to cause any drastic change to SQL output I think we should bring that disable/renable FGB function ? In that way we doesn't need to worry about FGB errors anymore. |
The failing test is a great example of the risk here is the query in the test
When I run it locally on my DB I get 460 rows returned If I CHANGE the group by to include the columns in the ORDER BY I get 513 rows returned - so if we had 'fixed' it because it 'should have added the ORDER BY columns in GROUP BY as per the fix' then we would get 53 rows returned that the original developer never had a plan for ... |
@monishdeb I agree on the disable & re-enable sadly. I get the logic of what we are trying to do to meet fgb compliance but it's very very hard to retrofit that on our search logic which has a lot of quirks |
…_GROUP_BY_MODE is enabled
@eileenmcnaughton @seamuslee001 @lcdservices as per the discussion I have to choose to disable/reenable FGB when there are GROUP BY columns present. Please check my latest change. |
This would seem to get rid of those nasty fatal errors without making changes to query output that could have unexpected results |
Jenkins test this please |
I've added merge ready - this should be merged IMHO but it gives it a cool down - I don't believe SELECT SQL_CALC_FOUND_ROWS will hit us here |
Merging now - then we will re-run #12085 & see how it goes. Based on online feedback this may be a candidate for backporting |
Thanks @eileenmcnaughton |
Overview
Steps to replicate:
Issue: https://lab.civicrm.org/dev/core/issues/68
Before
This leads to DB Error - https://pastebin.com/jxbaAbpS
After
Fixed the DB Error