wenzhenghu opened a new issue, #67303: URL: https://github.com/apache/doris/issues/67303
### Search before asking - [x] I searched the existing issues and pull requests. #48224 reported the earlier general recycle-bin lock contention, but it was closed as stale and does not cover the ABA correctness race or the remaining database-cascade lock scope after #61366. ### Version Apache Doris `master` at `2689e0d7fdb111bf822cebc26d7d5765a563e276` (2026-08-28). The behavior was introduced/retained by #61366 (`e2a678cb74a5ef6278a7c968901c2de9348e0626`) and is still present on the current `master`. ### What's Wrong? #61366 changed `CatalogRecycleBin` from a coarse monitor to a `ReentrantReadWriteLock` and introduced per-item erase processing. Two problems remain. #### 1. Correctness: stale expired-ID snapshots can erase a newly recycled generation (ABA race) The normal expired database/table/partition paths: 1. Collect only expired object IDs under the read lock. 2. Release the read lock. 3. Later acquire the write lock and erase whichever `Recycle*Info` is currently mapped to that ID. There is no validation that the current `Recycle*Info` instance and recycle timestamp are the same generation observed during the expired scan, and expiration is not checked again after acquiring the write lock. Current code: - Database: https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L298-L333 - Table: https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L460-L507 - Partition: https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L605-L645 A possible interleaving is: 1. The recycle-bin daemon scans an expired entry with ID `X` and stores `X` in `expiredIds`. 2. A concurrent DDL recovers `X`, removing the old recycle-bin entry. 3. The object is dropped again. Recovery preserves the metadata ID, so this creates a new `Recycle*Info` for ID `X` with a fresh recycle timestamp. 4. The daemon processes its stale ID snapshot and erases the new entry without checking its identity or fresh timestamp. This can bypass the configured retention window and make a newly dropped database/table/partition unrecoverable. The erase operation is also journaled, so the wrong deletion is not merely an in-memory transient. The table path was later changed to call `get(tableId)` before cleanup and `remove(tableId)` afterward, but it still operates on the current generation without revalidating the snapshot or expiration, so the race remains. #### 2. Lock granularity: same-name database cleanup still erases all child tables under one write lock `eraseDatabaseWithSameName()` acquires the recycle-bin write lock and calls `eraseAllTables()`. While that lock is held, `eraseAllTables()` scans `idToTable` and, for every matching table, performs `beforeEraseTable`, `onEraseOlapTable`, map removals, and `logEraseTable`: https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L356-L434 Consequently, an old same-name database containing many tables can still hold the global recycle-bin write lock for `O(idToTable size + child-table count * per-table cleanup cost)`. Concurrent DROP/recycle operations can wait for the entire database cascade, including when their callers already hold database/table metadata locks. This retains the lock-amplification risk that the microbatch change intended to remove. The later #65859 added `beforeEraseTable()` to this loop but did not split the lock scope. The currently open #61504 also changes retention behavior without addressing either concurrency pattern. ### What You Expected? 1. An expired-scan result must identify the exact generation that was observed. After reacquiring the write lock, the daemon should verify that the current `Recycle*Info` and recycle timestamp still match the snapshot and that the entry is still expired before performing any cleanup. If it was recovered/recycled or its timestamp changed, the stale work item should be skipped. 2. Same-name database cascade cleanup should release the recycle-bin write lock between child tables, while using an explicit deletion reservation/state (or an equivalent protocol) to prevent `recoverDatabase` from observing or recovering a partially erased database. The database erase journal should be written only after its child cleanup has completed consistently. The same generation-validation rule should be applied to database, table, and partition paths. ### How to Reproduce? The ABA race can be covered deterministically with an FE unit-test hook/latch: 1. Put an expired database, table, or partition in `CatalogRecycleBin`. 2. Start `runAfterCatalogReady()` and pause it after the expired snapshot is collected but before the per-item write lock is acquired. 3. Recover the object and recycle/drop the same object again, preserving its ID and recording a fresh recycle timestamp. 4. Resume the daemon. 5. Verify that the new recycle-bin entry and its fresh timestamp remain and that no erase journal was produced for the new generation. This test should be repeated for database, table, and partition paths. The database-cascade lock issue can also be demonstrated deterministically: 1. Create enough same-name recycled databases to exceed `max_same_name_num`, with one old database containing many recycled tables. 2. Block or slow the first per-table cleanup callback while `eraseDatabaseWithSameName()` is running. 3. Concurrently invoke another recycle-bin write operation such as `recyclePartition()`. 4. Observe that it cannot acquire the recycle-bin write lock until all child tables of that database have been processed. The existing `testMicrobatchEraseReleasesLockBetweenItems` relies on `Thread.sleep(50)` and does not establish that the daemon is still in the erase iteration when the concurrent recycle runs: https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/test/java/org/apache/doris/catalog/CatalogRecycleBinTest.java#L989-L1050 ### Anything Else? Related history: - Earlier general lock-contention report: #48224 - Lock-granularity/microbatch implementation: #61366 - Later code added more per-table work inside the same database-cascade write-lock scope: #65859 - Open retention redesign that currently retains these patterns: #61504 This report is based on source and history analysis of the current `master`. A deterministic race test has not yet been run. ### Are you willing to submit PR? - [ ] Yes I am willing to submit a PR! ### Code of Conduct - [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct) -- 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]
