-
Notifications
You must be signed in to change notification settings - Fork 5k
Feat/336/wal-recover #35200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.3.6
Are you sure you want to change the base?
Feat/336/wal-recover #35200
Changes from 9 commits
50b5164
4eeeb7b
8be4d82
1f23a8a
8d8b4f5
dbf6f82
281ce7b
224b376
f77e404
4fb8017
5567cc6
0967b0b
93d55a5
c98c398
de6a4de
e970734
7fb598d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -303,6 +303,8 @@ const char* syncStr(ESyncState state); | |||
|
|
||||
| int32_t syncNodeGetConfig(int64_t rid, SSyncCfg* cfg); | ||||
|
|
||||
| int32_t syncNotifyWalTruncated(int32_t vgId, int64_t truncatedVer); | ||||
|
|
||||
|
Comment on lines
+306
to
+307
|
||||
| int32_t syncNotifyWalTruncated(int32_t vgId, int64_t truncatedVer); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,7 +166,7 @@ int32_t walInit(stopDnodeFn stopDnode); | |
| void walCleanUp(); | ||
|
|
||
| // handle open and ctl | ||
| SWal *walOpen(const char *path, SWalCfg *pCfg); | ||
| SWal *walOpen(const char *path, SWalCfg *pCfg, int32_t replica); | ||
|
xiao-77 marked this conversation as resolved.
|
||
| int32_t walAlter(SWal *, SWalCfg *pCfg); | ||
| int32_t walPersist(SWal *); | ||
| void walClose(SWal *); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -356,7 +356,7 @@ bool tsStartUdfd = true; | |||||||
| // wal | ||||||||
| int64_t tsWalFsyncDataSizeLimit = (100 * 1024 * 1024L); | ||||||||
| bool tsWalForceRepair = 0; | ||||||||
| bool tsWalDeleteOnCorruption = false; | ||||||||
| int32_t tsWalRecoveryPolicy = 0; // Default: refuse to start for single replica | ||||||||
|
|
||||||||
| // ttl | ||||||||
| bool tsTtlChangeOnWrite = false; // if true, ttl delete time changes on last write | ||||||||
|
|
@@ -981,7 +981,7 @@ static int32_t taosAddServerCfg(SConfig *pCfg) { | |||||||
| TAOS_CHECK_RETURN(cfgAddInt64(pCfg, "syncApplyQueueSize", tsSyncApplyQueueSize, 32, 2048, CFG_SCOPE_SERVER, CFG_DYN_SERVER,CFG_CATEGORY_GLOBAL)); | ||||||||
| TAOS_CHECK_RETURN(cfgAddInt32(pCfg, "syncRoutineReportInterval", tsRoutineReportInterval, 5, 600, CFG_SCOPE_SERVER, CFG_DYN_SERVER,CFG_CATEGORY_LOCAL)); | ||||||||
| TAOS_CHECK_RETURN(cfgAddBool(pCfg, "syncLogHeartbeat", tsSyncLogHeartbeat, CFG_SCOPE_SERVER, CFG_DYN_SERVER,CFG_CATEGORY_LOCAL)); | ||||||||
| TAOS_CHECK_RETURN(cfgAddBool(pCfg, "walDeleteOnCorruption", tsWalDeleteOnCorruption, CFG_SCOPE_SERVER, CFG_DYN_NONE,CFG_CATEGORY_LOCAL)); | ||||||||
| TAOS_CHECK_RETURN(cfgAddInt32(pCfg, "walRecoveryPolicy", tsWalRecoveryPolicy, 0, 1, CFG_SCOPE_SERVER, CFG_DYN_NONE, CFG_CATEGORY_LOCAL)); | ||||||||
|
||||||||
| TAOS_CHECK_RETURN(cfgAddInt32(pCfg, "walRecoveryPolicy", tsWalRecoveryPolicy, 0, 1, CFG_SCOPE_SERVER, CFG_DYN_NONE, CFG_CATEGORY_LOCAL)); | |
| TAOS_CHECK_RETURN(cfgAddInt32(pCfg, "walRecoveryPolicy", tsWalRecoveryPolicy, 0, 1, CFG_SCOPE_SERVER, CFG_DYN_NONE, CFG_CATEGORY_LOCAL)); | |
| TAOS_CHECK_RETURN(cfgAddInt32(pCfg, "walDeleteOnCorruption", tsWalRecoveryPolicy, 0, 1, CFG_SCOPE_SERVER, CFG_DYN_NONE, CFG_CATEGORY_LOCAL)); |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4090,3 +4090,8 @@ bool syncNodeCanChange(SSyncNode* pSyncNode) { | |||||||||||
| return true; | ||||||||||||
| } | ||||||||||||
| #endif | ||||||||||||
|
|
||||||||||||
| int32_t syncNotifyWalTruncated(int32_t vgId, int64_t truncatedVer) { | ||||||||||||
| sInfo("vgId:%d, notified sync module: WAL truncated to ver:%" PRId64, vgId, truncatedVer); | ||||||||||||
| return TSDB_CODE_SUCCESS; | ||||||||||||
| } | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation of
|
||||||||||||
| int32_t syncNotifyWalTruncated(int32_t vgId, int64_t truncatedVer) { | |
| sInfo("vgId:%d, notified sync module: WAL truncated to ver:%" PRId64, vgId, truncatedVer); | |
| return TSDB_CODE_SUCCESS; | |
| } |
Copilot
AI
Apr 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syncNotifyWalTruncated() is added as a public/internal API but is not called anywhere in this PR, and currently only logs a message without informing any SSyncNode state. This doesn’t match the PR description (“Call notification after three-replica recovery / Enable Raft to detect and sync missing logs”). Either wire this into the WAL truncation/recovery path (and have it trigger whatever raft re-sync behavior is required) or keep it internal until there’s an actual caller/behavior.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -431,7 +431,69 @@ static int32_t walRenameCorruptedDir(SWal* pWal) { | |||||||||||||||||||||||||
| TAOS_RETURN(code); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| static int32_t walRenameCorruptedDir(SWal* pWal) __attribute__((unused)); |
Copilot
AI
Apr 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code is declared in walTruncateCorruptedFiles() but never used. This will trigger an unused-variable warning on stricter builds; please remove it or use it for error propagation.
| int32_t code = TSDB_CODE_SUCCESS; |
Copilot
AI
Apr 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description mentions notifying sync/Raft after three-replica recovery, but walTruncateCorruptedFiles() (the new truncation/recovery path) does not call syncNotifyWalTruncated() or any equivalent hook when replica >= 3. If Raft relies on this notification to resync missing logs, add the call here (or in the caller) at the point where truncation is committed.
Copilot
AI
Apr 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
walTruncateCorruptedFiles() doesn't validate fileIdx before calling taosArrayGet(pWal->fileInfoSet, fileIdx) and before computing the remove batch length (size - fileIdx). If fileIdx is negative or >= array size, this can lead to invalid memory access or a huge batch removal due to underflow. Add defensive checks at the start of this function and return an appropriate error when fileIdx is out of range.
Copilot
AI
Apr 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
walTruncateCorruptedFiles() assumes the corrupted file itself has already been truncated by walScanLogGetLastVer, but walScanLogGetLastVer() intentionally skips truncation when fileIdx == sz - 1 (last file). That means corruption in the last WAL segment may not actually be removed, and the node may keep re-entering repair logic (or fail later) even though walTruncateCorruptedFiles() returns success. Consider truncating the corrupted file here as well (or adjusting walScanLogGetLastVer/repair flow) so last-segment corruption is recoverable under the intended policy/replica behavior.
Copilot
AI
Apr 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
walTruncateCorruptedFiles() currently deletes files starting at fileIdx + 1, but it’s also called from walLogEntriesComplete() when a non-consecutive firstVer is detected. In that gap/mismatch case, keeping the fileIdx file leaves the WAL still inconsistent (the gap remains). Consider deleting from fileIdx (or otherwise handling the gap case separately) and then recomputing vers.lastVer/writeCur to reflect the remaining prefix.
Copilot
AI
Apr 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in log message: "snaphot" should be "snapshot".
| ", snaphot index:%" PRId64 ", fileIdx:%d", | |
| ", snapshot index:%" PRId64 ", fileIdx:%d", |
Copilot
AI
Apr 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in log message: snaphot index should be snapshot index for consistency/searchability in logs.
| ", snaphot index:%" PRId64 ", fileIdx:%d", | |
| ", snapshot index:%" PRId64 ", fileIdx:%d", |
Copilot
AI
Apr 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
walTruncateCorruptedFiles() assumes fileIdx is a valid index into pWal->fileInfoSet (taosArrayGet/taosArrayRemoveBatch). In walLogEntriesComplete(), fileIdx can become == sz when the loop completes without breaking, which would make taosArrayGet out-of-bounds and can crash. Add explicit range checks (fileIdx >= 0 && fileIdx < taosArrayGetSize(...)) before using fileIdx, and decide a safe truncation point (or return an error) when the inconsistency is only in meta/vers rather than a specific corrupted file.
| TAOS_RETURN(walTruncateCorruptedFiles(pWal, fileIdx, replica)); | |
| if (fileIdx >= 0 && fileIdx < sz) { | |
| TAOS_RETURN(walTruncateCorruptedFiles(pWal, fileIdx, replica)); | |
| } | |
| wError("vgId:%d, WAL metadata/version inconsistency detected without a valid corrupted file index, " | |
| "skip truncation, fileCnt:%d, fileIdx:%d", | |
| pWal->cfg.vgId, sz, fileIdx); | |
| TAOS_RETURN(TSDB_CODE_WAL_FILE_CORRUPTED); |
Copilot
AI
Apr 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
walLogEntriesComplete() can call walTruncateCorruptedFiles(pWal, fileIdx, ...) with fileIdx == sz when the while loop finishes without hitting break but complete is still false. That makes taosArrayGet(pWal->fileInfoSet, fileIdx) out-of-bounds inside walTruncateCorruptedFiles, which can crash during WAL open/repair. Clamp fileIdx to a valid range (e.g., sz - 1) or handle the fileIdx >= sz case explicitly before calling the truncation routine.
Copilot
AI
Apr 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After walLogEntriesComplete() calls walTruncateCorruptedFiles(), the code returns success without updating pWal->vers/totSize or persisting meta. If any files are removed, the in-memory and on-disk meta can remain stale, and subsequent components may observe an incorrect lastVer. Ensure the truncation path recomputes versions and saves meta (or returns a code that forces the caller to persist the repaired meta).
Copilot
AI
Apr 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
walTruncated and truncatedVer are declared but never used, which is dead code and can trigger -Wunused-variable warnings in some build configurations. Either remove them or use them for the intended truncation notification/metadata tracking (e.g., record truncatedVer and propagate it to the sync module).
| bool walTruncated = false; | |
| int64_t truncatedVer = -1; |
Copilot
AI
Apr 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In walCheckAndRepairMeta(), pFileInfo->lastVer is only updated when lastVer >= 0. For an empty first log file where firstVer == 0, the computed lastVer becomes -1 and this branch is skipped, leaving pFileInfo->lastVer potentially stale. Update pFileInfo->lastVer for the empty-file case as well (even when lastVer < 0).
| if (code == TSDB_CODE_SUCCESS && lastVer >= 0) { | |
| wInfo("vgId:%d, repaired file %s, last index:%" PRId64 ", fileSize:%" PRId64 ", fileSize in meta:%" PRId64, | |
| pWal->cfg.vgId, fnameStr, lastVer, fileSize, pFileInfo->fileSize); | |
| // update lastVer | |
| if (code == TSDB_CODE_SUCCESS) { | |
| if (lastVer >= 0) { | |
| wInfo("vgId:%d, repaired file %s, last index:%" PRId64 ", fileSize:%" PRId64 ", fileSize in meta:%" PRId64, | |
| pWal->cfg.vgId, fnameStr, lastVer, fileSize, pFileInfo->fileSize); | |
| } | |
| // update lastVer, including the empty-file case where lastVer == firstVer - 1 |
Copilot
AI
Apr 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the lastVer<0 && code!=TSDB_CODE_WAL_LOG_NOT_EXIST branch, walTruncateCorruptedFiles() mutates pWal->fileInfoSet (removes entries starting at fileIdx). After that, pFileInfo still points to the removed element, but the code can still enter the update block and write pFileInfo->lastVer / use pFileInfo->fileSize, which becomes invalid memory access. Also, totSize may already include sizes of files that just got deleted. After truncation, avoid using the old pFileInfo pointer and recompute totSize/versions from the remaining fileInfoSet (e.g., restart the scan or break and rebuild totals).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Header comment says
walRecoveryPolicy"only affects single replica", but the new logic checks it for allreplica != 3cases (so it would also affect replica=2 if that configuration exists). Please align the comment (and possibly the config semantics) with actual behavior to avoid misconfiguration.