[BugFix] use db lock in follower or cbo_use_db_lock is true #40725
Conversation
Signed-off-by: packy92 <wangchao@starrocks.com>
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| // 3. cbo_use_lock_db = false | ||
| return isOnlyOlapTable | ||
| && GlobalStateMgr.getCurrentState().isLeader() | ||
| && !session.getSessionVariable().isCboUseDBLock(); | ||
| } | ||
|
|
||
| private static ExecPlan createQueryPlan(Relation relation, |
There was a problem hiding this comment.
The most risky bug in this code is:
Potential deadlock or incorrect lock release when isLockFree condition changes after unlocking.
You can modify the code like this:
boolean lockReleased = false;
try {
if (stmt instanceof QueryStatement) {
QueryStatement queryStmt = (QueryStatement) stmt;
resultSinkType = queryStmt.hasOutFileClause() ? TResultSinkType.FILE : resultSinkType;
ExecPlan plan;
// Check isLockFree before releasing the lock
boolean lockFreeCondition = isLockFree(isOnlyOlapTableQueries, session);
// Ensure that we only unlock and set needWholePhaseLock once based on the lock-free condition
if (lockFreeCondition) {
unLock(locker, dbs);
lockReleased = true;
needWholePhaseLock = false;
plan = createQueryPlanWithReTry(queryStmt, session, resultSinkType);
} else {
plan = createQueryPlan(queryStmt.getQueryRelation(), session, resultSinkType);
}
setOutfileSink(queryStmt, plan);
return plan;
} else if (stmt instanceof InsertStmt) {
// Handle other statement types...
}
// Continuing with the rest of the method...
} finally {
// Ensure locks are properly released in case of exceptions before the manual unlock
if (!lockReleased) {
unLock(locker, dbs);
}
}Explanation: The main issue comes from changing the needWholePhaseLock condition and releasing locks based on runtime conditions which might change unexpectedly or could be subject to race conditions. It's crucial to ensure locks are correctly managed across all execution paths, including error handling. Adding a try-finally block ensures that resources are appropriately cleaned up even when exceptions occur. This modification doesn't directly solve concurrency issues but rather ensures that locks get released consistently, avoiding potential deadlocks or prematurely releasing locks. Further investigation might be needed to address the thread safety of isLockFree checks and their impacts comprehensively.
Signed-off-by: packy92 <wangchao@starrocks.com>
packy92
force-pushed
the
main_cp_db_lock
branch
from
b7ea0d5
to
bd11d0c
Compare
|
[FE Incremental Coverage Report]✅ pass : 12 / 12 (100.00%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
@Mergifyio backport branch-3.1 |
|
@Mergifyio backport branch-3.0 |
|
@Mergifyio backport branch-2.5 |
✅ Backports have been created
|
✅ Backports have been created
|
✅ Backports have been created
|
Signed-off-by: packy92 <wangchao@starrocks.com> (cherry picked from commit 8fae880) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/sql/StatementPlanner.java
Signed-off-by: packy92 <wangchao@starrocks.com> (cherry picked from commit 8fae880) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/sql/StatementPlanner.java
Signed-off-by: packy92 <wangchao@starrocks.com> (cherry picked from commit 8fae880) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/sql/StatementPlanner.java
Why I'm doing:
What I'm doing:
use db lock in follower or cbo_use_db_lock is true
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: