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.

Sign up for GitHub

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

Jump to bottom

CRM-20313,CRM-19357: Add index to civicrm_activity.status_id and unique index to civicrm_entity_financial_account table #10056

Closed
wants to merge 1 commit into from
Closed

CRM-20313,CRM-19357: Add index to civicrm_activity.status_id and unique index to civicrm_entity_financial_account table #10056

wants to merge 1 commit into from

Conversation

Edzelopez
Copy link
Contributor

@Edzelopez Edzelopez commented Mar 27, 2017 •
edited by civicrm-builder
Loading

@Edzelopez Edzelopez mentioned this pull request Mar 27, 2017
Closed
@monishdeb
Copy link
Member

jenkin test this please

@Edzelopez
Copy link
Contributor Author

Is this okay to merge?

@monishdeb
Copy link
Member

monishdeb commented Mar 28, 2017 •
edited
Loading

Tested on upgrade and install. The PR works for me.

@colemanw @eileenmcnaughton can you please merge this PR if the final patch looks good to you.

colemanw
colemanw previously requested changes Mar 30, 2017
View reviewed changes
CRM/Upgrade/Incremental/php/FourSeven.php Outdated
$tables = array('civicrm_activity' => array('status_id'));
CRM_Core_BAO_SchemaHandler::createIndexes($tables);
return TRUE;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this should be a generic function following the same pattern as \CRM_Upgrade_Incremental_Base::addColumn() and \CRM_Upgrade_Incremental_Base::dropColumn.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw we've been recommending that function. I guess it could be moved to another class - but there are non-upgraded reasons to add & drop indexes.

My reservation about this & the 2 other open PRs for adding indexes is that on large sites adding indexes to large tables can be very slow and I feel like we should complete the work @aydun started to allow indexes to be declared & sites to reconcile them, rather than forcing them to either not upgrade to the next point version or have a significant outage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying don't use the CRM_Core_BAO_SchemaHandler::createIndexes function. I'm saying wrap it in a generic function in CRM_Upgrade_Incremental_Base instead of wrapping it in this one-off function.

@monishdeb monishdeb force-pushed the JoeMurray-patch-2 branch 2 times, most recently from a12253e to 1136146 Compare March 31, 2017 19:44
@monishdeb
Copy link
Member

@colemanw I have raised a separate ticket to add these two functions

CRM_Upgrade_Incremental_Base::addIndex($ctx, $table, $column)
CRM_Upgrade_Incremental_Base::dropIndex($ctx, $table, $indexName)

So this PR is depedent on #10086 and I have also updated this PR accordingly.

@monishdeb
Copy link
Member

Jenkin test this please

@monishdeb
Copy link
Member

@colemanw I have made the changes like you suggested, can you merge it?

@monishdeb monishdeb force-pushed the JoeMurray-patch-2 branch from 1136146 to d2699ac Compare April 1, 2017 04:56
@eileenmcnaughton
Copy link
Contributor

Note that if we merge this we should also merge the other PRs for adding indexes if 4.7.19 is going to be painful for large sites we should consolidate the pain

@monishdeb monishdeb force-pushed the JoeMurray-patch-2 branch 2 times, most recently from 3a6abbd to e9b4845 Compare April 5, 2017 05:19
@monishdeb monishdeb changed the title CRM-20313 Add index to civicrm_activity.status_id CRM-20313,CRM-19357: Add index to civicrm_activity.status_id and unique index to civicrm_entity_financial_account table Apr 5, 2017
@monishdeb
Copy link
Member

@eileenmcnaughton I have included the #10023 changes in this PR as it was dependent on this patch. Is there any other PR which adds index in 4.7.19 ?

Bdw test build is failing for each PR at the moment

@monishdeb monishdeb mentioned this pull request Apr 5, 2017
Closed
@monishdeb
Copy link
Member

Jenkin test this please

@eileenmcnaughton
Copy link
Contributor

I'm happy to merge the schema changes & changes for new installs. I would prefer to use a better approach for adding this & other schema changes via update - @aydun has done 95% of the work & #10109 has all except a small piece ready to go in.

Shall we get the DAO & schema changes merged & the new install sql for this & the other indexes & then worry about how to add them to existing installs in a non-damaging way?

@colemanw
Copy link
Member

colemanw commented Apr 8, 2017

@monishdeb I merged #10131 can you please remove the similar code from here?

@monishdeb
Copy link
Member

@colemanw @JoeMurray I am removing the upgrade code as suggested by Eileen here. Will check what the best way to handle these indexes in upgrade after #10109 gets merged.

@monishdeb monishdeb force-pushed the JoeMurray-patch-2 branch 3 times, most recently from 143fde6 to d3d5cfd Compare April 10, 2017 09:33
@eileenmcnaughton eileenmcnaughton dismissed colemanw’s stale review April 10, 2017 20:10

issues in review addressed

@eileenmcnaughton
Copy link
Contributor

The test failures are related - likely either something wrong with the test or this has revealed an actual bug

@monishdeb monishdeb force-pushed the JoeMurray-patch-2 branch 3 times, most recently from 9332a3f to eaca7bc Compare April 12, 2017 17:25
@monishdeb
Copy link
Member

@eileenmcnaughton the test build passed. Please have a look at the final patch

@eileenmcnaughton
Copy link
Contributor

@monishdeb so 2 things here ...

  1. 'name' => 'index_entity_id_entity_table', - our standard naming is to include all the fields - I don't know if there is any issue with a longer name but if not we should use that
  2. there will also be a change to the install sql to go in the commit

Note it would be ideal to add these indexes before running the mysql
civicrm_activitiy.activity_date_time https://issues.civicrm.org/jira/browse/CRM-20204
civicrm_mailing_event_queue.hash https://issues.civicrm.org/jira/browse/CRM-19383

@eileenmcnaughton
Copy link
Contributor

Looks like the sql doesn't get committed - so that part is moot. We probably still should change that index name. Looks like 64 is the max http://stackoverflow.com/questions/16920001/maximum-length-of-a-column-name-in-mysql and I don't think we are there

@monishdeb
Copy link
Member

monishdeb commented Apr 13, 2017 •
edited
Loading

@eileenmcnaughton I have changed the index name and then included the civicrm_generated.mysql after running UI. Ensured from UI that everything is running fine.
Here's the changed index in table

  1. Activity.status_id index
    image

  2. Unique key index_entity_id_entity_table_account_relationship
    image

@JoeMurray @monishdeb
CRM-20313,CRM-19357: Add index to civicrm_activity.status_id and uniq…
d9155f3 
…ue index to civicrm_entity_financial_account table
@seamuslee001 seamuslee001 mentioned this pull request Apr 16, 2017
Merged
@eileenmcnaughton
Copy link
Contributor

closed in favour of #10169

@monishdeb monishdeb closed this Apr 16, 2017
@monishdeb monishdeb deleted the JoeMurray-patch-2 branch April 16, 2017 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eileenmcnaughton eileenmcnaughton eileenmcnaughton left review comments

@colemanw colemanw colemanw left review comments

Assignees
No one assigned
Labels
master merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Edzelopez @monishdeb @eileenmcnaughton @colemanw @civicrm-builder @JoeMurray