From 7ca4eaaa4425f9b6f538b2b02959ddacd4601004 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 28 Oct 2026 11:31:57 -0400 Subject: [PATCH] perf: drop row locks from bulk criterion updates bulk_update_competency_criteria() locked the group lineage and the criteria it edits. The lineage includes the competency's root group, shared by every course that uses the competency, so one bulk update serialized every other bulk update for that competency and, through InnoDB's shared lock on foreign-key parents, stalled criterion inserts under it in any course. The locks guarded against a concurrent archive, and no code path archives groups or criteria. Without them, each extra identical concurrent request writes one redundant history row. The saves run in primary-key order, so two overlapping requests cannot deadlock on each other's criterion rows. The tag and course-run locks in associate_competency_criterion() stay: they protect one-root-per-tag, duplicate criteria, and the per-course dominance check, none of which a database constraint can express. Co-Authored-By: Claude Fable 5.1 --- src/openedx_learning/applets/cbe/api.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index abe29ebf8..8b0d2a0d8 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -642,10 +642,9 @@ def bulk_update_competency_criteria( # Archiving a group doesn't archive its criteria, yet ADR 0002 Decision 2 closes them to authoring. # An archived ancestor counts too: it stands in for a delete that would have cascaded to this leaf. - # Locked for the same reason as the criteria below. lineage = [group.id, group.parent.id, group.parent.parent_id] archived_by_id = dict( - CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage).values_list("pk", "archived") + CompetencyCriteriaGroup.objects.filter(pk__in=lineage).values_list("pk", "archived") ) archived_group_id = next((pk for pk in lineage if archived_by_id.get(pk)), None) if archived_group_id is not None: @@ -655,9 +654,12 @@ def bulk_update_competency_criteria( ) ) - # Locked so a concurrent archive can't slip in after the archived check, and so a concurrent - # identical request waits and then sees nothing to change instead of writing a second history row. - criteria_by_id = CompetencyCriterion.objects.select_for_update().filter(group=group).in_bulk(criterion_ids) + # These reads take no row locks. The lineage includes the competency's root group, a row every + # course using the competency shares, so locking it would serialize every bulk update for the + # competency and block criterion inserts under it. An archive path added later owns its own + # locking. Each extra identical concurrent request costs one redundant history row, since + # _apply_rule_source() compares before saving. + criteria_by_id = CompetencyCriterion.objects.filter(group=group).in_bulk(criterion_ids) missing_ids = [criterion_id for criterion_id in criterion_ids if criterion_id not in criteria_by_id] if missing_ids: raise CompetencyCriterion.DoesNotExist( @@ -679,7 +681,9 @@ def bulk_update_competency_criteria( # every criterion in a leaf shares the group's tag, so they all resolve to the same profile. applicable = _get_applicable_rule_profile(group.tag.taxonomy.id) if rule_profile_id is None else None criteria = [criteria_by_id[criterion_id] for criterion_id in criterion_ids] - for criterion in criteria: + # Saves run in primary-key order. Each save() takes an exclusive row lock, so two overlapping + # requests saving in their own request order could each wait on the other's row. + for criterion in sorted(criteria, key=lambda item: item.pk): _apply_rule_source( criterion, rule_profile_id,