[fix](dictionary) stabilize insert target during database drop#65476
[fix](dictionary) stabilize insert target during database drop#65476zclllyybb wants to merge 3 commits into
Conversation
Resolve insert targets as a database-table pair so authorization and retry validation do not recover the database through a stale table object. Carry the dictionary owner database through Nereids sink binding and executor creation. Remove dropped dictionary IDs from the canonical map and validate exact dictionary identity before and after loading to reject same-name recreation. Add deterministic FE unit races for normal inserts and dictionary loads, plus a debug-point regression covering concurrent REFRESH DICTIONARY and DROP DATABASE. Tests: ./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.insert.InsertTargetDropRaceTest Tests: ./run-regression-test.sh --run -f regression-test/suites/dictionary_p0/test_create_drop_sync.groovy Tests: ./build.sh --fe Tests: ./build.sh --be
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review found two issues, so I am requesting changes.
Critical checkpoint conclusions:
- Goal and correctness: the paired database/table target flow addresses the main stale-target path, but the dictionary load still has a post-check drop race that can journal an unreplayable version decrement.
- Scope: the implementation is focused on FE dictionary insert/drop handling and related tests.
- Concurrency, lifecycle, and persistence: M1 covers a remaining drop/commit-failure interleaving that can leave OP_DICTIONARY_DEC_VERSION after OP_DROP_DICTIONARY.
- Parallel paths and config/session propagation: normal insert and dictionary insert paths were checked; I did not find an additional session/config propagation issue.
- Tests: the added race coverage is useful, but M2 notes that the dictionary test gate is not deterministic for the intended block-before-planning path.
- Validation: static review only. This checkout is shallow and lacks thirdparty/installed and executable thirdparty/installed/bin/protoc, so I did not run FE/regression tests.
- User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite produced no candidates and converged with NO_NEW_VALUABLE_FINDINGS. tests-session-config proposed TC-1, which was accepted as M2. After M1 and M2 were in the ledger, both live subagents returned NO_NEW_VALUABLE_FINDINGS in convergence round 1.
| try { | ||
| if (!dictionaryIds.containsKey(dictionary.getDbName()) | ||
| || !dictionaryIds.get(dictionary.getDbName()).containsKey(dictionary.getName())) { | ||
| if (!isCurrentDictionaryWithoutLock(dictionary)) { |
There was a problem hiding this comment.
This identity check is still too early for the failure path below. After this block releases the dictionary-manager read lock, a concurrent DROP DICTIONARY/DROP DATABASE can remove this same dictionary and log OP_DROP_DICTIONARY; if commitNowVersion() then returns false, lines 527-528 still call logDictionaryDecVersion(dictionary) for an object that is no longer current. On replay the drop removes the dictionary, and replayDecreaseVersion() calls getDictionary(dbName, dictName), so the later decrement entry cannot be applied. Please recheck/hold the dictionary identity before logging the decrement, and skip the rollback journal entry once the dictionary has already been dropped.
|
|
||
| Dictionary dictionary = Env.getCurrentEnv().getDictionaryManager().getDictionary(dbName, "dic1"); | ||
| try { | ||
| await(() -> dictionary.getStatus() == DictionaryStatus.LOADING); |
There was a problem hiding this comment.
Waiting for LOADING does not guarantee the load thread has reached DictionaryManager.dataLoad.blockBeforePlan. dataLoad() sets LOADING before it resolves/checks the database and before it enters the debug-point loop, so this test can drop the database early and still pass through the new database == null || !isCurrentDictionary(...) branch without exercising the stale captured-database/planning path. Please synchronize on a point reached after the debug point is entered, or make the debug point signal the test, so this race test fails if the planning path regresses.
TPC-H: Total hot run time: 29123 ms |
TPC-DS: Total hot run time: 180359 ms |
ClickBench: Total hot run time: 25.03 s |
zclllyybb
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
|
/review |
There was a problem hiding this comment.
Automated review complete. I found three non-duplicate correctness issues and left inline comments.
Checkpoint conclusions:
- Goal: the PR aims to stabilize insert/dictionary targets during drop or recreate races. It improves dictionary identity checks and database propagation, but the current implementation still has retry-side effects and one dictionary cleanup bug.
- Scope/focus: the change is mostly focused on FE insert target handling and dictionary refresh. The dictionary database is preserved through the Nereids sink chain.
- Concurrency/lifecycle: issues remain in normal insert retry cleanup and group-commit state across retry attempts. Dictionary drop/replay has an existing review thread; a separate partial-load abort-version issue is reported inline.
- Persistence/compatibility: no format or mixed-version protocol change was found. The partial dictionary cleanup issue can leave BE refreshing state behind after commit failure.
- Parallel paths: normal insert, dictionary insert, bind/physical sink propagation, drop table/database dictionary paths, and tests were reviewed. Existing comments already cover the JUnit debug-point synchronization gap and decrement-after-drop replay hazard.
- Tests: static review only. The review prompt explicitly says not to build or modify code, so I did not run FE/regression tests. No additional user-provided focus was supplied.
| */ | ||
| public AbstractInsertExecutor(ConnectContext ctx, TableIf table, String labelName, NereidsPlanner planner, | ||
| Optional<InsertCommandContext> insertCtx, boolean emptyInsert, long jobId, boolean needRegister) { | ||
| this(ctx, table.getDatabase(), table, labelName, planner, insertCtx, emptyInsert, jobId, needRegister); |
There was a problem hiding this comment.
This overload still lets normal insert retry attempts recover the database from the stale Table object before the new database/table ID recheck runs. In initPlan(), ExecutorFactory.build() constructs the executor before the second getTarget() comparison; the OLAP path still passes only the original targetTableIf, so this constructor calls table.getDatabase() and, with needRegister, immediately adds an InsertLoadJob. If the original database/table was dropped and recreated while planning, Table.getDatabase() resolves the old table's qualifiedDbName against the current catalog and can register a load job under the replacement database even though the retry check later sees the ID mismatch and continues. Please either pass the captured InsertTarget database into the normal executor constructors too, or delay executor construction/load-job registration until after the target identity check has proven this planned target is still current.
| try { | ||
| if (targetTableIf.getId() != newestTargetTableIf.getId()) { | ||
| LOG.warn("insert plan failed {} times. query id is {}. table id changed from {} to {}", | ||
| if (targetDatabase.getId() != newestTargetDatabase.getId() |
There was a problem hiding this comment.
This retry branch needs to clear per-attempt state before continue. analyzeGroupCommit() runs earlier in the same loop and can set ctx.groupCommit from the target being planned; if that target is then rejected here because the database/table ID changed, the next retry reuses the same ConnectContext. Since analyzeGroupCommit() returns immediately when the flag is already true, the replacement table is never rechecked for schema-light-change, partition, MTMV, transaction, or plan-shape eligibility, and the planner/executor can still take the group-commit path for a target that would not qualify. Please reset/recompute the group-commit flag when discarding a retry attempt, while preserving the HTTP stream-load case that intentionally pre-sets it.
| try { | ||
| if (!dictionaryIds.containsKey(dictionary.getDbName()) | ||
| || !dictionaryIds.get(dictionary.getDbName()).containsKey(dictionary.getName())) { | ||
| if (!isCurrentDictionaryWithoutLock(dictionary)) { |
There was a problem hiding this comment.
There is another cleanup mismatch in this same post-load section for partial refreshes. When statementContext.isPartialLoadDictionary() is true, the sink staged dictionary.getVersion() on the selected BEs, not version + 1; DictionarySink.toThrift() only uses version + 1 for full refreshes. But if commitNowVersion() fails, this branch skips the full-load decrement and still calls abortSpecificVersion(..., dictionary.getVersion() + 1). BE abort only erases the refreshing dictionary when the version matches exactly, so a failed partial load can leave the staged current-version refresh in _refreshing_dict_map. Please abort dictionary.getVersion() for partial loads and dictionary.getVersion() + 1 for full loads.
### What problem does this PR solve? Issue Number: None Related PR: apache#65476 Problem Summary: A dictionary commit failure could append a version-decrement journal after the dictionary had already been dropped, which breaks edit-log replay. Partial refresh failures also aborted the next version instead of the version staged on BEs. Separately, insert retries constructed executors and registered load jobs before confirming that the planned database and table were still current, and a discarded group-commit attempt could leak derived state into the next attempt. Serialize the dictionary identity check and full-refresh rollback journal with DROP, capture and abort the exact staged version, and delay insert executor construction until target ID and schema validation succeeds. Restore both group-commit mode and its selected merge backend when an attempt is discarded. ### Release note Fix concurrent dictionary refresh/drop cleanup and prevent discarded insert plans from creating load jobs or leaking group-commit state. ### Check List (For Author) - Test: Unit Test and Regression test - InsertTargetDropRaceTest: 6 tests passed - InsertIntoTableCommandTableStreamTest: 4 tests passed - dictionary_p0/test_create_drop_sync.groovy: passed - ./build.sh --fe: passed - Behavior changed: Yes. Failed refresh cleanup is replay-safe and insert retry side effects occur only for a validated target. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65476 Problem Summary: The FE Code Style Checker rejected the dictionary race test because BooleanSupplier appeared after Consumer instead of following lexicographical import order. Sort the imports according to the repository CheckStyle rule. ### Release note None ### Check List (For Author) - Test: mvn clean checkstyle:check - Behavior changed: No - Does this need documentation: No
|
/review |
|
run buildall |
TPC-H: Total hot run time: 29585 ms |
TPC-DS: Total hot run time: 180188 ms |
There was a problem hiding this comment.
Automated review completed after three rounds with subagent convergence. I am requesting changes for three remaining issues: retry planning keeps stale StatementContext relation caches, the Groovy dictionary race regression can pass before the intended debug point is reached, and prepared group-commit full-prepare can still build a cached planner from a pre-validation target table.
Critical checkpoints: the PR goal is clear and most dictionary full/partial version, drop/replay, and delayed executor-construction paths now look addressed, but retry/lifecycle coverage is still incomplete in the accepted cases. The relevant concurrency risk is metadata drop/recreate racing with FE planning state; no new config, protocol, storage-format, or FE-BE compatibility issue was found. Test coverage was reviewed statically only; no builds or tests were run in this review-only environment, and .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc are missing. User focus: no additional user-provided review focus was supplied.
| ctx.setGroupCommit(groupCommitBeforeAttempt); | ||
| ctx.getStatementContext().setGroupCommitMergeBackend(groupCommitBackendBeforeAttempt); | ||
| newestTargetTableIf.readUnlock(); | ||
| continue; |
There was a problem hiding this comment.
This retry still keeps the failed attempt's StatementContext relation caches. Each attempt builds a new CascadesContext from the same ctx.getStatementContext(), and CollectRelation/BindRelation populate tables and insertTargetTables through getAndCacheTable(...).computeIfAbsent; StatementContext.close() only releases locks, not those maps. After INSERT INTO db.target SELECT ... FROM db.source plans once, a drop/recreate can make this branch detect the old target id and continue, but the next attempt can still bind db.source and even insertTargetSchema from the stale cached objects while BindSink re-resolves the replacement target. If the schemas match, the new id/schema validation can pass with old source metadata in the plan. Please clear/recreate the per-attempt relation state before retrying, or plan retries with a fresh StatementContext while preserving only intentionally shared statement state.
| refreshFuture = thread { | ||
| sql "REFRESH DICTIONARY test_create_drop_sync.dic1" | ||
| } | ||
| awaitUntil(10) { |
There was a problem hiding this comment.
This wait still does not prove the refresh thread reached DictionaryManager.dataLoad.blockBeforePlan. dataLoad() sets LOADING before the database/current-dictionary check and before the debug-point loop, so this test can observe LOADING, drop the database, and have the refresh fail at the early dropped-dictionary check with the same message asserted below. In that case the regression passes without exercising the captured-database planning path it is meant to cover. Please synchronize on a signal reached after the FE debug point is actually entered, or keep this exact race coverage in a harness that can inspect DebugPoint.executeNum.
| Throwables.throwIfInstanceOf(e, RuntimeException.class); | ||
| throw new IllegalStateException(e.getMessage(), e); | ||
| } | ||
| if (!needBeginTransaction) { |
There was a problem hiding this comment.
This needBeginTransaction=false validation still leaves prepared group-commit full-prepare with a stale target. GroupCommitPlanner.executeGroupCommitInsert() resolves OlapTable table = command.getTable(ctx) before calling command.initPlan(..., false), but after this branch returns it ignores the returned executor and builds/caches the stream-load planner from that earlier table and table.getDatabase(). If the database/table is dropped and recreated between those calls, initPlan(false) can retry and validate the replacement target while the cached GroupCommitPlanner is built from the dropped table object's schema/id. Please have the full-prepare caller derive the database/table from the target that survived initPlan(false) validation, or re-resolve and check the target ids again after validation before constructing or reusing the cached planner.
ClickBench: Total hot run time: 24.9 s |
|
/review |
There was a problem hiding this comment.
I found four issues that should be addressed before merging. The most important ones are in dictionary refresh failure handling: staged BE dictionary versions can survive both pre-commit load failures and mixed commit outcomes, which can leave FE and BE disagreeing about the usable dictionary version. I also found one insert retry authorization-state leak and one regression debug-point cleanup leak.
Review checkpoints covered: the task is a stale target/drop-race fix across Nereids insert planning and dictionary refresh; the patch is focused but still leaves retry-state and dictionary lifecycle gaps; concurrency/lifecycle risk is present in metadata drop/recreate and refresh RPC paths; persistence/replay concerns already raised in existing threads were deduplicated; test coverage exists but one regression cleanup path is unsafe. I did not run builds or tests because the review prompt says this runner is review-only.
| targetTableIf.getId(), newestTargetTableIf.getId()); | ||
| targetDatabase.getId(), targetTableIf.getId(), | ||
| newestTargetDatabase.getId(), newestTargetTableIf.getId()); | ||
| ctx.setGroupCommit(groupCommitBeforeAttempt); |
There was a problem hiding this comment.
This retry path still preserves another failed-attempt StatementContext field. The first planning attempt runs CheckPrivileges and sets stmtCtx.privChecked=true; if this branch discards that attempt because the target id/schema changed, the code restores only group-commit state before continue. The accepted retry then reuses the same StatementContext, so CheckPrivileges.rewriteRoot() returns before visiting source relations, views, CTE producers, or TVFs, leaving only the explicit target LOAD check. Please reset per-attempt privilege state here, or use a fresh per-attempt StatementContext that preserves only intentional cross-attempt fields.
|
|
||
| // commit and check the result. not modify metadata so dont need lock. | ||
| long stagedVersion = dictionary.getVersion(); | ||
| if (!commitNowVersion(ctx, dictionary)) { |
There was a problem hiding this comment.
This rollback still assumes commit failure means no BE made the staged version visible, but commitNowVersion() treats mixed results as a plain false. If one BE returns OK and a later BE fails or times out, the OK BE has already moved stagedVersion into _dict_id_to_dict_map and erased _refreshing_dict_map; the subsequent abortSpecificVersion(..., stagedVersion) cannot undo it because BE abort only removes refreshing entries. FE then decrements back to the old version and future dict_get plans carry that old version, while the committed BE requires the staged version exactly. Please handle mixed commit results explicitly, for example by not rolling FE metadata back once any BE has committed, or by adding a BE protocol that can restore the previous committed dictionary.
| } | ||
|
|
||
| InsertIntoDictionaryCommand command = new InsertIntoDictionaryCommand(baseCommand, dictionary, adaptiveLoad); | ||
| InsertIntoDictionaryCommand command = new InsertIntoDictionaryCommand( |
There was a problem hiding this comment.
The pre-commit failure paths still do not clean up staged dictionary data on BEs. A dictionary sink stages the refresh at EOS via DictionaryFactory::refresh_dict(), but if one BE has staged data and command.run() later throws, or leaves an error in ctx.getState(), the following catch/error branches only restore FE status and rethrow. They never call abortSpecificVersion(), and the later dropped-dictionary/commit-failure abort blocks are not reached. Because BE status collection and delete_dict() only look at committed dictionaries, the _refreshing_dict_map entry can remain invisible until another refresh overwrites it. Please abort the staged version in both pre-commit error paths when the selected BEs are known, using the current version for partial loads and version + 1 for full loads.
| long originalVersion = (sql "SHOW DICTIONARIES")[0][3].toLong() | ||
| def commitFailureFuture | ||
| try { | ||
| GetDebugPoint().enableDebugPointForAllFEs('DictionaryManager.dataLoad.blockBeforeCommit') |
There was a problem hiding this comment.
This cleanup is split across two blocks, so DictionaryManager.commitNowVersion.forceFailure can leak if the setup fails before commitFailureFuture.get() is reached. For example, awaitUntil(10) throws on timeout and the first finally disables only blockBeforeCommit, leaving the force-failure debug point active on every FE. Since that debug point makes every later dictionary commit return false, please disable both debug points in the same outer finally that covers their enableDebugPointForAllFEs calls.
2 participants
What problem does this PR solve?
Problem Summary:
A dictionary refresh can retain a
Dictionaryobject whileDROP DATABASEremoves its owner database. The old path repeatedly recovered the database throughdictionary.getDatabase()and checked existence by database/name, so it could either dereference a null database or accept a newly created dictionary with the same name.This change establishes one insert-target invariant: resolve the database and table/dictionary together, then carry that exact pair through authorization, sink binding, retry validation, and executor construction. Dictionary loads additionally validate canonical dictionary identity by ID before planning and after execution. Database drop now removes dictionary IDs from the canonical map, matching replay behavior and preventing same-name ABA.
Ordinary internal-table inserts keep their existing table read-lock/write-lock and transaction ordering. The paired target only closes the pre-lock metadata lookup window and adds database ID to retry validation.
Release note
Fix concurrent dictionary refresh and database drop returning an internal null-pointer error or accepting stale dictionary metadata.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?