-
-
Notifications
You must be signed in to change notification settings - Fork 817
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
CRM-21298 System check: bypass missing indices check until update indices works #11250
Conversation
I'm ok with merging this since it has caused issues. Note that we are in a bind now though. We have no clear analysis as to which sorts of indexes are still causing problems, given that @jitendrapurohit worked to fix this & appears to have fixed some but not 100% of them (but presumably all those that affect his customers). So I don't think there is enough clear information to fix the remaining bugs once we merge this. This is our main method now to add indexes in upgrades (and it would be bad for that to change) so we are kind of in a 'don't add any new indexes' pattern now. (or do but don't expect them to be changed on existing installs since they won't know) |
I just changed the tag from merge ready to merge on pass since I guess other people might want to chime in before it's merged |
@eileenmcnaughton I hope that this is a very short-term measure, and maybe a fix could be done in the course of the 4.7.29 cycle. I actually don't know anything about any of the considerations under the hood on the "update indices" process, and I suspect that most users, implementors, and developers don't either. I think that's why we resent it so much: the experience is that CiviCRM was doing just fine, and then all of a sudden after an upgrade We all on here know that's not the case, but most CiviCRM users aren't working with a partner, let alone one who follows changes in the code. That's why I'm of the mind that this is so urgent. |
Being hyperaware about CiviCRM can be a good thing in some instances like this one. I, for one, can appreciate a notice from Civi about missing indices. As long as this pull request doesn't completely suppress the notice from, say, the civi status page.. |
@clnlf It will completely suppress the notice. I haven't been able to bring myself to merge this without putting aside a day to do proper analysis on what remaining issues remain with running the script - but I'm open to another merger merging it. |
Ok if we are still getting fatal errors from the fix indexes tool then we should merge this for now. I share the hope that this is only a temp fix. |
CRM-21298 System check: bypass missing indices check until update indices works
Overview
The "Performance warning: Missing indices" system check should be bypassed until the "Update Indices" task works reliably.
This is the same as #11114 but for
master
.Before
Users of many sites (including as one example a demo site where 4.7.17 was installed and then upgraded to 4.7.25) are presented with a
WARNING
level message in the system check stating that they have missing indices. An Update Indices button is in the message. Clicking the button (for many sites) causes a database error as documented in CRM-20817.After
The system check will never produce the "Performance warning: Missing indices" message.
Technical Details
I left all the code in there to run the check and update the indices. The
checkIndices()
function just returns an empty array before getting to that point. This allows for easy removal for testing and when the update tool is actually reliable.