gauravchodwadia commented on issue #57492: URL: https://github.com/apache/airflow/issues/57492#issuecomment-5465772149
> And if you look at your dag runs table. Are there any failed dag runs? Picking this up since it went unanswered — the numbers in the original report answer it. `dag_stats` returned `failed_dag_count: 1` while `historical_metrics_data` over the same 24h returned 10 scheduled runs, all successful. So one Dag's latest run failed at some point *before* that window, and nothing failed inside it. Those two panels are measuring different things: `dag_stats` counts **Dags** whose latest run failed, with no time bound, while `historical_metrics_data` counts **DagRuns** inside the selected `run_after` window. They will disagree whenever a Dag's last failure predates the window. That looks like a labelling question rather than a bug, and I don't think it's mine to decide. While reading that code I did find something next to it that does look like a defect, along with a reason it's more awkward to fix than it first appears. **The card disagrees with the list it links to.** `dag_stats` resolves a Dag's latest run with `ORDER BY logical_date DESC` filtered on `logical_date IS NOT NULL`, but the Dags list behind that card's own link (`dags?last_dag_run_state=failed`) uses `max(DagRun.id)` via `generate_dag_with_latest_run_query`. Since `logical_date` is nullable in Airflow 3, a Dag whose most recent run is manual or asset-triggered gets scored on an older dated run. It diverges both ways — an old failure counted when the latest run succeeded, and a current failure missed when the older dated run succeeded. I have regression tests where the card reads 1 and the list it links to reads 0, and the mirror case. **The awkward part.** The obvious fix is to order by `id` so both surfaces agree. But #67721 introduced that correlated `ORDER BY logical_date DESC LIMIT 1` deliberately — "Adding new indexes would be a larger change (a migration), so this PR focuses on reusing the existing indexes" — and `logical_date` turns out to be the only candidate ordering key with a per-Dag composite index to reuse. I measured the alternatives on PostgreSQL 17. Fair warning on the method: this is a **synthetic replica**, not a real Airflow database — `dag`/`dag_run` tables carrying just the columns these queries touch, plus the full real index set from `DagRun.__table_args__`. Row widths differ from production, so treat the ratios as the signal rather than the absolute milliseconds. 2,000 Dags × 2,500 runs, ~4.8M `dag_run` rows after making 5% of Dags dormant (last run old — a sparse schedule, or paused after a burst): | latest-run ordering | plan | time | |---|---|---| | `logical_date DESC` (today) | `Index Scan Backward using dag_run_dag_id_logical_date_key` | 21 ms | | `id DESC` | `Index Scan Backward using dag_run_pkey` | 27,126 ms | | `run_after DESC` | `Index Scan Backward using idx_dag_run_run_after` | 24,221 ms | | `id DESC` + `(dag_id, id)` index | `Index Scan Backward using idx_dag_run_dag_id_id` | 16 ms | So `run_after` is no escape, and the correctness fix needs an index to come with it. Worth noting the regression only shows up once some Dags are dormant — with every Dag recently active, `id DESC` benchmarks at ~97 ms and looks fine, which is how it would slip through. A `(dag_id, id)` index costs ~165 MB at that row count and makes the corrected query slightly faster than the current one. Since `dag_id` is its leading column it should also serve everything `idx_dag_run_dag_id` (~34 MB) serves, so it might be able to replace that index rather than add to it — I haven't verified that across the other queries that use it, so it's a question rather than a claim. Which shape would you prefer? 1. Narrow correctness fix plus the `(dag_id, id)` migration. 2. Correctness fix only, if there's a formulation you'd rather use that keeps the current index. 3. Leave it — if the card-vs-list mismatch is considered acceptable. Happy to open a PR for whichever, tests included. The benchmark is a single self-contained script and I'm glad to post it so the numbers can be checked or re-run at a different shape. One last observation in case it's useful for the broader "what is a Dag's latest run" question: `get_dags` renders each row's run history with `ORDER BY run_after DESC` while selecting which rows match `last_dag_run_state` via `max(id)`, so for a backfill the list can show a row whose displayed last run is green. That's pre-existing and separate from this — just the same underlying ambiguity showing up somewhere else. *Disclosure: I used an AI assistant while reading the code, building the benchmark harness, and drafting this comment. The measurements are ones I ran myself and the script is reproducible — happy to post it.* -- 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]
