Skip to content

Migrations: Optimise ConvertLocalLinks migration to process data in pages, to avoid having to load all property data into memory#21003

Merged
Zeegaan merged 4 commits intomainfrom
v17/improvement/optimize-convert-local-links-migration
Dec 2, 2025
Merged

Migrations: Optimise ConvertLocalLinks migration to process data in pages, to avoid having to load all property data into memory#21003
Zeegaan merged 4 commits intomainfrom
v17/improvement/optimize-convert-local-links-migration

Conversation

@AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Nov 29, 2025

Prerequisites

  • I have added steps to test this contribution in the description below

This PR comes follows and escalated internal support issue where very large databases would fail to migrate beyond 15 due to the migration step that converts local links to the new format.

Description

I've replicated locally with a database copy and found the problem lies with attempting to load all the potentially affected property data into memory, at which point I'd get stack overflow and memory related exceptions.

I've modified the migration to process property data in pages of 10000, after which it as able to complete.

In the same test I also found a failure in migration the system dates for the umbracoContentVersionCultureVariation table with a similar amount of records, but that was resolved with a reasonable update to the connection string, applying ;Connect Timeout=300.

Running locally, with a test database containing 1.4 million rich text property data records, the whole migration from 13, which includes a few other long-running steps, completes in 12 minutes.

In order to mitigate any concern that this refactor affects the functionality of the migration, I've ensured a rich text property data value of ... <a href=\"/{localLink:umb://document/8729b1c6e11a48ff909fa30b9b2ad74f}\" title=\"Test\">Link to page</a> ... is successfully migrated to ... <a href=\"/{localLink:8729b1c6-e11a-48ff-909f-a30b9b2ad74f}\" type=\"document\" title=\"Test\"\>Link to page\</a> ....

Testing

Prepare a database from Umbraco 13 containing some data in rich text fields that include links to other content items (which will be added to the rich text markup as "local links").

Connect the database to an Umbraco 17 code base to migrate and verify the migration completes and the local links work as expected after the upgrade.

Not from this PR's changes - it was noted also in thttps://github.com//pull/20887 - but I see the resulting output has the <, > and " characters encoded, but this doesn't seem to cause any problems in practice. When firing up the site after the migration, the local links are handled as expected in the backoffice.

If you want to check with the actual test database I've been using and was provided as an example of the problem, it's available on an internal Slack thread.

…id having to load all property data into memory.
Copilot AI review requested due to automatic review settings November 29, 2025 09:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes the ConvertLocalLinks database migration to handle large-scale databases by implementing pagination. The migration previously attempted to load all property data into memory at once, causing out-of-memory exceptions on databases with millions of records. The new approach processes data in pages of 10,000 records at a time, enabling successful migration even for very large databases (tested with 1.4 million records).

Key Changes:

  • Pagination logic processes property data in pages of 10,000 records instead of loading all records into memory
  • Enhanced logging to track progress through pages and provide better visibility during long-running migrations
  • Improved documentation with XML comments explaining the migration's purpose and linking to relevant GitHub PRs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Member

@Zeegaan Zeegaan left a comment

Choose a reason for hiding this comment

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

Looks fine to me, and it works locally aswell 💪

/// <summary>
/// Initializes a new instance of the <see cref="ConvertLocalLinks"/> class.
/// </summary>
[Obsolete("Please use the constructor taking all parameters. Scheduled for removal in Umbraco 18.")]
Copy link
Member

Choose a reason for hiding this comment

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

Okay this is very nitpicky so feel free to ignore. I mean yes we could remove this in 18, but every migration prior to 17 will be removed in the next major version 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just reworded it to make that point clear. Left it still with an Obsolete attribute just to help indicate which constructor we expect callers to use.

@Zeegaan Zeegaan merged commit 742de79 into main Dec 2, 2025
25 of 26 checks passed
@Zeegaan Zeegaan deleted the v17/improvement/optimize-convert-local-links-migration branch December 2, 2025 01:09
Zeegaan pushed a commit that referenced this pull request Dec 2, 2025
… pages, to avoid having to load all property data into memory (#21003)

* Optimize ConvertLocalLinks migration to process data in pages, to avoid having to load all property data into memory.

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Updated obsoletion warning.

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
(cherry picked from commit 742de79)
@Zeegaan
Copy link
Member

Zeegaan commented Dec 2, 2025

Cherry picked for 17.0.1 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants