Doris-Breakwater commented on issue #67303: URL: https://github.com/apache/doris/issues/67303#issuecomment-5466771201
Breakwater-GitHub-Analysis-Slot: slot_82ca316534bf ## Initial maintainer analysis **Assessment:** this is a high-confidence, source-verified FE correctness bug plus a separately verified long critical section. The ABA path can durably erase a newly recycled generation before its retention window expires; it should be prioritized as a correctness/data-recoverability issue, not only as lock contention. The database-cascade latency impact is workload-dependent, but the reported lock scope is real. The issue is currently open with no labels, assignee, or linked PR; it should be classified using the repository's normal FE/catalog correctness taxonomy. Reviewed against the reported `master` commit [`2689e0d7fdb111bf822cebc26d7d5765a563e276`](https://github.com/apache/doris/commit/2689e0d7fdb111bf822cebc26d7d5765a563e276). ### Verified facts 1. **The ABA interleaving is valid for database, table, and partition expiration.** The normal expiration passes snapshot only `List<Long> expiredIds` under the read lock, then later acquire the write lock and act on the entry currently mapped to that ID ([database](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L294-L333), [table](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L456-L507), [partition](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L601-L645)). None of those write-locked sections compares the current `Recycle*Info` or recycle timestamp with what was scanned, and none re-runs `isExpire`. 2. **Recovery followed by another drop replaces the generation while retaining the ID.** Each recycle operation constructs a new `Recycle*Info` and writes a new timestamp under the write lock ([recycle methods](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L169-L258)). Recovery removes that entry and timestamp but restores the same `Database`, `Table`, or `Partition` object/metadata ID ([database recovery](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L758-L800), [table recovery](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L866-L990), [partition recovery](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/do ris/catalog/CatalogRecycleBin.java#L995-L1105)). Therefore the old ID-only work item can select the fresh generation exactly as reported. 3. **The erroneous erase is durable.** Table and partition paths explicitly write erase journals while holding the lock; the database path calls `Env.eraseDatabase(..., true)`, which writes `logEraseDb` ([database journaling](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java#L7236-L7243)). This is not limited to transient leader memory. For a database ABA, the stale database work item can also remove the fresh database recycle entry while leaving its freshly recycled child-table entries behind, making normal database recovery impossible. 4. **The same-name database cascade holds the global recycle-bin write lock across the entire child pass.** `eraseDatabaseWithSameName()` enters the write lock before `eraseAllTables()`; that helper scans `idToTable`, invokes per-table callbacks, removes map entries, and journals each table before returning ([cascade](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L356-L434)). In cloud mode this scope can include a MetaService call from `beforeEraseTable()` and retrying RPC work, including three-second retry sleeps, from `onEraseOlapTable()` ([cloud cleanup](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/cloud/datasource/CloudInternalCatalog.java#L1057-L1117)). In non-cloud mode it still includes the full map scan and per-table/tablet cleanup. Thus the long-hold mechanism is confirmed even though no product ion wait-time measurement was supplied. 5. **The existing microbatch test does not cover this behavior.** `testMicrobatchEraseReleasesLockBetweenItems` recycles 50 fresh partitions with distinct names, sleeps, and runs the daemon ([test](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/test/java/org/apache/doris/catalog/CatalogRecycleBinTest.java#L989-L1048)). The normal expiration default is one day, and `FeConstants.runningUnitTest` bypasses only `isExpireMinLatency`, not `isExpire` ([expiration check](https://github.com/apache/doris/blob/2689e0d7fdb111bf822cebc26d7d5765a563e276/fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java#L288-L292)). Each same-name group also has size one, below the default keep count of three. Consequently this test appears to execute no erase item at all; replacing `Thread.sleep(50)` with deterministic coordination is necessary but not sufficient—the fixtures must first be made eligible for erasure. The current head of draft PR #61504 (`1e8b4145524157feede2950971d01ee0288ab1f5`) still uses ID-only expired snapshots and retains the database cascade, so that work should be coordinated with this fix rather than treated as resolving it. ### Required fix properties - Snapshot an immutable candidate containing at least `(id, Recycle*Info identity, recycleTime)` while holding the read lock. Under the later write lock, require both the current info reference and timestamp to match the candidate and re-check expiration using the current time before doing any callback, removal, or journal write. Apply the invariant consistently to database, table, and partition expiration. - Do not move database child cleanup outside the lock without a recoverability protocol. Reserve/mark the exact database generation as erasing under the lock, prevent `recoverDatabase` from observing that generation as recoverable, process children in bounded lock scopes, and finalize/log the database erase only after consistent child completion. - Define reservation durability across FE leader change, edit-log replay, and image load. An in-memory-only flag is insufficient if a leader fails after some child erase journals but before the database erase journal. - Define failure semantics explicitly. The current `eraseAllTables()` can successfully remove and journal some children, encounter `beforeEraseTable()` failure for another child, return `false`, and leave the database entry referring to its original child set. Any redesign should make partial progress retryable without exposing a partially erased database to recovery. - Keep slow external/RPC cleanup and journal waits out of a global critical section wherever the reservation protocol permits it. ### Tests and remaining evidence No runtime logs or Doris profile are needed to establish the ABA root cause from this revision, but the PR should provide deterministic tests before merge: 1. For database, table, and partition separately, pause after candidate capture, recover and recycle the same ID with a fresh timestamp, resume, and assert that the fresh info object/timestamp remains and no erase journal/callback targets it. 2. Add a control case proving an unchanged expired generation is erased, so a stale-work guard cannot silently disable cleanup. 3. For the database cascade, block the first child callback with latches, start an unrelated recycle writer, and assert the intended bounded-lock behavior without sleeps. 4. Cover callback failure after at least one child succeeded, retry, concurrent recovery rejection, leader replay/image reload between child and database finalization, and the requirement that the database erase journal follows all child erases. 5. Run the concurrency cases for both the ordinary internal catalog and the cloud override, where cleanup performs external RPCs. To quantify operational priority for the lock issue, a production or stress-test thread dump/lock-wait measurement plus database/table counts and cloud/non-cloud mode would be useful. That measurement is not a prerequisite for accepting the source-level correctness defect. -- 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]
