mikebridge commented on PR #45034: URL: https://github.com/apache/superset/pull/45034#issuecomment-6074705539
@rebenitez1802 on your [second review](https://github.com/apache/superset/pull/45034#pullrequestreview-5461543046): good catch on the missing isolation note. `UPDATING.md` now documents that tradeoff and recommends READ COMMITTED. Superset defaults only the `mysql`/`postgresql` URI backends when no isolation level is configured, so `mariadb://` and `mariadb+pymysql://` deployments must set `SQLALCHEMY_ENGINE_OPTIONS = {"isolation_level": "READ COMMITTED"}` explicitly. On the probes and lock, could we keep the existing behavior for this PR rather than restrict the protection to the scheduled purge? - Each check takes the permission-row lock and runs one or two `LIMIT 1` probes. `LIMIT 1` bounds the rows returned, not the scan cost, so deleting N datasets can repeat catalog scans and hold locks until commit; the scheduled purge cap doesn't bound that ORM path. I haven't benchmarked a large catalog, so I'm not claiming that cost is negligible. - Skipping the lock after an unlocked ownership check would need a separate concurrency proof, and scoping it to purge would remove the ORM path's protection. So the behavior stays as it is, with no index or migration, and the catalog-scale cost remains an explicit open concern. This follow-up changes no code paths. -- 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]
