sadpandajoe commented on code in PR #45029:
URL: https://github.com/apache/superset/pull/45029#discussion_r4232172548
##########
superset/tasks/version_history_retention.py:
##########
@@ -364,44 +415,65 @@ def _run_prune_pass(
# parents: children and the M2M association live on independent
# validity lifecycles and may anchor a still-live row at an older
# transaction than the parent's current live row.
- live_bearing_tables: list[sa.Table] = [*tables.parent, *tables.child]
- if tables.m2m is not None:
- live_bearing_tables.append(tables.m2m)
+ live_bearing_tables: list[sa.Table] = _live_bearing_tables(tables)
# The Celery task runs outside the request-bound DB session, so we
# use a fresh connection rather than ``db.session`` to avoid stepping
# on web-request state.
- with (
- db.engine.connect().execution_options(isolation_level="SERIALIZABLE")
as conn,
- conn.begin(),
- ):
- window = _resolve_prune_window(
- conn, cutoff, live_bearing_tables, after_id, _MAX_PRUNE_BATCH
- )
- tx_ids = window.prunable
-
- parent_rows = _delete_for_transactions(conn, tables.parent, tx_ids)
- child_rows = _delete_for_transactions(conn, tables.child, tx_ids)
- m2m_rows = (
- _delete_for_transactions(conn, [tables.m2m], tx_ids)
- if tables.m2m is not None
- else 0
- )
-
- # Drop the version_transaction rows themselves. ON DELETE
- # CASCADE on version_changes.transaction_id removes the
- # associated change records automatically. Same SQLite bind-
- # parameter chunking applies as the shadow deletes above.
- tx_rows = 0
- for chunk in _chunked(tx_ids, _TX_ID_CHUNK_SIZE):
- tx_rows += (
- conn.execute(
- sa.delete(tables.transaction).where(
- tables.transaction.c.id.in_(chunk)
- )
- ).rowcount
- or 0
+ with db.engine.connect().execution_options(isolation_level="SERIALIZABLE")
as conn:
+ transaction: sa.engine.Transaction = conn.begin()
+ try:
+ window: _PruneWindow = _resolve_prune_window(
+ conn, cutoff, live_bearing_tables, after_id, _MAX_PRUNE_BATCH
)
+ tx_ids: list[int] = window.prunable[:max_prune]
+
+ parent_rows: int = _delete_for_transactions(conn, tables.parent,
tx_ids)
+ child_rows: int = _delete_for_transactions(conn, tables.child,
tx_ids)
+ m2m_rows: int = (
+ _delete_for_transactions(conn, [tables.m2m], tx_ids)
+ if tables.m2m is not None
+ else 0
+ )
+
+ # Drop the version_transaction rows themselves. ON DELETE
+ # CASCADE on version_changes.transaction_id removes the
+ # associated change records automatically. Same SQLite bind-
+ # parameter chunking applies as the shadow deletes above.
+ tx_rows: int = 0
+ for chunk in _chunked(tx_ids, _TX_ID_CHUNK_SIZE):
+ tx_rows += (
+ conn.execute(
+ sa.delete(tables.transaction).where(
+ tables.transaction.c.id.in_(chunk)
+ )
+ ).rowcount
+ or 0
+ )
+ except Exception:
+ try:
+ transaction.rollback() # pylint:
disable=consider-using-transaction
Review Comment:
The old `with conn.begin():` rolled back automatically when a delete raised;
this explicit `except` branch is now what keeps a failure between the
parent-shadow delete and the transaction-row delete from being committed. No
test makes `_delete_for_transactions` or the transaction delete raise inside
the real `_run_prune_pass`: `test_retention_retries_on_serialization_failure`
fails before the pass runs, and `test_prune_failed_rollback_stops_retry` fails
at commit. If this `rollback()` became a `commit()`, every test here would
still pass while a half-applied prune (parent history gone, child rows and the
transaction left) went through. Could you add a case where the child-shadow
delete raises after the parent delete and assert the error propagates and
`rollback` (not `commit`) is called?
--
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]
