(WIP) CRM-16860 - Schema-upgrader should be merge-friendly #7440
Conversation
Move helper functions that are specific to revision-based approach: * buildQueue * doIncrementalUpgradeStart * doIncrementalUpgradeStep * doIncrementalUpgradeFinish
This is a cleaner high-level contract (less side-effecty, more accurate name, not tied to "$rev" concept).
The upgrade-handler class should no longer be determined by version. These classes are standalone entities (independent of version).
This function just calls `CRM_Core_DAO::executeQuery`, and it's only used once.
We won't care about sequential list of revision numbers. Instead, we'll want a list of named upgrade steps.
|
@colemanw @eileenmcnaughton - This is a replacement for #7421. |
…00_Legacy. Since 4.7 will be the new stable, any maintenance upgrades will go to 4.7. We want those to use the new merge-friendly upgrade scheme, so we should put all 4.7-related changes in a subdirectory to set the proper atmosphere.
totten
force-pushed
the
master-upgrade-files-2
branch
from
30aa66f
to
9741625
Compare
totten
force-pushed
the
master-upgrade-files-2
branch
from
9741625
to
6158210
Compare
|
Recent updates to PR:
Still haven't sorted out how to handle |
totten
force-pushed
the
master-upgrade-files-2
branch
from
5767b11
to
bda7dd3
Compare
|
Started analyzing how SchemaStructure works. It seems like any code that relies on version-based SchemaStructure would necessarily call List of use-cases per grep: https://gist.github.com/totten/1102df4870b15fa5fc1e |
|
@totten does the tracking go as far as cross version? The example I'm thinking of is a missing index. I want to add it to my 4.6 sites & not have the upgrade break when I get to 4.7. Let's pretend I was dealing with 4.7 & 4.8 since this patch is not in 4.7. Could I backport a mysql from 4.8 & run it against my 4.7 Db & have the db track it having already run? |
|
Yes. Supposing this upgrade-manager is used in both 4.7 and 4.8:
When the step executes, it stores a note ( |
|
@totten , I happen to be going through the backlog to pull requests, hitting this one. Thought maybe we could give it a heartbeat & comments on next steps, and potential contributions & contributors (dev & QA). Considering other changes (e.g., staged testing), maybe this even becomes less a priority, and can be moved to a "indefinite parking lot" (if there is such a thing)? From a personal standpoint, ideally, if something isn't going to happen (which maybe the case here), I prefer to close them. |
litespeedmarc
added
the
needs-work-not-review-ready
litespeedmarc
changed the title
litespeedmarc
added
needs-work-not-review-ready
|
@totten close & re-open later? |
|
Closing per above comments until someone is ready to get this review-ready |
|
@totten can you rebase this please? I will be reviewing this PR to see if it can improve our upgrade system for the 5.x release cycle. |
|
A few quick reactions on reopening/updating this:
|
|
Trying to digest/process a bit more about the multilingual schema stuff ties in here. This is a bit scratchpad with thoughts going in different directions. A. On general orderTrying to rephrase what's going on with this patchset... an upgrade is basically a bunch of steps, executed in order. This PR is based on the critique that:
Of course, we still need some way to sort upgrade steps. The PR (basically) changes the sort-key from "version number" to "filename" (and allows each version to have different/imperfectly-overlapping sets of files). I'm still ambivalent about the second claim that there's a lot of tolerance for small variations in order. It feels mostly true, and we can help make it true at a policy level (e.g. by avoiding backports that define upgrade steps or change schema), but it's hard to prove or be sure. If we really needed something more technically exacting, we could allow each step to express dependencies (and use some kind of topological sort). But I feel a little YAGNI on that -- tempted to wait for more evidence of the need. B. On styles of upgrade logicWhen upgrading a dataset, you can describe the upgrade imperatively (list of steps) or declaratively (describe the desired end-state). Declarative style is nice because it's almost automatic -- you don't have to explicitly write a list of steps; it just compares the start-state with the end-state and automatically picks a set of changes to reconcile any differences. But it's hard to generalize and automatically reconcile all schema changes. For example, suppose a new release converts a numeric column The imperative style is less automatic (you have to explicitly write each change), but it's fairly easy to improvise and define exactly the migration that you need. The existing logic is therefore a hybrid. It's mostly imperative, but (in between steps) it does a bit of declarative reconciliation for multilingual schema. C. Multilingual schema changes as concrete stepsOne way to understand the current operation of
The multilingual bit adds some extra calls after each step (to
But in practice, we don't have schema snapshots for every incremental step. (There are only a few files in
This analysis is appealing... because it leads to one fairly simple solution: whenever one feels inclined to make a new snapshot of the schema, just do it a little differently:
The file would be substantively similar, but it could use the "filename" as the key instead of "version number". Overall, the upgrade steps (when keyed by filename) would look more like:
However, I'm not certain this addresses every edge-case of multilingual. In particular, I think it would address the calls to D. On new upgrade steps vs old upgrade stepsAfter writing the original PR, I sorta regretted all the work that went into refactoring the old upgrade logic. That felt like the most brittle part of the PR, and (after a couple years) the content would fade into irrelevance. A narrower patch could be focused on adding the |
E. schemaStructureTables($rev,TRUE): ExperimentContinuing the open question from section C above...
I did an experiment to get a more concrete understanding of how Notwithstanding some timestamps, the upgrade results were the same (with or without F. schemaStructureTables($rev,TRUE): AnalysisWhy was it ever added? Why doesn't it matter now? My theory: This was just a special case of a more general problem that we've mitigated by other means. What does
The upgrader calls Upgrading+metadata is a more general problem. Several parts of the system (not just multilingual; also settings and various APIs/BAOs/DAOs) rely on metadata, and (for a period of time, several years ago) we had a few bugs because upgrade logic relied on unstable metadata. To avoid future bugs, we made a general KISS recommendation for upgrade logic: Prefer simple SQL semantics over API/BAO/DAO. If you keep the upgrade-logic simple+concrete, then it doesn't matter whether the active metadata reflects the old/pre-upgrade system, the new/post-upgrade system, or some intermediate form. Returning the question: How can it be that |
|
@totten One of the problems i have noticed with the multilingual upgrade and i have tried to mitigate it by having an isUpgradeMode on the views re-create. The problem i was seeing was this: Last known state - 4.7.alpha1 The build Views function then fails because it constructs a SELECT statement when going through the 4.7.12 stage which includes the column that is added in 4.7.15 because the DAOs have said this column must be in the database. My solution is probably a hack and probably caused issues with the rebuilding of the schema properly but its not been clear what is the "correct" solution. I think one solution could be is that if we are able to make use off the |
This refactors the
CRM_Upgradesystem that performs schema updates. The major goals:php/andsql/folders.)This creates a directory structure like:
WIP. TODO:
CRM_Upgrade_Form::setSchemaStructureTables($rev)