RehanAhmad25 commented on PR #70072: URL: https://github.com/apache/airflow/pull/70072#issuecomment-5011197943
Yeah, I think we can change it, RESTRICT looks like the right call here. Dug through git blame on this. `created_dag_version_id` was added in #49097 back in April 2025 (the "use best available serdag" PR), and it went with SET NULL from the start. What's interesting is that at that same time, `task_instance.dag_version_id` was actually on CASCADE, not RESTRICT, so deleting a dag_version would've silently wiped out any TI rows pointing at it. That got caught and fixed a month later in migration 0072 (3.1.0), moving TI over to RESTRICT specifically to stop that data loss. DagRun just never got the same follow-up. Looks like an oversight from that period rather than a deliberate choice, nothing in the PR discussion explains why DagRun should behave differently from TI here. I checked whether RESTRICT would actually be safe to add: the only place dag_version rows get deleted at all is delete_dag(), and it already deletes TaskInstance and DagRun rows before the DagModel cascade touches DagVersion (there's a comment in that function about doing this on purpose for TI's RESTRICT constraint). So DagRun would already be gone by the time the cascade fires, switching to RESTRICT shouldn't affect normal dag deletion at all, it'd only ever trigger in exactly the broken scenario this issue describes. I can add a migration for it, basically mirroring 0072 but for dag_run.created_dag_version_id, want me to add that to this PR or should it go separately? -- 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]
