Fix archival for cascading deletes by archiving dependent tables first #51952
Conversation
There was a problem hiding this comment.
Interesting approach.
I could approve easly but I have a small caveat and consideration.
I would love for others to take a look and I would likely just hard-code the relationships if I were implementing it (they are very, unlikely to change and we do not have many of those).
I wonder if there is no ready to use tools /libs that already do that (sounds like a problem that many people might have) and maybe we should look for it rather than using our code that looks complex'ish. And I am not entirely sure if the inspector will sufficiently abstract away the DB structure - there might be some subtle behavioural differences between different databases and I can imagine that this code starts failing because of some "small" changes in Postgres or MySQL in the future, or not working in some past versions of those.
Alternatively - maybe we can - rather than having all the topological sort, simply hardcode the sequences of deletion of tables and sort deletion according to it?
That sounds like way more maintainable, less code, less complexity and easier to reason about.
|
That’s a fair point — I agree that keeping the cleanup logic simple and explicit (especially in a critical area like deletions) is worth the trade-off in maintainability. I have added FK relationship in our already existing table_config.
|
Cool :) |
|
ok it seems the main change here is that you add the dependent tables to the "effective table list" but, i do not see whether / how you ensure that you sort the deletions "outside in" such that you purge and archive the dependent tables prior to the target tables -- am i missing that logic? |
The way we construct the list of tables means we do do it with dependents first, and we rely on dict ordering to keep the right order as we start cleaning. |
| @@ -81,6 +82,10 @@ class _TableConfig: | |||
| keep_last: bool = False | |||
| keep_last_filters: Any | None = None | |||
| keep_last_group_by: Any | None = None | |||
| # We explicitly list these tables instead of detecting foreign keys automatically, | |||
Backport failed to create: v3-0-test. View the failure log Run details
You can attempt to backport this manually by running: cherry_picker c77fdd6 v3-0-testThis should apply the commit to the v3-0-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continue |
#51952) Co-authored-by: Jed Cunningham <[email protected]> (cherry picked from commit c77fdd6)
apache#51952) Co-authored-by: Jed Cunningham <[email protected]> (cherry picked from commit c77fdd6) (cherry picked from commit b06d15a)
…t tables first (#51952) (#52211) * Fix archival for cascading deletes by archiving dependent tables first (#51952) Co-authored-by: Jed Cunningham <[email protected]> (cherry picked from commit c77fdd6) (cherry picked from commit b06d15a) * Fix test --------- Co-authored-by: Rahul Vats <[email protected]>
When cleaning up tables like
dag_runin Airflow, foreign key constraints (e.g., task_instance.dag_run_id) can trigger cascading deletions. As a result, related records (like task_instance) may be deleted before they are archived, leading to incomplete archival and potential data loss.This PR updates the cleanup logic to:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.