perf(versioning): defer capture policy until versioned work

[mikebridge]

SUMMARY

Core consulted the host's VERSIONING_CAPTURE_PREDICATE on every flush and commit, whether or not the unit of work touched a versioned model. Writes that have nothing to do with versioning (SQL Lab query status rows, user upserts, permalinks, report logs) therefore paid for the host's policy lookup. For a multi-tenant host, that lookup can mean remote calls.

The predicate is now consulted only once a flush or commit holds versioned work: a pending versioned instance (new, dirty or deleted) or retained pre-flush state. The decision is still made once per transaction and then frozen. The baseline listener, the change listener and finalize_change_records all read that one decision, so a later flush in the same transaction cannot see a different answer. A savepoint rollback keeps the outer decision.

Behavior

• History rows: unchanged. The same *_version, version_transaction and version_changes rows are written as before, with the predicate allowing or denying.
• A predicate that raises: before, it failed every flush, including SQL Lab and log writes. Now it fails only flushes that carry versioned work. The error still propagates there and is not turned into a silent denial.
• A predicate whose answer changes mid-transaction: this is the one case where output differs. Master could write a *_version row with no matching version_changes rows. This change captures both or neither.
• The superset.versioning.capture.finalize.latency metric: emitted only for commits that capture versioned work. Unrelated commits no longer emit a sample. This keeps the percentile series focused on capture cost. A monitor that uses this metric's sample count as a commit-throughput heartbeat would see fewer samples.

Performance scope

The win applies to hosts that install a VERSIONING_CAPTURE_PREDICATE. Measured with a counting predicate in the unit tests:

Sequence	Predicate calls before	After
Two non-versioned flushes plus an empty commit	7	0
A versioned delete and commit	4	1

With the OSS default (no predicate), there is no host lookup to defer. The pending-object scan that replaces it costs a few microseconds more per flush for large unrelated flushes. That cost was not measurable at the commit level, and the scan stops at the first versioned object.

Out of scope

• A predicate is None fast path for the OSS default. It trades a config lookup for a scan that grows with the pending set, so it is a separate change with its own measurements.
• The read path (api_helpers). It has no pending write to key on and is a separate optimization.
• The savepoint bug in which a versioned insert rolled back inside a savepoint broke the next versioned insert in the same outer transaction. It is fixed on master by fix(versioning): preserve history across savepoint rollback #45033, which this branch includes. The two tests that covered it here now pass without xfail markers.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A (backend).

TESTING INSTRUCTIONS

pytest tests/unit_tests/versioning gives 443 passed, with no xfails.

The new tests in test_runtime_capture.py assert both predicate call counts and history row counts. They cover:

• non-versioned-first and versioned-first transactions, with the predicate allowing and denying
• one frozen decision across mixed flushes through finalization
• savepoint commit and rollback, followed by query autoflush
• hard and soft deletes
• a raising predicate
• a save after a rolled-back savepoint

In test_listener.py, the latency test asserts that a non-versioned commit emits no finalize.latency sample.

Copied onto the merge base, the new tests fail at their predicate-call assertions, not at history-row assertions. That matches a change meant to leave the captured rows alone. Reviewers also compared the history rows written before and after this change across 51 write shapes (on in-memory SQLite) and found them identical. The shapes included many-to-many edits from both sides, child-only edits, cascade and bulk deletes, merge(), autoflush, nested savepoints, and unrelated work ordered ahead of versioned work.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
• Introduces new feature or API
• Removes existing feature or API

🤖 Generated with Claude Code

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	3ea8bff
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ac7dc9a43f7880008a52df1
😎 Deploy Preview	https://deploy-preview-44928--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.64%. Comparing base (bdbddb7) to head (3ea8bff).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #44928   +/-   ##
=======================================
  Coverage   82.64%   82.64%           
=======================================
  Files        3007     3007           
  Lines      187305   187325   +20     
  Branches    43398    43402    +4     
=======================================
+ Hits       154797   154814   +17     
- Misses      29742    29744    +2     
- Partials     2766     2767    +1     

Flag	Coverage Δ
hive	36.15% <62.06%> (-0.04%)	⬇️
mysql	55.12% <100.00%> (+0.01%)	⬆️
postgres	55.13% <100.00%> (+0.01%)	⬆️
presto	38.06% <100.00%> (+0.02%)	⬆️
python	86.45% <100.00%> (-0.01%)	⬇️
sqlite	54.86% <100.00%> (+0.01%)	⬆️
unit	79.63% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codeant-ai-for-open-source]

CodeAnt PR Risk: Low Risk

• The PR appears safe to merge; capture policy is now checked only when versioned work is pending.
• The added tests cover unrelated writes, mixed flushes, savepoints, deletes, and policy failures.

Assessed commit: 3ea8bff9d70c

[rebenitez1802]

Request changes: the performance change is correct, cleanly designed, and well-tested — but an internal tracker ID committed in two test reason= strings must be scrubbed before this lands in public OSS.

🔴 High (blocking — OSS hygiene, not a runtime bug) — internal tracker ID committed to the public repo
The two xfail(strict=True) markers in tests/unit_tests/versioning/test_runtime_capture.py (~L466 on test_lazy_capture_savepoint_and_query_autoflush's (True, True) param, and ~L548 on test_capture_after_savepoint_rollback_with_stable_policy) carry a reason= string that embeds an internal Shortcut-style tracker ID (the SC-… prefix). Please drop the ID — replace both with a public GitHub issue link or neutral prose, e.g. reason="inherited SQLAlchemy-Continuum savepoint-rollback bug on nested-rollback-then-reuse".

🟢 Low — baseline listener now couples child capture to the child being versioned
In superset/versioning/baseline/listener.py, the gate calls capture_for_write → _has_versioned_work before force_parent_dirty_on_child_change, so a child-only edit passes the gate only because the dirty child is itself __versioned__. Safe today (TableColumn/SqlMetric both declare it), but a future child added to child_to_parent_registry that isn't versioned would silently skip baseline capture where master captured via the forced-dirty parent. Consider documenting the "every registered child must be Continuum-versioned" invariant at the registry.

🟢 Low — soften a claim in the PR description
"No application code path on master performs that sequence" isn't strictly true — the importer and cascade-purge paths do use begin_nested() with versioned entities in the session. Neither matches the xfail's exact rollback-then-reuse sequence (the importer rolls back only non-versioned Tag/TaggedObject; purge raises before any versioned write), so it's a pre-existing Continuum risk rather than a new break. Suggest rewording to "the savepoint-rollback paths that exist only roll back non-versioned work."

🟢 Low — observability: finalize-latency samples drop on non-versioned commits
finalize.latency is no longer emitted for commits with no versioned work (intended, and documented in metrics.py). This improves the percentile alerts it's meant for; the only thing that could false-fire is a sample-count/rate monitor used as a commit-throughput heartbeat on superset.versioning.capture.finalize.latency. Worth a one-line heads-up to operators (optionally UPDATING.md).

Otherwise this looks solid. No capture-loss path: the lazy guard is live (an empty Operations is falsy and has_changes is a property), and _has_versioned_work is a strict superset of Continuum's own is_session_modified, so it can't under-detect — M2M/membership edits are caught via the dirtied versioned parent, and one-to-many children are themselves versioned. "Decide once, freeze" also closes a latent torn-decision window the old per-listener capture_enabled calls allowed. Nice work.

[gabotorresruiz]

Hey Mike, no further blockers from me. Rafael's review already carries the blocking item and I am deliberately not restating it; what I went after is the one thing his review reasons about rather than tests, which is whether deferring the policy can lose a capture.

I ran 161e928d and the merge base 5a2f4c8e side by side through 51 write shapes on the real baseline, Continuum and change listeners, dumping every *_version, version_transaction and version_changes row plus the live tables after each one: many to many edits from both sides, a raw association insert, child only add/edit/delete, metric only edits, cascade delete, bulk update()/delete(), bulk_save_objects, flush(objects=...), mutations injected from inside before_flush and after_flush, merge(), autoflush during a query, two savepoint levels, the importer shaped savepoint that rolls back only non versioned rows, and every ordering of unrelated work ahead of versioned work. All 51 came out identical on both sides, so I could not construct a capture loss path. The only sequences that do diverge are the ones where the host answer flips inside one transaction, and there this branch is the coherent side: master wrote a dashboards_version row with zero matching version_changes rows, while this branch captures both or neither.

I also reproduced both rows of your table exactly (7 to 0 and 4 to 1), and a 20 row SQL Lab style sequence goes 42 predicate calls to 0; the versioned path drops from 4 calls per commit to 1 and was not measurably slower over 200 commits. Copying the new tests onto 5a2f4c8e, 15 of 16 fail there, every one of them at a predicate.assert_not_called() or assert_called_once_with line rather than at a history row assertion, which matches a change that is meant to leave the captured rows alone. Both savepoint sequences also fail identically on 5a2f4c8e, same OperationalError and same rows, so the CodeAnt risk note is about the inherited bug and not about this diff.

CI is green on this head. Not approving while Rafael's change request stands. Everything I ran was sqlite in memory, so the Postgres and MySQL savepoint and association paths here rest on CI only.

[gabotorresruiz]

Not a blocker, more a scoping question. With VERSIONING_CAPTURE_PREDICATE unset, which is the OSS default, capture_enabled is three config.get calls, so there is no host lookup left to defer and _has_versioned_work ends up the more expensive of the two: measured per call on sqlite, capture_enabled is flat at about 0.9us while this helper is about 0.9us with 1 pending object, 4.7us at 50, 38us at 500 and 152us at 2000, and it runs 4 times per unrelated flush plus once at the commit. I could not see it at the commit level (a 2000 row unrelated commit measured 61 to 68ms here against 64 to 67ms on 5a2f4c8e), and any() short circuits on the first versioned object so the versioned path is untouched. Is the intent that the win only lands for hosts that install a policy, or would a predicate is None fast path ahead of the scan be worth adding?

[mikebridge]

Thanks for the detailed comparison. On this tree the performance target is hosts with an installed predicate: when it's None, capture_enabled does no host lookup, while _has_versioned_work still scans pending objects. Could we treat a default-policy fast path as a separate, measured optimization, with tests that preserve deferred decisions and finalizer sample semantics, rather than imply an OSS-default speedup here? The hygiene follow-up is in d2d69c3632.

[gabotorresruiz]

That works for me. Agreed the win here is the installed predicate case, and a predicate is None fast path is its own change with its own numbers, since it trades a config lookup for a scan that grows with the pending set. Nothing further from me on this thread.

I also checked d2d69c3632: both reason strings lost the id and strict=True plus raises=sa.exc.OperationalError are untouched, so the xfails still fail the suite if they ever start passing.

[mikebridge]

Thanks for checking. Agreed on keeping the default-policy fast path as a separate, measured optimization. One update since that snapshot: 3ea8bff9d7 brings in master's #45033 savepoint fix, so the two former strict-xfail cases now pass and their markers were removed. An independent control confirmed they fail on #45033's parent and pass on the merge.

[mikebridge]

@rebenitez1802 thanks for catching the tracker IDs. Both xfail reasons now use neutral prose in d2d69c3632, and a case-insensitive scan of every added line in the PR finds no internal tracker IDs or Shortcut links. The strict xfail behavior is unchanged. Would you mind taking another look when you have a moment?

[fitzee]

Verdict: no blockers. @rebenitez1802's blocking request is addressed. Remaining items are non-blocking (stale PR description, merge-time hygiene, one nit).

Reviewed head 3ea8bff9d7. pytest tests/unit_tests/versioning passes locally on this head: 443 passed, 0 xfailed (sqlite only). CI is green.

Rafael's requested changes

Item	Status
🔴 Tracker ID in the two xfail reason= strings	Addressed. d2d69c3632 rewrote both reasons in neutral prose, and 3ea8bff9d7 then dropped both markers, because #45033 fixes the savepoint bug they tracked. git diff bdbddb76dc..3ea8bff9d7 has no SC-\d+, shortcut, jira or preset.io matches in code, comments or tests. The PR title and body are clean too.
🟢 Document "every registered child must be Continuum-versioned"	Not done. Nothing changed in superset/versioning/baseline/. Optional.
🟢 Soften the "no application code path…" claim	Not done, though the xfails it described are gone now (see below).
🟢 Heads-up that finalize.latency samples drop on unrelated commits	Covered only in the metrics.py:69-71 docstring. There is no UPDATING.md line. Optional.

Non-blocking

1. The PR description no longer matches the head. It still says "415 passed, 2 xfailed" and has a "Two strict xfails…marked xfail(strict=True)" section. Since the #45033 merge, test_capture_after_savepoint_rollback_with_stable_policy and the (True, True) case of test_lazy_capture_savepoint_and_query_autoflush pass without markers, and the suite is 443/0. Please update the TESTING section, which also resolves Rafael's wording point. A smaller point: "With the predicate off or raising, behaviour is unchanged" is not quite right for the raising case. A raising predicate used to fail every flush, including SQL Lab and log writes. Now it only fails flushes that carry versioned work. That is an improvement, but the description should say so.

2. Tracker IDs are still in two commit messages (a199a7728d, 47b79b2b8e), both merge commits, and the branch name. The repo only allows squash merges (.asf.yaml), so none of this reaches master as long as the squash body is trimmed at merge. Whoever merges should check that the squash message does not include the commit list.

3. Nit: capture_for_write can create a Continuum unit when capture is off. At superset/versioning/unit_of_work.py:49-52, when no unit exists yet and versioned work is pending, the function calls versioning_manager.unit_of_work(session) (which opens a connection and writes into session_connection_map / units_of_work) before capture_enabled() checks ENABLE_VERSIONING_CAPTURE. The baseline listener guards on options["versioning"] first (baseline/listener.py:105). capture_initial_states (changes/listener.py:582) and finalize_change_records (changes/listener.py:451) do not. On master, capture_enabled() returned before touching Continuum state.

   • On a normal OFF boot this cannot happen, because those listeners are never registered.
   • It can happen after an ON→OFF re-init in one process (multi-app or test reentrancy). That path also detaches Continuum's after_commit / after_rollback / engine rollback clear hooks, so each such unit stays in the process-global dict.
   • Fix: check versioning_manager.options["versioning"] at the top of capture_for_write, or check it before line 52.

Checked, no issue found

• Can a versioned write skip the policy? No. _has_versioned_work (unit_of_work.py:36-40) covers new, dirty and deleted plus retained initial state. That is a superset of Continuum's is_session_modified, and both installed plugins (VersioningFlaskPlugin, SkipUnmodifiedPlugin) leave is_session_modified at the base False. In capture_if_needed (unit_of_work.py:86-96), has_changes also covers ops tracked during the flush and association pending_statements. process_after_flush runs while new/dirty/deleted still hold their pre-flush contents, so it agrees with the before-flush answer.
• Commit path. For a commit with no earlier flush (pending delete or insert, then commit()), the first call is finalize_change_records → capture_for_write, which creates the unit and decides. test_lazy_capture_delete_policy covers this.
• Lifetime of the frozen decision. _capture_allowed lives on the connection-keyed unit and is cleared in reset() (unit_of_work.py:75-78) through Continuum's clear on commit/rollback and clear_connection on engine rollback. Nothing moves to the thread-local, so nothing carries over to the next request or Celery task on a reused scoped session. Savepoint rollback keeps the decision (restore_checkpoint, :141-147), which matches "decide once per transaction", and test_lazy_capture_savepoint_and_query_autoflush asserts it.
• Exceptions. If the predicate raises in finalize_change_records, it does so before _FINALIZING_KEY is set (changes/listener.py:451-454), so there is no stuck re-entrancy flag. If it raises in before/after flush, the flush aborts and SQLAlchemy rolls back the subtransaction.
• Performance. The claim holds when a predicate is installed. With the OSS default (predicate is None) the pending-object scan costs a little more than capture_enabled. That was discussed with @gabotorresruiz and deferred as a separate change, which seems reasonable.
• Tests. The new cases assert predicate call counts and history row counts. They cover non-versioned-first and versioned-first transactions, savepoint commit and rollback, query autoflush, hard and soft delete, and a raising predicate. The changes to test_listener.py (patching capture_for_write=True) keep the timing and lookup-failure tests on the path that captures.

Approving. Please update the PR description (1) before merge; 2 and 3 can be handled at merge or as a follow-up.

[mikebridge]

@fitzee thanks for the review. The description is refreshed for 3ea8bff9d7. The TESTING section is now 443 passed with no xfails; the savepoint cases are covered by #45033. The raising-predicate behavior is described as it now works: only flushes with versioned work fail. The description also covers the finalize.latency sampling change and scopes the win to hosts with an installed predicate, leaving the default-policy fast path out of scope.

[rebenitez1802]

Approving — the blocker from my previous review is resolved. The internal tracker ID is scrubbed from both reason= strings, and the two savepoint tests are now un-xfailed and passing for real on top of master's savepoint-rollback repair — nice reconciliation of the lazy-capture change with the upstream fix.

Re-verified: the capture logic is unchanged from my earlier pass (no new code slipped in via the merge), the un-xfailed tests assert the real repaired behavior (a rolled-back nested shadow leaves no stale *_version row), and CI is green across sqlite/postgres/presto.

Remaining items are all non-blocking nits, entirely optional: documenting the "every registered child must be Continuum-versioned" invariant in the baseline listener, and a one-line UPDATING.md note on the finalize-latency sample change. Thanks for the quick turnaround.