Skip to content

fix(migration): bound memory when migrating form answers to items_ids - #26249

Merged
trasher merged 3 commits into
12.0/bugfixesfrom
fix/12.0-migration-answerssets-oom
Nov 6, 2026
Merged

trasher merged 3 commits into
12.0/bugfixesfrom
fix/12.0-migration-answerssets-oom

Conversation

@orthagh

@orthagh orthagh commented Nov 6, 2026

Copy link
Copy Markdown
Member

The 11.0 → 12.0 migration loaded glpi_forms_answerssets in batches of 250 rows including answers; with multi-MB payloads this exhausts the default 128M memory_limit on real databases.
Ids are now fetched in batches and each payload is loaded on its own, so peak memory depends on the largest row (126 MB → 80 MB on a support DB copy, ~4% slower).
Covered by the existing QuestionTypeItemItemsIdsMigrationTest.

The 11.0 -> 12.0 migration fetched glpi_forms_answerssets rows in batches
of 250 including their `answers` payload. A single payload can weigh
several megabytes (inlined uploads), so one batch exhausted the default
128M memory_limit on real databases.

Fetch only ids in batches, then load and migrate each payload on its own,
so the peak memory depends on the largest row rather than on the batch.
@orthagh
orthagh requested a review from ccailly November 6, 2026 08:31
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Code Review by Qodo


Resolved findings
1. The memory fix has no regression test ✓ Resolved
Description
The new ID-batching path changes how answer payloads are loaded, but this PR adds no test that fails
under the former 250-payload query. The existing migration test rewrites and checks one answer-set
row, so it passes with either loading strategy and cannot catch a return of the reported memory
failure.

@trasher trasher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could maybe add a test with more than 250 entries to check the "pagination" works? Not a blocker.

@orthagh

orthagh commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

Added testAnswerMigrationProcessesSeveralBatches (260 answer sets, batch size is 250); it fails if the loop stops after the first batch.

@trasher
trasher merged commit 96af3bc into 12.0/bugfixes Nov 6, 2026
26 checks passed
@trasher
trasher deleted the fix/12.0-migration-answerssets-oom branch November 6, 2026 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants