Repository navigation
perf(deletion-retention): scope purge-audit repeat checks with legacy database fallback - #44262
Conversation
…er entity set `_repeats_an_earlier_block` was an EXISTS nested inside an EXISTS, correlated per candidate row: O(history²) per candidate, evaluated under the pruning coordination lock by the batch re-check. On a 6,000-row multi-reason entity a 500-id batch held the lock 6.4 s on PostgreSQL and ~50 s on MySQL 8 (REPEATABLE READ), blocking every concurrent `write_ahead()` for that long. Reformulate the predicate as an uncorrelated derived-table select and test the row with `id IN (...)`: the immediately preceding distinct blocked timestamp P and its reason statistics come from LAG() over a per-(entity, timestamp) group table, and a row repeats iff P exists, P is in the current streak, and both timestamp groups hold solely the row's reason (NULL-safe) — the same rule as before (proof sketch in the docstring), including the inclusive tie bounds and the force-row exemption. All existing behavioural tests are unchanged; a 40-seed randomised equivalence test keeps the legacy nested-EXISTS shape in the test module and asserts identical candidate sets for all three categories. Under the lock the derived tables are scoped to the batch's entities by literal IN-lists of the ids' distinct (entity_type, entity_uuid) values, read under the lock right after acquisition: scoping only, over immutable columns, with every candidacy predicate still re-verified in SQL over the fresh locked snapshot (sc-118200). Literal values are deliberate — a scope JOIN or IN-subquery collapsed PostgreSQL's cardinality estimates into nested loops (5.4 s), and a subquery scope was catastrophic on MySQL. Lock-hold at batch 500 / 50 on the measurement seed (medians): PostgreSQL 6,423 → 75 ms / 149 → 29 ms; MySQL 8 RR ≈49,700 → 160 ms / 1,243 → 131 ms; unlocked discovery 1.4 s/97 s → 60 ms/332 ms. MySQL's remaining cost is five sequential window passes for the five LAG columns (SQLAlchemy cannot emit a named WINDOW); a single-pass variant measured ~90 ms and is tracked separately. Evidence, plans and harness: superset-spec/reviews/2026-09-14-pr-43490/lock-hold-evidence/ (sc-120493/). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
Code Review Agent Run #14a119Actionable Suggestions - 0Additional Suggestions - 3
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44262 +/- ##
==========================================
+ Coverage 80.40% 80.60% +0.20%
==========================================
Files 2932 2941 +9
Lines 174525 176008 +1483
Branches 40548 40826 +278
==========================================
+ Hits 140319 141864 +1545
+ Misses 31537 31480 -57
+ Partials 2669 2664 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aminghadersohi
left a comment
There was a problem hiding this comment.
Differential-tested old vs new at category level across ~2,700 fixtures: equivalent, including the inclusive-bounds tie rule, NULL/mixed-NULL groups and the scope cross-product. Main concerns are the title's O(batch) claim and lag() against a metadata DB documented as supporting MySQL 5.7.
| sa.select( | ||
| groups, | ||
| *[ | ||
| sa.func.lag(groups.c[name]) |
There was a problem hiding this comment.
lag() is the only window function in the metadata-DB query surface (master has none), yet configuring-superset.mdx:287 still lists MySQL 5.7, which has no window functions. CI only covers mysql:8.0, and pruning runs by default, so a 5.7 metadata DB would fail at runtime.
There was a problem hiding this comment.
Good catch on the documented MySQL 5.7 compatibility gap; could we preserve the non-window path for that backend? Pruning does default to disabled (PURGE_AUDIT_PRUNING_ENABLED = False), so this affects deployments that enable it rather than all default deployments.
There was a problem hiding this comment.
Addressed in 6d41ae4: dispatch retains the legacy predicate for MySQL below 8.0 and older/unknown MySQL-family versions, including normalized MariaDB detection. Both paths have candidate gates and differential coverage. Fresh tests: 147 unit; 34 integration plus one skip on SQLite; all 35 on PostgreSQL 17. No live MySQL 5.7 or MariaDB execution in this round, so that validation remains a stated limitation. Pruning still defaults off.
| not a time bound, which the batch-size setting trades against drain speed. | ||
| literal surviving ids. The repeat check computes timestamp groups for the | ||
| batch's entities in uncorrelated window tables; the other guards use | ||
| correlated index probes. Lock-hold depends on the batch's entity histories, |
There was a problem hiding this comment.
This line is accurate, but the PR title's "lock-hold O(batch) on every backend" contradicts it. Holding batch at 50 and raising history depth 200->12800 (64x), the locked re-check went 1.97ms -> 109ms (55x): the group and LAG tables scan each scoped entity's full history, unfiltered by batch id.
There was a problem hiding this comment.
Good catch—the source already describes history-dependent lock-hold, and the title overstates the guarantee. Would it be worth dropping the O(batch) claim and describing the candidate-entity scoping instead?
There was a problem hiding this comment.
Addressed with 6d41ae4 and the updated PR title/body. Removed the O(batch) guarantee and stale 500-row/short-indexed-window guidance. The default is 50, maximum 100, and lock time remains dependent on history, plan and backend; no production capacity claim is inferred from synthetic measurements.
| *[ | ||
| sa.func.lag(groups.c[name]) | ||
| .over( | ||
| partition_by=(groups.c.entity_type, groups.c.entity_uuid), |
There was a problem hiding this comment.
Load-bearing but unpinned: dropping entity_type from this partition leaves all 66 unit + 34 integration tests green, while making a row take its predecessor from a different entity_type sharing the uuid and be deleted as a repeat. The equivalence test seeds a single entity_type.
There was a problem hiding this comment.
Good catch—the partition is correct, but the fixtures do not pin the shared-UUID/different-type case. Could we add an executable case that preserves each type’s first block and fails when entity_type is removed from the partition?
There was a problem hiding this comment.
Addressed in 6d41ae4: a shared-UUID/different-entity-type fixture exercises both repeat implementations, alongside the expanded equivalence cases. The partition expression remains unchanged. Fresh affected suites passed, including all 35 integration tests on disposable PostgreSQL 17.
| table: sa.FromClause, | ||
| now: datetime, | ||
| cutoff: datetime, | ||
| scope_entities: Sequence[tuple[str, str | None]] | None = None, |
There was a problem hiding this comment.
scope_entities is never read in this body: compiled at MAX_BATCH_SIZE the evidence re-check binds 507 placeholders vs 3,520 for duplicate. _delete_batch still runs the scope lookup under the coordination lock for this category, so it is a locked round-trip with no consumer.
There was a problem hiding this comment.
Good catch—the evidence predicate does not consume the entity scope. Could we make the locked lookup category-specific while retaining evidence’s locked ID recheck and keeping scope lookup for duplicate and operational pruning?
There was a problem hiding this comment.
Addressed in 6d41ae4. Discovery carries immutable entity pairs before the lock; evidence pruning does not fetch or consume unused scope, while its locked ID candidacy recheck remains. Explicit recheck adapters preserve argument checking. Fresh tests and changed-file pre-commit passed.
| Keep the repeat-id query uncorrelated: sc-120493 measurements in | ||
| lock-hold-evidence/REPORT.md, Variant 2, showed MySQL repeatedly executing |
There was a problem hiding this comment.
lock-hold-evidence/REPORT.md does not exist in this repo — the PR body places it in a separate spec repo. A shipped docstring should not cite a path no maintainer can open.
| Keep the repeat-id query uncorrelated: sc-120493 measurements in | |
| lock-hold-evidence/REPORT.md, Variant 2, showed MySQL repeatedly executing | |
| Keep the repeat-id query uncorrelated: the sc-120493 Variant 2 | |
| measurements showed MySQL repeatedly executing |
There was a problem hiding this comment.
Good catch—the relative path is not present in this repository. Could we take your suggestion, which keeps the reason for the uncorrelated query without pointing maintainers at an unavailable local artifact?
There was a problem hiding this comment.
Addressed in 6d41ae4: removed the unavailable repository-relative report link and retained the reason for keeping the repeat-ID query uncorrelated. Final-snapshot independent review found no publication-blocking defects; the documented backend/capacity limitations remain.
…d work Address PR apache#44262 review threads PRRT_kwDOAlosUs6ic6j7 and PRRT_kwDOAlosUs6ic6kF: select the legacy non-window repeat query for metadata servers below MySQL 8.0, verify equivalence, and pin entity-type isolation when UUIDs collide. Address PRRT_kwDOAlosUs6ic6kJ by avoiding an unused evidence-category scope query under the coordination lock while preserving its candidacy recheck. Address PRRT_kwDOAlosUs6ic6kM by removing the unavailable local report path from the docstring. Leave the title correction in PRRT_kwDOAlosUs6ic6kA to the coordinator.
Address the SQLAlchemy and data-systems fix round for PR apache#44262 / sc-120493: use a conservative unknown-version fallback and normalized MariaDB capabilities, and make legacy repeat predicates self-filter candidates. Carry immutable identity from unlocked discovery instead of querying it under the coordination lock. Cap batches at 100 for the historical SQLite bind budget, remove redundant legacy scope binds, type the recheck seam, and log plan and batch outcomes. Pin the fixes with reproduced regressions, rich direct-path fixtures, and scoped differential coverage.
Address the PR apache#44262 independent guidance and Protocol/partial findings: retain the safe 100-row ceiling, remove contradictory tuning claims, and make recheck argument drift visible to mypy through explicit adapters. Clarify legacy and coordination-lock contracts without changing candidacy semantics.
Addresses the remaining independent review contradiction: retain the 100-row bound and describe workload-dependent lock time, not a promised index-backed short window. Clarify pending-aware guards without changing predicates.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
CI update for 6d41ae4: the current Docker setup job fails with multiple Alembic heads before tests: https://github.com/apache/superset/actions/runs/35159600323/job/105007164634 . Independent graph inspection of apache master at 7b6dd75 finds heads 0884f655c0a6 and e2f3a1b9c640; this PR changes no migrations. The local 147-unit/35-PostgreSQL integration results do not make CI green. Upstream reconciliation and current-head CI remain required. |
Code Review Agent Run #cd8d39Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
Prepared by Codex at Mike's request. Feedback accounting at Repaired-master PostgreSQL CI completes |
aminghadersohi
left a comment
There was a problem hiding this comment.
Re-checked all five round-1 findings by execution: all fixed, and the entity_type LAG partition is now killed by the new fixture. One residual coverage gap on the new legacy path, inline.
| "trigger": "scheduled", | ||
| "actor": "system", | ||
| "created_on": datetime(2026, 1, day), | ||
| "reason": "same", |
There was a problem hiding this comment.
Follow-on to the LAG-partition thread: the legacy path's twin conjunct between.entity_type == table.entity_type can be dropped with all 147 unit tests and the 40-seed equivalence test green — every row here shares one reason, so it is trivially satisfied. Varying reason by type kills it.
| "reason": "same", | |
| "reason": "same" if entity_type == "chart" else "other", |
There was a problem hiding this comment.
Good catch; addressed in e16fe62 by varying the reason by entity type as suggested. Removing the between.entity_type conjunct passes the old fixture but fails the amended fixture; restoration passes, independently reproduced in review. All 105 affected unit tests and mandatory branch-file hooks including MyPy passed; fresh CI is queued. Could you take another look? The adjacent earlier.entity_type discrimination case remains an explicitly recorded follow-up, not coverage claimed by this change.
There was a problem hiding this comment.
Confirmed at b472597d: dropping between.entity_type reds partition-reason-isolation, restoring it passes, and the 40-seed equivalence test is green both ways. The earlier.entity_type follow-up is covered too — predecessor-type-isolation kills it.
| connection.scalars( | ||
| sa.select(table.c.id).where( | ||
| table.c.id.in_(ids if scope_entities != [] else []), | ||
| predicate(table, datetime(2026, 2, 1)), |
There was a problem hiding this comment.
scope_entities is never passed to predicate(...), so None and the two-pair case run identically and [] only asserts an empty id list. The parameter reads as scoped/unscoped coverage it does not provide here.
There was a problem hiding this comment.
Good catch; addressed in e16fe62. The misleading parameterization is removed, and a separate fixture passes scope_entities directly into the window predicate with no outer ID filter: unscoped, two-pair, one-pair and empty scope yield 3/2/1/0 repeats. Forcing scope to None produces three failures; restoration passes, independently reproduced in review. Could you check that this captures the intended contract? The legacy path intentionally has no scope parameter; deliberate cross-product widening remains documented rather than newly pinned by this fixture.
There was a problem hiding this comment.
Confirmed. test_window_repeat_predicate_applies_entity_scope passes scope_entities straight into the window predicate and the four arms give 3/2/1/0 repeats; that is the contract. It is not scope-only either — dropping the LAG partition reds two of its arms.
Address review threads 4035027978 and 4035028005: vary the intervening entity reason so a missing legacy type guard fails, and exercise scope_entities inside the window predicate without masking it with an outer ID filter.
Code Review Agent Run #88ef33Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
Reviewed e16fe62468. No correctness defects found in the head; approving. One coverage suggestion that I think is worth landing before #44349 rewrites the predicate.
Small suggestion: pin the window path's boundary-join entity_type conjunct. The outerjoin(boundary, entity_type == ... AND entity_uuid == ...) in the LAG path is correct, but dropping its entity_type clause leaves test_repeat_paths_isolate_entity_types_and_agree and the 40-seed equivalence test green (executed against the head module on SQLite). A concrete input where the mutation deletes a survivor: (chart, X) blocked d5 "same", confirmed d6, blocked d7 "same", blocked d8 "same"; (dashboard, X) confirmed d1. Head yields {d8} on both paths; the mutated join yields {d7, d8} because d7 joins the dashboard's boundary row and the chart's "blocked since" survivor is pruned. Same class as the LAG-partition finding, one clause over. Could we add a cross-type confirmed streak breaker to the shared-UUID fixture so both the partition and the boundary join are killed? The legacy path's earlier.entity_type conjunct has the same unpinned status, which you already recorded as a follow-up.
Verified with no issues: predicate equivalence across current-group ties, mixed NULL/coded previous groups, boundary tie/between/equal cases, future rows, force as anchor/between/candidate, and pending/failed exclusion; the cross-lock hint only widens the window tables and a stale hint can only reduce deletions, never add one; batch config is fail-closed at 1..100 with invalid_config incremented rather than success; bind-count arithmetic stays under the 999 SQLite floor at the ceiling; the evidence recheck ignores scope and remains id-bound; MariaDB/MySQL version dispatch behaves correctly for 5.5.5-10.x-MariaDB, 10.1.x-MariaDB, 5.7.x-log, and 8.0.0-rc under the pinned SQLAlchemy parser.
Validation gaps, already disclosed in the PR: no live MySQL 5.7 or MariaDB execution. One operational note worth a line in the docs: dispatch trusts the banner from SELECT VERSION(), so a proxy advertising a synthetic 8.x version in front of a 5.7 backend would be routed to LAG and fail at runtime.
Answer Richard's review on apache#44262: distinguish boundary joins and legacy predecessors from same-UUID records of another type, so removing any entity_type guard fails the fixture. Document the server-banner trust assumption without changing purge behavior.
|
@richardfogaca Good catch—added the boundary-join counterexample and a same-reason cross-type predecessor case in b472597d95, retaining the original partition case. All three entity_type mutations are caught: removing the LAG partition guard produces 2 failed cases; removing the boundary-join guard or legacy earlier.entity_type guard each produces 1 failed case. Both unmodified paths pass all three cases; the affected unit suites pass 151 tests and branch-file hooks pass. Also documented that dispatch trusts SELECT VERSION(), including the synthetic-8.x proxy limitation. No production code changed; live MySQL 5.7/MariaDB validation remains unexercised here. |
Code Review Agent Run #052785Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
aminghadersohi
left a comment
There was a problem hiding this comment.
Verified by execution: all three entity_type mutations you name are killed, and the UPDATING.md SELECT VERSION() note is accurate. One guard of the same family is still unpinned, inline.
| ], | ||
| [2], | ||
| id="predecessor-type-isolation", | ||
| ), |
There was a problem hiding this comment.
Production is correct, but _streak_boundary_scalar's entity_type conjunct (prune_audit.py:325) survives all 151 unit tests and the 40-seed equivalence test; dropping it ages the streak's earliest "blocked since" row out via the operational category. This case reds it on the legacy path:
| ), | |
| ), | |
| pytest.param( | |
| [ | |
| ("chart", STATUS_BLOCKED, 5, "same"), | |
| ("chart", STATUS_BLOCKED, 7, "same"), | |
| ("dashboard", STATUS_CONFIRMED, 9, None), | |
| ], | |
| [1], | |
| id="foreign-newer-streak-breaker", | |
| ), |
There was a problem hiding this comment.
Addressed in 1e2982ea941e126924cb0a5fc9418e4259a69b65: the foreign-newer-streak-breaker case now pins the scalar boundary's entity_type guard, and the existing isolation cases are retained. Thanks for confirming the mutation is caught in your subsequent review.
rusackas
left a comment
There was a problem hiding this comment.
Thanks for sticking with this through ten rounds of @aminghadersohi's mutation testing, @mikebridge. This is about as thoroughly execution-proven as a deletion path gets.
Couple of things before I approve, though. _repeat_path() treats every dialect outside mysql/mariadb as window-capable, but SQLite only picked up LAG in 3.25 (2018), so an older bundled sqlite3 as the metadata DB would dispatch the window query and fail on every scheduled run. Same class of gap as the MySQL 5.7 case, just never discussed. Worth a version floor, or at least documenting it the same way?
Also, @aminghadersohi's _streak_boundary_scalar coverage thread from a couple days ago is still open, same bug class as the earlier LAG-partition finding, and they already handed you the test case.
| table: sa.Table = PurgeAuditLog.__table__ | ||
| return ( | ||
| sa.select(table.c.id) | ||
| sa.select(table.c.id, table.c.entity_type, table.c.entity_uuid) |
There was a problem hiding this comment.
_EvidenceRecheck.__call__ ignores scope entirely, so entity_type/entity_uuid here get fetched per candidate row for nothing.
| sa.select(table.c.id, table.c.entity_type, table.c.entity_uuid) | |
| sa.select(table.c.id) |
There was a problem hiding this comment.
Addressed in 1e2982ea941e126924cb0a5fc9418e4259a69b65: evidence discovery now selects only the audit ID, as suggested. The locked candidacy recheck and delete behavior are preserved. The final affected suite passed all 161 tests.
aminghadersohi
left a comment
There was a problem hiding this comment.
_streak_boundary_scalar's entity_type guard is killed by the new case — confirmed against a no-op control arm. Its entity_uuid twin at :326 is still unpinned by both suites; inline.
| ("dashboard", STATUS_CONFIRMED, 9, None), | ||
| ], | ||
| [1], | ||
| id="foreign-newer-streak-breaker", |
There was a problem hiding this comment.
Same function, other half (prune_audit.py:326): dropping boundary.entity_uuid == target.entity_uuid survives all 152 unit tests and the 40-seed equivalence test — two charts, different UUIDs, one confirmed newer, and the other's "blocked since" row ages out. Fixture hardcodes the UUID, no fence.
There was a problem hiding this comment.
Addressed in 1e2982ea941e126924cb0a5fc9418e4259a69b65: added the two-chart/different-UUID isolation case. Removing the boundary entity_uuid guard fails the new case; the unmodified control passes. Thanks for independently confirming that this arm closes the gap in your latest review.
|
@aminghadersohi @rusackas the remaining review fixes are published in
Fresh local validation: 161 deletion-retention tests and all applicable full-branch-file hooks pass. Independent Astra final review approved the exact published tree, including its own SQL/mutation control. Old-SQLite/MySQL/MariaDB runtime and capacity measurements remain disclosed limits; mocked version boundaries aren't live old-engine evidence. New-head CI is running. Could you take a final pass on these fixes and re-approve if satisfied? No retention defaults or feature enablement changed. Thanks! |
Code Review Agent Run #53598eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
aminghadersohi
left a comment
There was a problem hiding this comment.
r4's entity_uuid gap in _streak_boundary_scalar is closed — the new entity_uuid arm is the sole killer, against a green no-op control. This commit's two production changes are pinned too: 4/4 mutants on them red a test.
|
Comment follow-up at For completeness, acknowledging Bito's older duplicated-test-setup suggestion: #44262 (comment) . The tests still use separate setup; that cleanup has not been implemented and is not being claimed fixed. The earlier boundary-logic duplication suggestion was already recorded as a structural follow-up: #44262 (comment) . Thanks @aminghadersohi for the latest execution check confirming the UUID gap is closed and the production-change mutants are caught. Human threads are left for their reviewers to resolve; final approval is still outstanding. |
rusackas
left a comment
There was a problem hiding this comment.
Confirmed, the evidence discovery select is down to just the id column now, matches what _EvidenceRecheck already assumed.
@aminghadersohi's mutation-testing pass on this one was something else, catching load-bearing conjuncts that every existing test let through. Nice touch parametrizing the isolation cases over both entity_type and entity_uuid instead of hand-rolling separate fixtures for each.
CI's green, bito's clean across every push. LGTM!
SUMMARY
Follow-up to #43490's coordination-lock-hold discussion (sc-120493, follow-up sc-120950). Scope purge-audit repeat checks to the candidate entities and retain a non-window fallback for supported older MySQL-family servers.
LAG, partitioned by both entity type and UUID. MySQL below 8.0, MariaDB below 10.2, and unknown MySQL-family versions use the conservative legacy predicate. MariaDB detection uses the dialect's normalized version rather than the replication-compatibility prefix.(id, entity_type, entity_uuid)across the coordination-lock boundary as hints. The locked recheck still verifies candidacy against current committed state before deleting by ID. Evidence pruning does not fetch unused entity scope. Explicit adapters preserve the recheck call contract.invalid_config, not only the backlog gauge. Pruning remains disabled by default.There is no O(batch) lock-hold guarantee: cost depends on each scoped entity's history, plan and backend. Prior benchmark numbers are not presented as measurements of this final snapshot. Production capacity adequacy remains unestablished; this change does not introduce a tuning-control expansion.
#44349 is a separately owned follow-up built on an earlier version of this branch. Its named-window optimization is not included here; it needs reconciliation and verification against the updated predicate/fallback contracts.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend pruning query, compatibility and operational guidance.
TESTING INSTRUCTIONS
Fresh local verification: 147 unit tests passed; 34 integration tests passed and one repeatable-read test skipped on SQLite; all 35 integration tests passed on a disposable PostgreSQL 17 server, including the concurrent-state recheck. Both predicate paths are exercised by differential tests. Changed-file pre-commit passed. Independent final-snapshot Claude review found no publication-blocking defects, retaining the previously recorded nonblocking follow-ups.
No live MySQL 5.7 or MariaDB execution in this verification round; SQLite execution and dialect/version tests are not substitutes for those servers. No production volume, arrival-rate or latency assumptions are inferred from synthetic tests.
ADDITIONAL INFORMATION