Repository navigation
Conversation
There was a problem hiding this comment.
Has the new direct-delete path been tested against MySQL, both with and without batching? We should verify that the derived-table query works in both cases, including composite primary keys.
Also, I believe the CI failure should be fixed by PR #74122
|
Nice approach. Skipping the archive entirely instead of create-then-drop also removes the need for that There's going to be a conflict with #73568 which adds some warnings int he same block you are editing, would you consider putting this in draft until that one merges and them rebasing on top of it? #73417 is also in the same block so there's a lot of churn in there right now. Edit: actually, looking closer, this is nearly identical to the change I asked for in #73417 when I asked them to split theirs into two PRs. I'd prefer if we merged #73568, then this one, then (assuming mallesh splits the chart changes out) the chart portion of #73417 |
e2ba3a2 to
b353dce
Compare
@SameerMesiah97 I also rebased onto the latest main. The fresh CI run is in progress |
|
|
||
| logger.debug("direct delete statement:\n%s", delete.compile()) | ||
| session.execute(delete) | ||
| session.commit() |
There was a problem hiding this comment.
Could we keep the transaction boundary in the existing batch orchestration rather than committing inside _delete_directly()?
The existing cleanup loop already commits batches, so this is not a new behavioral regression, but the new helper could remain responsible only for executing the DELETE while its caller preserves the commit-per-batch behavior.
b353dce to
aaf2a90
Compare
|
#73568 merged. Ping me when you get the CI passing on this one and I'll give it a proper review. Thanks for your patience! |
aaf2a90 to
5c4f539
Compare
| with suppress(Exception): | ||
| session.rollback() | ||
| raise | ||
| continue |
There was a problem hiding this comment.
As written, this continue is right for the fix but will cause another because it makes the whole skip_if_referenced block around L613 unreachable.
One fix would be to have _delete_directly return the number of rows deleted and compare that against a count of the selection, warning when it deleted fewer than it found. deleted == 0 won't do, since L597-600 rejects it for staying silent on a partial skip. You can throw the warning at L604 into a helper that both paths can call.
There was a problem hiding this comment.
the continue is intentional here. The direct DELETE embeds limited_query, including its skip_if_referenced guard, so the condition is reevaluated within the DELETE itself. Unlike the archive path, there is no separately copied row that can remain both archived and live. Adding a count would introduce another query per batch and a new count/delete race. The regression test covers both whole and partial direct skips. I’ll add a comment to make this distinction clearer.
| print("Finished Performing Delete") | ||
|
|
||
|
|
||
| def _delete_directly( |
There was a problem hiding this comment.
It would likely be more efficient to build the table once and pass it into this method, no? Maybe I'm missing a reason to redo it each pass like this?
There was a problem hiding this comment.
Agreed. the source table does not change between batches, so reflecting it each time is unnecessary. I’ll cache the reflected table in _do_delete() and pass it to _delete_directly(), while keeping the initialization lazy so an empty cleanup does not perform reflection
The XCom read projection no longer accepts writes after the storage split, and direct deletion re-evaluates late reference guards without an archive snapshot.
Large batched cleanups should not repeat schema introspection for a source table that does not change between batches.
5c4f539 to
d9e025e
Compare
airflow db clean --skip-archivecurrently copies every selected batch into an archive table before deleting the source rows and dropping the archive table. On large metadata database cleanups, this extra copy generates substantial WAL on top of the delete and can cause sharp disk-usage spikes. It also requires schemaCREATE/DROPprivileges even though the archive is not preserved.This change gives
--skip-archivea direct-delete path:EXISTS, avoiding MySQL's unsupportedLIMITin anINsubquery.--skip-archivecontinues to create and retain archive tables.The CLI help and cleanup documentation now state that
--skip-archivedeletes directly without creating archive tables.PostgreSQL WAL comparison
On a disposable PostgreSQL 13 database, deleting 1,000,000 rows from the same two-column table and measuring with
pg_wal_lsn_diff(pg_current_wal_lsn(), :start_lsn)produced:CREATE TABLE AS, joinedDELETE,DROP): 172,750,560 bytesThe direct path generated approximately 42% less WAL in this comparison.
Tests
uv run --project airflow-core pytest airflow-core/tests/unit/utils/test_db_cleanup.py --with-db-init -xq(SQLite: 100 passed, 3 backend-specific skipped)batch_size=2(2 passed)prek run --from-ref upstream/main --stage pre-commitWas generative AI tooling used to co-author this PR?
Generated-by: Codex following the guidelines