CRM-20533 - avoid errors when creating missing indices.

[jmcclelland]

Drop existing indices with the same name and drop and re-create
dependent foreign keys.

---

• CRM-20533: 'Update Indices' System Status action can get tied up (index name already exists, localised dbs?)

[xurizaemon]

Tested this on an affected site and it worked well, thanks @jmcclelland!

[jitendrapurohit]

As the changes made here loops on $missingIndices variable, I think this won't delete any existing entry if there is only duplicate indexes in the table with empty $missingIndices.

This is much likely to happen as $missingIndices will only be set if the complete order of columns matches to the expected indices in the table.

Can we migrate this to getMissingIndices() function instead. We already get outdated duplicate index in $existingKeyIndices variable so we can delete that straight away as soon as we find it.

[jmcclelland]

Thanks for the feedback @jitendrapurohit .

I just rebased against master and removed the duplicate code. How does this look?

[xurizaemon]

Jenkins, test this please?

@jmcclelland This patch does not apply to master for me currently. I can't tell why - Jenkins seems to be applying it OK, and applying the individual commit .diffs does work ... don't worry about this unless you have issues too!

Got it: If applying to 4.7.22, this depends on #10566. Still IDK why my local git copy would not merge it, but nvm.

[davialexandre]

While upgrading a site to CiviCRM 4.7.22 I had problems to create the missing indices. During the investigation, I found this PR and also #10566. Even though I hadn't had the chance yet to fully review the code and check the logics, a few things caught my attention:

• Before CRM-20774 - Add check for existing key index in table #10566, the getMissingIndices() method was simple enough and had a single responsibility. Now it has two: return the missing indices AND the existing ones
• The same way, before this PR, the createMissingIndices() also was simple and had a single responsibility. Now it has a new optional param, which is a smell that a method is doing too much. Also, the method is huge now, with nested loops and ifs and elses. All signs that it can be refactored and split into smaller methods
• The method docblock wasn't updated to document the new param
• There are a lot of inline comments in createMissingIndices(). Usually, this is a sign of things that can be extracted to new methods and the comments should be their docblocks
• The syntax style for variables names is a bit inconsistent. Sometimes it's using snake_case and sometimes it's using camelCase

@jmcclelland I understand you're not responsible for the changes to the getMissingIndices() method, but since this PR depends on them and it has been merged already, I think it makes more sense to comment about it here.

[eileenmcnaughton]

I agree with @davialexandre that getExistingIndices should be a separate function to getMissingIndices ... and with his comments above in general

[jitendrapurohit]

okay, there seems to be some terminology confusion as getMissingIndices() doesn't actually return the existing indices but the false/incomplete indices which need to be deleted manually.

Anyway, the ambiguity is being removed now in an alternate PR #10908 with a straightforward approach of deleting incomplete indices directly from the Update Indices button (more details on the PR). Please let me know if you have any thoughts

[eileenmcnaughton]

@jitendrapurohit there are several attempts at fixing this issue - can we close this PR since it seems you superceded it.

Annecdotally there is still a variant we have not caught but I don't really know what it is

[eileenmcnaughton]

I'm closing this & keeping #10415 open - the latter does not have the fix but @aydun has done good analysis on it & continuing there makes sense