mikebridge commented on code in PR #44089:
URL: https://github.com/apache/superset/pull/44089#discussion_r4007205421
##########
superset/versioning/changes/shadow_queries.py:
##########
@@ -317,8 +340,14 @@ def _dashboard_child_records_for_tx_from_shadows(
if prior_tx is None:
continue
- post_uuids = _dashboard_slice_uuids_at_tx(session, dashboard_id,
transaction_id)
- pre_uuids = _dashboard_slice_uuids_at_tx(session, dashboard_id,
prior_tx)
+ # Resolve the attachment windows once (threading the committing
+ # *session* so the flushed-but-uncommitted current-tx association rows
+ # are visible), then take the pre/post membership by which windows
+ # contain each tx — the windows are tx-independent, so no need to
+ # re-scan the association history for both reads.
+ attached = charts_attached_to_dashboard(dashboard_id, session=session)
+ post_uuids = _dashboard_slice_uuids_at_tx(session, attached,
transaction_id)
+ pre_uuids = _dashboard_slice_uuids_at_tx(session, attached, prior_tx)
Review Comment:
Agreed it's real and pre-existing, and thanks for the concrete tx=2/3 case.
I looked at decoupling membership from content-validity here and decided
against it in this PR: the obvious fix — resolve the uuid from *any*
`slices_version` row for the id — is id-reuse-unsafe. A hard delete frees the
integer id, and a later chart can carry it with a different uuid, so the
content row valid AT tx is precisely what disambiguates incarnations; dropping
that would trade the phantom membership diff for a wrong-entity diff (the same
bug class #44015 just closed on the restore lock). A correct decoupling needs
an id-reuse-safe anchor (e.g. the earliest content row at/after the attachment
window's start, with the incarnation boundary handled), which is more than this
rewrite should carry. I've recorded the consequence and the reason in the
docstring (b0e79891b1) and would file it as a follow-up rather than fold it in
— happy to be told otherwise.
##########
superset/versioning/changes/shadow_queries.py:
##########
@@ -317,8 +340,14 @@ def _dashboard_child_records_for_tx_from_shadows(
if prior_tx is None:
continue
- post_uuids = _dashboard_slice_uuids_at_tx(session, dashboard_id,
transaction_id)
- pre_uuids = _dashboard_slice_uuids_at_tx(session, dashboard_id,
prior_tx)
+ # Resolve the attachment windows once (threading the committing
+ # *session* so the flushed-but-uncommitted current-tx association rows
+ # are visible), then take the pre/post membership by which windows
+ # contain each tx — the windows are tx-independent, so no need to
+ # re-scan the association history for both reads.
+ attached = charts_attached_to_dashboard(dashboard_id, session=session)
+ post_uuids = _dashboard_slice_uuids_at_tx(session, attached,
transaction_id)
+ pre_uuids = _dashboard_slice_uuids_at_tx(session, attached, prior_tx)
Review Comment:
The extra reads are the correctness fix, not incidental cost: the previous
single join filtered the M2M association shadow by `end_transaction_id`, which
Continuum never closes, so it over-reported membership (sc-119907). Membership
now comes from `charts_attached_to_dashboard`'s attach/detach windows — the
same helper restore and the impact rollup already use — and the slice scan is
member-scoped and chunked. Both are bounded per affected dashboard by that
dashboard's own attachment history (charts ever attached × attach/detach
events), which is small in practice, and only run at commit finalization for
dashboards actually touched in the transaction. Reviewed for this trade-off by
the panel + cross-model pass before opening; keeping as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
