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

Export merge to household - fix DB error relating to fields too long for table. #13338

Merged
merged 1 commit into from
Jan 2, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fixes a fatal error when trying to combine merge households and 'primary fields' on some mysql configs

Before

depending on mysql this could result in a DB error

Database Error Code: Row size too large (> 8126). Changing some columns to TEXT or BLOB may help. In current row format, BLOB prefix of 0 bytes is stored inline., 1118

After

Less fields written to the temp table so DB error not encountered

Technical Details

The key fix here is that the fields for merging to household are no longer added to the temporary table.

They are fully loaded in php anyway so there is no performance gain using a temp table.
Instead we iterate them in php as we process each row.

Comments

@mattwire @twomice @monishdeb - this is the PR that actually fixes the DB error after all the prep work.

It will need a bit of testing! I have just done very basic tests so far.

@civibot
Copy link

civibot bot commented Dec 21, 2018

(Standard links)

@civibot civibot bot added the master label Dec 21, 2018
@seamuslee001
Copy link
Contributor

Test failure seems to relate i think

@twomice
Copy link
Contributor

twomice commented Dec 27, 2018

FYI, test failures seem irrelevant and would not reproduce on my local dev.

CRM_Report_FormTest::testGetFromTo.testGetFromTo with data set #1
CRM_Report_FormTest::testGetFromTo.testGetFromTo with data set #2
CRM_Utils_DateTest::testGetFromTo.testGetFromTo with data set #1
CRM_Utils_DateTest::testGetFromTo.testGetFromTo with data set #2

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

related fail was fixed - we just seem to have a variety on unrelated fails now.

Would be good to merge this before rc is cut on 2 Jan

@eileenmcnaughton
Copy link
Contributor Author

@mattwire @twomice tests passing now - would be good to merge this before rc is cut on 2 Jan

@mattwire
Copy link
Contributor

I have tested the following contact exports:

  • Do not merge: OK
  • Merge same address: OK
  • Merge households: FAIL

I don't think merge households is actually working. On a test database (210 contacts) I get 210 contacts exported. If I do the same on git master (but change temp table definition to MyISAM so it executes) I get 140 contacts on export.

…for table.

The key fix here is that the fields for merging to household are no longer added to the temporary table.

They are fully loaded in php anyway so there is no performance gain using a temp table.
Instead we iterate them in php as we process each row
@eileenmcnaughton
Copy link
Contributor Author

@mattwire it was replacing with household details - but for every row - now it's skipping if the household is already exported

@mattwire
Copy link
Contributor

mattwire commented Jan 2, 2019

@eileenmcnaughton Confirmed the export contacts now do the same for all 3 and merge households is exporting the correct result. @seamuslee001 Can this be merged? Have we cut the RC yet, would be good to have this in the RC so it gets a full month of testing - bonus being merge household export actually works which it hasn't for quite a long time...

@eileenmcnaughton
Copy link
Contributor Author

Merging based on @mattwire review

@eileenmcnaughton eileenmcnaughton merged commit 30fdcb2 into civicrm:master Jan 2, 2019
@eileenmcnaughton eileenmcnaughton deleted the export branch January 2, 2019 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants