When using the --sync option during migrations, they get exponentially slower with the number of items to process.
One of my migrations has 35K items and the first phase during MigrateEvents::PRE_IMPORT takes a whole 20 minutes before even starting the migration itself (which takes even longer).
After profiling and investigating, I think the way migrate_tools keeps track of all source rows in migrate_tools_migrate_prepare_row is suboptimal and could be greatly improved.
Currently, it relies on States to make a list of IDs grow. Which implies loading some serialized data from the database, adding something to it, and re-serializing it to the database. For each row. It means that for each row, the amount of data to serialize and unserialize gets bigger and bigger. Here is where the main culprit is:
function migrate_tools_migrate_prepare_row(Row $row, MigrateSourceInterface $source, MigrationInterface $migration) {
if (!empty($migration->syncSource)) {
// Keep track of all source rows here [...]
$source_id_values = \Drupal::state()->get('migrate_tools_sync', []);
$source_id_values[] = $row->getSourceIdValues();
\Drupal::state()->set('migrate_tools_sync', $source_id_values);
}
}
After tweaking how this tracking works, I got it down from 20 minutes to less than 45 seconds in time.
Also as a bonus, I'm pretty sure the current system using states doesn't allow to run safely several migrations in parallel, so I've added the migration_id to the dedicated table so each migration has its own separated set of tracked IDs.
Have a big migration to run (with tens of thousands of entries) and run it with the --sync option. Illustration:
drush migrate:import YOUR_MIGRATION --sync
Rework the way the module keeps track of rows' source IDs in a dedicated database table, so adding new entries doesn't get slower and slower as the migration progresses.
None.
None.
Use a dedicated database table for source IDs tracking.
-- --
I'm planning on creating a MR shortly. I just need to figure out how to use the new MR system.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #7
pacproduct commentedI'm not sure what makes tests fail. Seems like the new database table is not found? When I install the module locally, it does get created correctly though.
What am I missing? Any help for fixing the CI would be appreciated :)
EDIT: Ah do I need to install schemas needed for tests? I'll try figuring this out...
Comment #8
pacproduct commentedYay! Tests pass :)
Switching status to "needs review".
Comment #9
pacproduct commentedComment #10
pacproduct commentedJust added a commit.
I noticed that the
migrate_tools_migrate_prepare_rowhook is running during the PRE_IMPORT phase, and also during the IMPORT phase. That was duplicating all SyncID entries although we need them during the PRE_IMPORT phase only. So I added a little something so IDs get recorded during the PRE_IMPORT phase only. Then, they stay available in database during the actual migration phase just in case another module would need them, and they get cleaned during the POST_IMPORT phase.Also added some missing comments.
Comment #11
simon georges commentedComment #12
denix commenteddear all,
we are following closely this patch because when working with large datasets the operation is incredibly expensive in terms of performances. We are running tests on our side and we can only appreciate better performances and no drawbacks for the time been.
Comment #13
Jorge Navarro commentedI tested it in a large migration and indeed, migration time become a fraction, looking forward to see it merged!
Comment #14
simon georges commentedConsidering several people have reported it working without drawbacks, let's try to move it forward ;-)
Comment #15
maxpahJust for information, with this patch (who is working fine) there is a log on reports/status page :
READ-COMMITTED
For this to work correctly, all tables must have a primary key. The following table(s) do not have a primary key: migrate_tools_sync_source_ids. See the setting MySQL transaction isolation level page for more information.
I think it's just this line missing in hook_schema declaration:
'primary key' => ['migration_id'],Comment #16
pacproduct commented@MaxPah I cannot reproduce your issue. Also,
migration_idcannot be used as primary key as this column doesn't contain unique entries.We could add an arbitrary serial 'id' as unique identifier and as primary key. Although it won't be used by the module, I guess it's good practice anyway.
I'll rework that shortly.
Comment #17
simon georges commentedComment #18
pacproduct commentedBecause branch `6.0.x` recently changed, I had to rebase my MR and resolve conflicts. However, now the MR itself doesn't apply anymore to version `6.0.2` of this module.
So here is an attached patch specfically for version `6.0.2`, so people can keep applying it easily if needed.
Comment #19
emixaam commentedSuccessfully tested Merge request 44 with migrate_tools 6.0.4 and migrate_plus 6.0.2, on Drupal 10.2.3. On a local environment, I tested removing 10000 items from the source of a custom migration and executing ´drush mim --sync´. The items were correctly removed, and the time to complete the sync import was much faster than without the patch.
Was there more thoughts given into the discussion #mr44-note197623 about using MigrateTools as a service vs moving to a dedicated service? I think it would make sense but I'm new on the issue.
Comment #20
pacproduct commented@emixaam I'm not sure what the best approach would be.
If the issue is that the service name is weird, I guess we could:
- Rename the class and its corresponding service. That could impact some code elsewhere in the project, I did not check.
- As the services currently declared by this module don't always match the name of their corresponding classes, another approach would be to rename the service only but not the class itself (e.g. "migrate_tools.helper").
If the issue is that the source code is not well-enough organized, that would require a bit more rework but maybe we could:
- Move all these functions into a separate service, but I'm not sure how to call it: these are utility helper functions for tracking source IDs during the first phase of a migration, insuring the module knows which entries to delete. But there's already a service with a name suggesting it handles that sync task (among other things): "migrate_tools.migration_sync" (
Drupal\migrate_tools\EventSubscriber\MigrationImportSync).- Therefore, another consideration would be to completely remove class
Drupal\migrate_tools\MigrateTools(which initially contained a small static functionbuildIdList) and put everything into the existing service "migrate_tools.migration_sync"I feel like the decision is up to the maintainer on what the best approach for their module is at this point, but what do you guys think?
Comment #21
bramdriesenUploading a static patch from the MR since the patch of #18 No longer applies to the latest release (6.0.4)
Comment #22
sassafrass commentedPatch applied cleanly for me on 6.0.4 and has helped.
Comment #23
antongp commentedState service uses a cache collector for performance since Drupal 10.3 https://www.drupal.org/node/3177901
Though on existing sites this feature needs to be enabled explicitly through the settings flag
$settings['state_cache'] = TRUE;, this is the default behavior for new sites (in default.settings.php), and in Drupal 11+ the setting is removed and state cache is permanently turned on.There are 2 queries presented in the change record to check current state usage and then decide to opt-in/out state cache depending on results.
Size/length can easily exceed 100,000 on thousands migration items. Setting the issue priority to Major...
Comment #24
vermario commentedAgree with the previous poster, this will become a problem in Drupal11 for sure. Sounds like it would be great to push this change forward.
In my test with our fairly large migration executed with --sync, after the patch the operation of checking for changes takes 6 seconds instead of 2 minutes 30 seconds.
I tried manipulating the source of the migration (changing data, editing one item, deleting data) and the migration was behaving accordingly.
I hope we can merge this soon :-)
Comment #25
scott_euser commentedThanks very much for this! We have tested this out quite thoroughly as we were similarly hitting an issue syncing roughly 40,000 entities from a CRM twice per hour and the sync was hanging. We have been running this for several days without issue as we figured its fairly safe since we could always remove the patch and re-sync the content the ~20 minute way.
We also did notice the issue of multiple migrate --sync's also having this problem:
Which is sorted by this MR.
The only thing I can suggest further is a subsequent follow-up where we could add a cron job just to check if there are any closed migrate IDs in the new table that still have data in the table itself. Essentially the code without this patch was causing our database to pile up each time in the key-value whenever it timed out, with orphan key-value data since the clean-up phase was never reached. While this is far more performant, its I suppose still possible some failure leaves orphans in the table if the clean-up step in this MR is never reached (ie,
::cleanSyncData()). But definitely a follow-up thing IMO.Comment #26
scott_euser commentedComment #27
scott_euser commentedCleaned up patches so its clear what we tested (ie, the visible MR)
Comment #28
hestenetThis would be helpful for Drupal.org's site migrations and the time period that we have Modern Drupal and D7 running in parallel.
Comment #29
fjgarlin commentedFor context, our user migration is +2 million records, so holding that number of IDs in memory is not an option.
Comment #32
heddnThanks everyone for your contributions.
Comment #34
loopy1492 commentedI just pulled our database from prod which is running migrate_tools version 6.0.4, then ran updates locally from version 6.0.4 to version 6.0.5 (according to composer.lock). `drush updb` is throwing:
The only thing I could think of is that it was set to -dev recently, but that is not the case.
When I ssh into production and run `drush sql-query "DESCRIBE migrate_tools_sync_source_ids"`, it complains that the table does not exist. When I run it locally, it does.
drush @alias.prod sql-query "DESCRIBE migrate_tools_sync_source_ids"
drush sql-sync @alias.prod @self -y
drush sql-query "DESCRIBE migrate_tools_sync_source_ids"
Comment #35
scott_euser commentedYou could have had the patch installed ages ago too, it existed over a year ago I believe.
Anyways probably you could open a follow-up issue to add a check if table not exists into the update hook.
Comment #36
loopy1492 commentedI was able to sync the database from one environment to the other upstream and get a successful updb. I'm not entirely sure what the deal is with syncing down to local, but I'm guessing it's something with my provider or my computer. Or possibly disparate MySQL versions, though that's a pie in the sky guess. Thanks for your response.
Comment #37
mjpa commentedI came across this issue after upgrading to 10.4 and looking into the state cache flag.
Our state values are all relatively small, except
migrate_tools_syncthat's pretty large.We've updated the module and ran updates etc which has created the new table - all fine there.
However, should the old state entry still exist? Should it not be removed as part of the update hook?
I can't find any code leftover that would remove it so it looks like it'll persist forever unless we delete it ourselves.
Comment #38
heddnCan you open a follow-up to remove the old state entry? We should get rid of it.
Comment #39
mjpa commentedYep, no problem. I've created #3497145: Remove the old migrate_tools_sync state entry