mikebridge commented on PR #41549: URL: https://github.com/apache/superset/pull/41549#issuecomment-5118149997
Heads-up that `9c73e69faf` landed after your re-approval — flagging it rather than merging over it silently, because it's a behavioural fix and it's squarely in the area you were worried about. The orphan-transaction sweep counted a `version_transaction` as referenced only through `transaction_id`, never `end_transaction_id`. Both are foreign keys: a shadow row points at the transaction that created it and, once a later edit closes it, at the one that ended it. Transactions spanning entities are the normal case — one flush saving a dashboard, chart and dataset gives all three a row at the same transaction — so as soon as any one of them is edited again, the others' end pointers keep that transaction alive. The sweep judged it orphaned, the delete failed the FK, and purge could not remove any entity with version history. The way it failed is the worse half. `cascade_hard_delete` catches `IntegrityError` alongside `PurgeBlockedError`, so the operator was told the entity was "blocked by existing deletion rules" — a deliberate policy decision — when the cascade was simply incomplete. It looked like intended behaviour. That masquerade is filed separately as an internal follow-up; this commit just removes the instance of it. This is the maintenance risk you named on 07-13, arriving through a column rather than a missing table. Worth noting how it surfaced: **this PR's own suite never purged an entity carrying version history through the API**, so CI was honestly green while the feature's core operation was broken for exactly the case it exists to serve. It only appeared because a test on the recovery-UI PR started exercising the path today. There's now a regression test here that fails against the old code. `version_history_retention` had already hit the identical trap and documents it on `_delete_for_transactions` — the reasoning transfers verbatim, which is a decent argument for the introspection-based tripwire we discussed rather than relying on either of us spotting it. CI is green on the new head. No obligation to re-read — the change is ~10 lines plus a test — but you're welcome to, and I'd rather you heard it from the PR than found it later. -- 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]
