progressivetech / net.ourpowerbase.sumfields

The Summary Fields extension creates read-only custom data fields that extend Contact and are automatically populated with up-to-date totals such as total lifetime contributions, last contribution amount, last attended event, etc.
Other
8 stars 29 forks source link

Full Group By error on 4.0.2 upgrade #48

Closed MegaphoneJon closed 5 years ago

MegaphoneJon commented 5 years ago

I receive an error on upgrade from 3.1.3 to 4.0.2. The SQL that triggers the error plus the error are below:

INSERT INTO `civicrm_temp_4be00c9efdfe0638b5c627b863a056ea` SELECT contact_id, (SELECT COALESCE(total_amount,0)
      FROM civicrm_contribution t1
      JOIN civicrm_line_item t2 ON t1.id = t2.contribution_id
      WHERE t1.contact_id = (SELECT contact_id FROM civicrm_contribution cc WHERE cc.id = trigger_table.contribution_id)
      AND t1.contribution_status_id = 1  AND t2.financial_type_id IN
      (12,15,16,17,14,18,4,7,42,41,40,11,39,56,55,57,53,54,9,13,37,38,36,34,35,52,51,48,49,50,2,5,1,8,6,24,25,26,27,28,10,47,46,44,43,45) ORDER BY t1.receive_date DESC LIMIT 1),
(SELECT MAX(receive_date) FROM civicrm_contribution t1
      JOIN civicrm_line_item t2 ON t1.id = t2.contribution_id
      WHERE t1.contact_id = (SELECT contact_id FROM civicrm_contribution cc WHERE cc.id = trigger_table.contribution_id) AND t1.contribution_status_id = 1 AND
      t2.financial_type_id IN (12,15,16,17,14,18,4,7,42,41,40,11,39,56,55,57,53,54,9,13,37,38,36,34,35,52,51,48,49,50,2,5,1,8,6,24,25,26,27,28,10,47,46,44,43,45)),
(SELECT COALESCE(MAX(total_amount), 0)
      FROM civicrm_contribution t1
      JOIN civicrm_line_item t2 ON t1.id = t2.contribution_id
      WHERE t1.contact_id = (SELECT contact_id FROM civicrm_contribution cc WHERE cc.id = trigger_table.contribution_id) AND
      t1.contribution_status_id = 1 AND t2.financial_type_id IN (12,15,16,17,14,18,4,7,42,41,40,11,39,56,55,57,53,54,9,13,37,38,36,34,35,52,51,48,49,50,2,5,1,8,6,24,25,26,27,28,10,47,46,44,43,45)) FROM `civicrm_line_item` AS trigger_table JOIN civicrm_contribution AS c ON trigger_table.contribution_id = c.id GROUP BY contact_id [nativecode=1055 ** Expression #2 of SELECT list is not in GROUP BY clause and contains nonaggregated column '9to5_dev_civi.trigger_table.contribution_id' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by]
jmcclelland commented 5 years ago

Thanks Jon - you should be able to safely downgrade (there are no database updates). Ug. Debugging full group by is no walk in the park.

mfb commented 5 years ago

Ouch :) we hit these group by errors too.

MegaphoneJon commented 5 years ago

I'm not particularly trigger-savvy - but perhaps we could write a pure SQL version of CRM_Core_DAO::disableFullGroupByMode()? I'm not even sure we'd need CRM_Core_DAO::reenableFullGroupByMode() since I assume triggers run in their own process.

mfb commented 5 years ago

The three select expressions are:

(SELECT COALESCE(total_amount,0)
      FROM civicrm_contribution t1
      JOIN civicrm_line_item t2 ON t1.id = t2.contribution_id
      WHERE t1.contact_id = (SELECT contact_id FROM civicrm_contribution cc WHERE cc.id = trigger_table.contribution_id)
      AND t1.contribution_status_id = 1  AND t2.financial_type_id IN
      (12,15,16,17,14,18,4,7,42,41,40,11,39,56,55,57,53,54,9,13,37,38,36,34,35,52,51,48,49,50,2,5,1,8,6,24,25,26,27,28,10,47,46,44,43,45) ORDER BY t1.receive_date DESC LIMIT 1),
(SELECT MAX(receive_date) FROM civicrm_contribution t1
      JOIN civicrm_line_item t2 ON t1.id = t2.contribution_id
      WHERE t1.contact_id = (SELECT contact_id FROM civicrm_contribution cc WHERE cc.id = trigger_table.contribution_id) AND t1.contribution_status_id = 1 AND
      t2.financial_type_id IN (12,15,16,17,14,18,4,7,42,41,40,11,39,56,55,57,53,54,9,13,37,38,36,34,35,52,51,48,49,50,2,5,1,8,6,24,25,26,27,28,10,47,46,44,43,45)),
(SELECT COALESCE(MAX(total_amount), 0)
      FROM civicrm_contribution t1
      JOIN civicrm_line_item t2 ON t1.id = t2.contribution_id
      WHERE t1.contact_id = (SELECT contact_id FROM civicrm_contribution cc WHERE cc.id = trigger_table.contribution_id) AND
      t1.contribution_status_id = 1 AND t2.financial_type_id IN (12,15,16,17,14,18,4,7,42,41,40,11,39,56,55,57,53,54,9,13,37,38,36,34,35,52,51,48,49,50,2,5,1,8,6,24,25,26,27,28,10,47,46,44,43,45))

If you wrap each of these with some kind of aggregate function like MAX() then the query runs.

mfb commented 5 years ago

...Running this query on a snapshot of our largish database took 37 minutes and 39 seconds but it did work :)

jmcclelland commented 5 years ago

Hi all - I just pushed a commit that I think should solve this problem. Would you mind trying it out and seeing if it works for you?

I used @mfb's suggestion of wrapping each trigger sql clause in MAX(). Since these only should produce one value, it won't affect the result, but will satisfy ONLY_FULL_GROUP_BY.

Also, I believe this bug is only triggered when the initial values are populated and not when the trigger is called, since the triggers don't use the same GROUP BY technique that is used when the data is initialized. So, I've left the triggers alone.

As a side note in case I'm wrong... I noticed that the trigger table (at least in my version of Maria DB) has a sql_mode column that suggests that triggers can use a sql_mode different then what is set globally. I saw this in the MySQL docs:

MySQL stores the sql_mode system variable setting in effect when a trigger is created, and always executes the trigger body with this setting in force, regardless of the current server SQL mode when the trigger begins executing.

So... if we did need to muck with the sql mode, it seems as though we could unset ONLY_FULL_GROUP_MODE when setting the trigger and it would stick.

MegaphoneJon commented 5 years ago

I can look at this tomorrow. Thanks @jmcclelland and @mfb for all your work on this!

MegaphoneJon commented 5 years ago

I can confirm that this fixes the error for me. Thank you @jmcclelland!

jmcclelland commented 5 years ago

Great - thanks for testing Jon.