fix: handle read-while-write race on online redo LWN blocks#10
Merged
Conversation
When reading online redo logs, Oracle may be in the process of writing a block. The block header (sequence, CRC) can be valid while the LWN vld flag or record size fields are not yet complete. Previously OLR crashed with ERROR 50051 or 50053. For online redo (group != 0): treat incomplete LWN blocks as "not ready yet" — break and let the Reader re-read the block later. Track how long we've been waiting and timeout after 30s with a clear error. For archived redo (group == 0): keep existing behavior (throw immediately) since archives are fully written. Confirmed by hex-dump: crash block showed valid vld=0x05 after OLR crashed, proving Oracle completed the write between OLR's read and the file copy. Reproduced reliably with Swingbench under load.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/parser/Parser.cpp`:
- Around line 1366-1367: The local variable lwnMidWriteStart and its constexpr
LWN_MID_WRITE_TIMEOUT_S must be converted into Parser state so the mid-write
deadline persists across yields/resumes and is not reinitialized to 0 on every
parse() entry; move lwnMidWriteStart into the Parser class (e.g. as a member
like m_lwnMidWriteStart) and keep LWN_MID_WRITE_TIMEOUT_S as a constant, then
update parse() to set the start time only the first time the parser begins
waiting for the mid-write and to check the timeout against the persistent member
on subsequent resumes; additionally clear/reset this member when the exact
pending header or record is recovered/validated (i.e. when the pending LWN
header becomes valid or a recovered record-size wait completes) so old deadlines
cannot leak into later waits within the same LWN group.
- Around line 1474-1485: The retry logic currently does a local `break` which
still lets the outer loop advance `currentBlock` and `confirmedBufferStart` and
leaves `lwnAllocated` incremented; instead, stop advancing the block and bubble
the retry to the outer block loop: when detecting a mid-write (the branch with
lwnMidWriteStart and LWN_MID_WRITE_TIMEOUT_S) set a retry flag (or use continue
on the outer loop) so you do not increment `currentBlock` or
`confirmedBufferStart`, and roll back or defer any `lwnAllocated`/chunk
allocation performed earlier (undo increment of lwnAllocated or postpone
allocation until recordSize is validated) to avoid exhausting MAX_LWN_CHUNKS;
ensure the code path that currently does `break` instead either continues the
outer block-processing loop or returns to re-read the same block so the Reader
will re-fetch the same block for retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f957a12f-b19e-43ce-a614-37db9705785a
📒 Files selected for processing (1)
src/parser/Parser.cpp
src/parser/Parser.cpp
Outdated
Comment on lines
+1366
to
+1367
| time_t lwnMidWriteStart = 0; | ||
| static constexpr time_t LWN_MID_WRITE_TIMEOUT_S = 30; |
There was a problem hiding this comment.
Persist the mid-write timer across resumes, and reset it on record recovery too.
If this parser yields while waiting, lwnMidWriteStart is rebuilt as 0 on the next parse() call, so the 30s budget never accumulates. It's also only cleared on a valid LWN header here, so a recovered record-size wait can leak its old deadline into a later wait in the same LWN group. This needs to live in Parser state and be cleared when the exact pending header/record becomes valid again.
Also applies to: 1406-1406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parser/Parser.cpp` around lines 1366 - 1367, The local variable
lwnMidWriteStart and its constexpr LWN_MID_WRITE_TIMEOUT_S must be converted
into Parser state so the mid-write deadline persists across yields/resumes and
is not reinitialized to 0 on every parse() entry; move lwnMidWriteStart into the
Parser class (e.g. as a member like m_lwnMidWriteStart) and keep
LWN_MID_WRITE_TIMEOUT_S as a constant, then update parse() to set the start time
only the first time the parser begins waiting for the mid-write and to check the
timeout against the persistent member on subsequent resumes; additionally
clear/reset this member when the exact pending header or record is
recovered/validated (i.e. when the pending LWN header becomes valid or a
recovered record-size wait completes) so old deadlines cannot leak into later
waits within the same LWN group.
rophy
force-pushed
the
fix/online-redo-lwn-race
branch
from
39c0627 to
5d4780e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/reader/Reader.h (1)
140-140: MarkreReadBlockas[[nodiscard]]to prevent ignored read results.At Line 140, this method’s return value is operationally important in retry logic. Marking it
[[nodiscard]]helps catch accidental ignored failures at compile time.Suggested API tweak
- int reReadBlock(uint8_t* buffer, uint64_t offset, uint size); + [[nodiscard]] int reReadBlock(uint8_t* buffer, uint64_t offset, uint size);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reader/Reader.h` at line 140, The declaration of reReadBlock(uint8_t* buffer, uint64_t offset, uint size) should be marked [[nodiscard]] so callers cannot accidentally ignore its return value; update the declaration in Reader.h to [[nodiscard]] int reReadBlock(...), and make the corresponding change to its definition/override(s) in the implementation files (e.g., Reader.cpp and any subclasses) so the attribute is present everywhere the symbol is defined to ensure compile-time warnings when return values are ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/reader/Reader.cpp`:
- Around line 763-765: Reader::reReadBlock currently forwards to redoRead and
returns whatever byte count redoRead returns, but callers expect a full-block
re-read; a short read leaves stale bytes in buffer. Change reReadBlock (the
method named reReadBlock) to call redoRead(buffer, offset, size), check the
returned byte count, and if it is not equal to size treat that as a failure
(return an error code such as -1 or another existing failure sentinel used by
the codebase) so callers can detect and retry/abort; do not let partial-byte
counts propagate as success.
---
Nitpick comments:
In `@src/reader/Reader.h`:
- Line 140: The declaration of reReadBlock(uint8_t* buffer, uint64_t offset,
uint size) should be marked [[nodiscard]] so callers cannot accidentally ignore
its return value; update the declaration in Reader.h to [[nodiscard]] int
reReadBlock(...), and make the corresponding change to its
definition/override(s) in the implementation files (e.g., Reader.cpp and any
subclasses) so the attribute is present everywhere the symbol is defined to
ensure compile-time warnings when return values are ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53b8143b-4cb0-4755-9eac-6600609ed836
📒 Files selected for processing (3)
src/parser/Parser.cppsrc/reader/Reader.cppsrc/reader/Reader.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/parser/Parser.cpp
rophy
force-pushed
the
fix/online-redo-lwn-race
branch
2 times, most recently
from
af93fdb to
4617808
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/parser/Parser.cpp`:
- Around line 1423-1428: The reReadBlock calls in the group retry branches
(triggered when group != 0 with lwnRereadAttempts++) can silently overwrite
redoBuffer bytes with raw data and return only >0/-1; instead, after
reader->reReadBlock(...) succeeds you must revalidate the freshly-read buffer
with Reader::checkBlockHeader (the same validation used on the normal path) and
handle special results like OVERWRITTEN or EMPTY by retrying (e.g., goto
lwnBlockRetry or increment attempts) or failing appropriately; update each
occurrence (the blocks that call reader->reReadBlock and then continue parsing —
e.g., the branch containing lwnRereadAttempts/MAX_REREAD_ATTEMPTS, and the
similar sections at the other noted locations) to run checkBlockHeader on the
buffer and conditionally proceed only when the header is valid.
- Around line 1498-1513: The loop can mark resolved=true even when recordSize4
becomes 0 after rereads, which leads to creating a zero-length LwnMember; update
the retry/validation logic around recordSize4 (the value filled by ctx->read32)
so you only set resolved = true and break when recordSize4 > 0 and the size
check (((*recordSize + sizeof(LwnMember) + recordSize4 + 7) & 0xFFFFFFF8) <=
Ctx::MEMORY_CHUNK_SIZE_MB * 1024 * 1024) passes; if recordSize4 == 0 continue
retrying instead of breaking, and keep the existing RedoLogException paths
(50053) for timeouts/oversize unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07fe9cd8-ae73-4e14-a5d5-a3593e2c74b9
📒 Files selected for processing (4)
src/parser/Parser.cppsrc/reader/Reader.cppsrc/reader/Reader.htests/sql/environments/rac/debezium/perf/validator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/reader/Reader.h
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/parser/Parser.cpp (2)
1498-1503:⚠️ Potential issue | 🔴 CriticalGuard against
recordSize4 == 0to avoid constructing zero-length records.After the reread at line 1498,
recordSize4could become 0 (e.g., if Oracle hasn't written the size field yet). The condition at line 1499 only checks if the size fits within the memory chunk, which is true whenrecordSize4 == 0. This causes:
resolved = trueand the loop exits- An
LwnMemberis created withsize = 0(line 1517)- Later in
analyzeLwn(line 130-133), this triggers error 50046 or produces undefined behavior🐛 Proposed fix
recordSize4 = (static_cast<uint64_t>(ctx->read32(redoBlock + blockOffset)) + 3U) & 0xFFFFFFFC; - if (((*recordSize + sizeof(LwnMember) + recordSize4 + 7) & 0xFFFFFFF8) <= Ctx::MEMORY_CHUNK_SIZE_MB * 1024 * 1024) { + if (recordSize4 > 0 && + ((*recordSize + sizeof(LwnMember) + recordSize4 + 7) & 0xFFFFFFF8) <= Ctx::MEMORY_CHUNK_SIZE_MB * 1024 * 1024) { resolved = true; break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parser/Parser.cpp` around lines 1498 - 1503, After re-reading `recordSize4` you must guard against the case where `recordSize4 == 0` before accepting `resolved = true`; update the conditional that currently tests the fit against `Ctx::MEMORY_CHUNK_SIZE_MB * 1024 * 1024` to also require `recordSize4 != 0` (so you don't create an `LwnMember` with zero size), ensuring any logic that builds an `LwnMember` (the code path that uses `*recordSize`, `sizeof(LwnMember)`, and `recordSize4`) only proceeds when `recordSize4` is non-zero; this prevents the zero-length record reaching `analyzeLwn`.
1423-1428:⚠️ Potential issue | 🟠 MajorMissing block header validation after reread may cause silent data corruption.
After
reReadBlock()succeeds, the code immediately jumps tolwnBlockRetryand continues parsing without revalidating the block header. The normal read path usesReader::checkBlockHeader(per context snippet atsrc/reader/Reader.cpp:110-169) to verify CRC, sequence, and detect OVERWRITTEN/EMPTY states. These retry paths bypass that validation.If Oracle overwrites the block with a different sequence during the 30-second retry window, the parser could process stale or corrupted data without detecting the overwrite.
🛡️ Suggested fix pattern
if (reader->reReadBlock(reader->redoBufferList[redoBufferNum] + redoBufferPos, static_cast<uint64_t>(currentBlock) * reader->getBlockSize(), - reader->getBlockSize()) > 0) + reader->getBlockSize()) > 0) { + // Revalidate block header after reread + auto headerCheck = reader->checkBlockHeader( + reader->redoBufferList[redoBufferNum] + redoBufferPos, currentBlock, false); + if (headerCheck == Reader::REDO_CODE::OK) goto lwnBlockRetry; + else if (headerCheck == Reader::REDO_CODE::OVERWRITTEN) + throw RedoLogException(50056, "block overwritten during retry at offset: " + + confirmedBufferStart.toString()); + // else continue retrying + }This concern also applies to the retry blocks at lines 1435-1440, 1454-1459, and 1491-1496.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parser/Parser.cpp` around lines 1423 - 1428, After a successful reReadBlock call the code currently jumps to lwnBlockRetry without revalidating the block header; update the retry paths so that after reader->reReadBlock(...) returns > 0 you call Reader::checkBlockHeader (or the same header validation used in the normal read path) on the buffer at reader->redoBufferList[redoBufferNum] + redoBufferPos (using currentBlock and reader->getBlockSize() as before), and only jump to lwnBlockRetry if checkBlockHeader passes; apply the same addition to the other retry blocks that use lwnRereadAttempts/reReadBlock (the blocks that mirror this pattern) so CRC/sequence/OVERWRITTEN/EMPTY checks are enforced before resuming parsing.
🧹 Nitpick comments (1)
src/parser/Parser.cpp (1)
1401-1403: Consider consolidating duplicateMAX_REREAD_ATTEMPTSdefinitions.The constant
MAX_REREAD_ATTEMPTS = 300is defined twice: once at line 1401 (for LWN header retries) and again at line 1489 (for record size retries). While this works due to different scopes, consolidating to a single class-level or file-level constant would improve maintainability.🔧 Suggested refactor
+namespace { + constexpr int MAX_REREAD_ATTEMPTS = 300; // 300 * 100ms = 30s +} + // ... in parse() function ... - static constexpr int MAX_REREAD_ATTEMPTS = 300; // 300 * 100ms = 30s int lwnRereadAttempts = 0; // ... - static constexpr int MAX_REREAD_ATTEMPTS = 300; bool resolved = false;Also applies to: 1489-1489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parser/Parser.cpp` around lines 1401 - 1403, Consolidate the duplicate MAX_REREAD_ATTEMPTS definitions by promoting it from the local blocks to a single shared constant (e.g., a static constexpr in the Parser class or a file-level anonymous-namespace constant) and remove the local definitions; update uses in the LWN retry logic (lwnRereadAttempts and the lwnBlockRetry loop) and the record-size retry logic (recordSizeRereadAttempts) to reference that single constant so both retry loops use the same MAX_REREAD_ATTEMPTS symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/parser/Parser.cpp`:
- Around line 1498-1503: After re-reading `recordSize4` you must guard against
the case where `recordSize4 == 0` before accepting `resolved = true`; update the
conditional that currently tests the fit against `Ctx::MEMORY_CHUNK_SIZE_MB *
1024 * 1024` to also require `recordSize4 != 0` (so you don't create an
`LwnMember` with zero size), ensuring any logic that builds an `LwnMember` (the
code path that uses `*recordSize`, `sizeof(LwnMember)`, and `recordSize4`) only
proceeds when `recordSize4` is non-zero; this prevents the zero-length record
reaching `analyzeLwn`.
- Around line 1423-1428: After a successful reReadBlock call the code currently
jumps to lwnBlockRetry without revalidating the block header; update the retry
paths so that after reader->reReadBlock(...) returns > 0 you call
Reader::checkBlockHeader (or the same header validation used in the normal read
path) on the buffer at reader->redoBufferList[redoBufferNum] + redoBufferPos
(using currentBlock and reader->getBlockSize() as before), and only jump to
lwnBlockRetry if checkBlockHeader passes; apply the same addition to the other
retry blocks that use lwnRereadAttempts/reReadBlock (the blocks that mirror this
pattern) so CRC/sequence/OVERWRITTEN/EMPTY checks are enforced before resuming
parsing.
---
Nitpick comments:
In `@src/parser/Parser.cpp`:
- Around line 1401-1403: Consolidate the duplicate MAX_REREAD_ATTEMPTS
definitions by promoting it from the local blocks to a single shared constant
(e.g., a static constexpr in the Parser class or a file-level
anonymous-namespace constant) and remove the local definitions; update uses in
the LWN retry logic (lwnRereadAttempts and the lwnBlockRetry loop) and the
record-size retry logic (recordSizeRereadAttempts) to reference that single
constant so both retry loops use the same MAX_REREAD_ATTEMPTS symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3599b82-7ef3-4374-83e0-4185951dedfc
📒 Files selected for processing (4)
src/parser/Parser.cppsrc/reader/Reader.cppsrc/reader/Reader.htests/sql/environments/rac/debezium/perf/validator.py
✅ Files skipped from review due to trivial changes (1)
- tests/sql/environments/rac/debezium/perf/validator.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/reader/Reader.cpp
- src/reader/Reader.h
When reading online redo logs, Oracle may be writing a block that has a valid header/CRC but incomplete LWN content. The Reader's buffer caches the stale copy. For online redo (group != 0): when LWN validation fails (50049, 50050, 50051) or record size is invalid (50053), re-read the block from disk via Reader::reReadAndValidate() — which does pread + checkBlockHeader to verify the re-read is valid (correct seq, CRC, not OVERWRITTEN). Retry up to 300 times (30s). On OVERWRITTEN, stop gracefully. For archived redo (group == 0): unchanged, throw immediately. Also excludes BENCHMARK_RESULTS from validator (Swingbench internal table with timing mismatch at test end). Validated: 517,995 events matched under Swingbench 4-user load, 126/126 redo fixture tests pass, 54/54 RAC SQL e2e pass.
rophy
force-pushed
the
fix/online-redo-lwn-race
branch
from
4617808 to
b534546
Compare
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
1 participant
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.
Summary
Fixes #8. OLR crashed with ERROR 50051/50053 when reading online redo logs
under heavy write load due to a read-while-write race on LWN block headers.
Root Cause
Oracle writes redo blocks in stages. OLR can read a block where the
header/CRC is valid but the LWN
vldflag or record size fields arenot yet complete. Previously OLR threw a fatal error and crashed.
Confirmed by hex-dump: crash block showed
vld=0x05(valid LWN)after OLR crashed — Oracle completed the write between OLR's read and
the file copy.
Fix
For online redo (
group != 0): treat incomplete LWN blocks as "notready yet" — break and let the Reader re-read the block later. Timeout
after 30s with a clear error message.
For archived redo (
group == 0): unchanged — throw immediately sincearchives are fully written.
Test plan
Summary by CodeRabbit