[fix](dictionary) stabilize insert target during database drop#65476
Draft
zclllyybb wants to merge 1 commit into
Draft
[fix](dictionary) stabilize insert target during database drop#65476zclllyybb wants to merge 1 commit into
zclllyybb wants to merge 1 commit 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
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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)) { |
Contributor
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); |
Contributor
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.
Contributor
TPC-H: Total hot run time: 29123 ms |
Contributor
TPC-DS: Total hot run time: 180359 ms |
Contributor
ClickBench: Total hot run time: 25.03 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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?