mikebridge commented on code in PR #45029:
URL: https://github.com/apache/superset/pull/45029#discussion_r4232417894
##########
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:
Thanks, good catch. Added
`test_prune_child_delete_failure_rolls_back_parent_delete` in f03bdd38d2. It
runs the real `_run_prune_pass` with the parent delete succeeding and the
child-shadow delete raising. It asserts that the error propagates, that
`rollback()` is called once, and that `commit()` isn't called and the
transaction-row delete never runs.
Turning the `rollback()` into `commit()` makes the test fail. Test-only
change.
--
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]
