Conversation
INSERT 성능 개선을 위해 WAL append 경로를 파일 write_all 기반에서 mmap segment append 방식으로 변경했다. 요청 처리 중에는 WAL frame을 mmap에 복사하고, 기본 10초 background flush loop와 checkpoint/rotation 시점에만 mmap flush와 sync_data를 수행하도록 했다. mmap segment는 wal_segment_size만큼 preallocate하며, 재시작 시 preallocated zero tail을 건너뛰고 마지막 append offset을 복원한다. 테이블 row append는 즉시 파일에 쓰지 않고 메모리 row buffer에 쌓은 뒤 background flush loop, buffer pressure, read/update/delete 직전 flush에서 디스크에 반영하도록 바꿨다. INSERT append 경로에서는 불필요한 row_storage_lock 직렬화를 제거하고, WAL payload도 InsertQuery 전체가 아니라 실제 table/row 데이터 중심으로 줄였다. 관련 WAL/row flush 테스트와 내구성 지침 문서를 갱신했다.
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughWAL은 mmap 기반 세그먼트 기록과 row 버퍼링/복구 흐름으로 바뀌었고, pgwire는 Bind 파라미터를 보존해 Bind 시점에 파싱하도록 확장됐다. 문서와 테스트도 새 WAL 인코딩, 내구성 정책, 복구 경로를 반영하도록 갱신됐다. ChangesWAL mmap 전환 및 Row Buffer 도입
pgwire Bind 파라미터 지연 파싱
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant WALManager
participant WALSegmentWriter
participant Disk
WALManager->>WALManager: rotate_if_needed(frame.len())
WALManager->>WALSegmentWriter: append_frame_to_mmap(frame)
WALManager->>WALSegmentWriter: sync_current_file()
WALSegmentWriter->>Disk: mmap flush + sync_data
sequenceDiagram
participant Client
participant Connection
participant PreparedStatement
Client->>Connection: Parse(query, parameter_types)
Connection->>PreparedStatement: raw_query 저장 또는 statement 생성
Client->>Connection: Bind(parameters)
Connection->>Connection: parse_parameterized_insert 또는 bind_query_parameters + parse_statement
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
RRDB pgwire Bind 메시지가 parameter 값을 읽고 버리지 않도록 수정했다. placeholder가 포함된 prepared statement는 Parse 단계에서 raw query로 보관하고, Bind 단계에서 parameter를 적용한다. 벤치마크에서 쓰는 INSERT INTO ... VALUES (, ) 형태는 tokenizer/parser를 다시 태우지 않고 직접 InsertQuery AST로 변환하는 fast path를 추가했다. 일반 parameterized SQL은 기존 파서 경로를 유지하되, bind 값을 SQL literal로 안전하게 치환하는 fallback을 둔다. Bind parameter decode와 parameterized INSERT fast path 테스트를 추가했다.
Owner
Author
|
✅ Total Coverage: 64.47% |
pgwire RRDB engine 테스트가 고정된 target/<test_name> 경로를 공유해 병렬 또는 반복 실행 시 서로의 테스트 데이터를 지울 수 있었다. 테스트 helper가 현재 테스트 바이너리명과 timestamp를 포함한 고유 base path를 사용하도록 바꿔 create_portal_does_not_execute_until_execute_is_called 테스트의 count 검증이 외부 테스트 상태에 영향받지 않게 했다.
Owner
Author
|
✅ Total Coverage: 65.02% |
기존 row write 최적화는 INSERT를 메모리에 버퍼링했지만, SELECT/UPDATE/DELETE 경로가 dirty rows를 읽기 전에 flush_row_buffers를 호출해 세그먼트 파일로 강제 반영했다. 이 때문에 PostgreSQL식 dirty buffer read가 되지 않고, 읽기/수정 쿼리가 디스크 write를 유발해 INSERT 성능 개선 효과가 사라질 수 있었다. RowBufferPool을 추가해 세그먼트별 persisted rows, pending append rows, rewrite dirty 상태를 관리하도록 바꿨다. full_scan은 buffer pool의 cached rows를 우선 사용하고, 캐시가 없을 때만 디스크 세그먼트를 읽어 pending rows와 병합한다. update/delete도 flush 없이 buffer pool rows를 수정하고 rewrite dirty로 표시한다. flush 경로는 append와 rewrite를 구분해 디스크에 반영하며, rewrite dirty 이후 추가 append가 들어와도 flush 시 rows가 유실되지 않도록 보장한다. 관련 scan 테스트로 dirty read, buffered update/delete, rewrite 이후 append 보존을 검증했다.
myyrakle
marked this pull request as ready for review
July 6, 2026 15:03
Owner
Author
|
✅ Total Coverage: 65.92% |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engine/wal/manager/mod.rs (1)
200-219: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
flush()는 fsync와 checkpoint를 분리해야 합니다.
spawn_wal_flush_loop가flush()를 호출하는데, 이 구현은 활성 세그먼트가 있으면 항상checkpoint()를 수행해 세그먼트를 닫습니다. 그래서 10초 주기 루프가 데이터량과 무관하게 WAL 파일을 계속 끊어 새 세그먼트를 만들 수 있습니다. 순수 내구성 목적이면sync_current_file()만 호출하고, checkpoint/rotation은 별도 조건이나 더 긴 주기로 제한하는 쪽이 맞습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/wal/manager/mod.rs` around lines 200 - 219, `flush()` currently calls `checkpoint()` unconditionally, which closes the active WAL segment and rotates files on every flush. Update `WALManager::flush` to perform only durability sync via `sync_current_file()` when there is pending data, and keep `checkpoint()` as a separate rotation path used only when an actual checkpoint is desired. Make sure `spawn_wal_flush_loop` still calls `flush()` for fsync-only behavior, while segment reset logic remains isolated in `checkpoint()`.src/pgwire/protocol/connection_codec.rs (1)
202-232: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift바이너리 파라미터 포맷을 처리해야 합니다.
num_param_format_codes를 읽고도 무시한 채 파라미터를 항상String::from_utf8로 해석해서, 텍스트가 아닌 bind 값은 여기서 깨집니다. 텍스트만 지원할 거면 명시적으로FEATURE_NOT_SUPPORTED를 반환하고, 아니면 바이너리 디코딩을 추가해야 합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pgwire/protocol/connection_codec.rs` around lines 202 - 232, The bind parameter parsing in ConnectionCodec::read_message currently ignores num_param_format_codes and always treats values as UTF-8 text, which breaks binary format parameters. Update the parameter decoding path to inspect the format codes before reading each value: if only text format is supported, return FEATURE_NOT_SUPPORTED explicitly from this branch; otherwise, extend the existing parameter loop to handle binary-decoded values in addition to the current String::from_utf8 path.
🧹 Nitpick comments (5)
src/engine/row_buffer.rs (1)
156-172: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
dirty_bytes의 rewrite 추정치가 실제 크기를 크게 과소평가함rewrite 대상일 때
rows.len() * size_of::<TableDataRow>()는Vec<TableDataField>의 헤더 크기만 반영하고 필드 데이터(문자열/배열 등 힙 내용)는 포함하지 않습니다.append_rows가 이 값을 flush 트리거 임계치와 비교하므로, rewrite가 큰 세그먼트에서는 임계치 도달이 지연되어 백그라운드 10초 flush 이전까지 메모리가 예상보다 커질 수 있습니다. 실제 직렬화 바이트 기반 추적을 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/row_buffer.rs` around lines 156 - 172, The rewrite branch in dirty_bytes underestimates memory because TableDataRow only captures the struct/header size and misses heap-backed field data, which delays flush triggering in append_rows. Update the RowBuffer dirty size calculation to use a more accurate serialized/encoded byte estimate for persisted_rows when segment.rewrite_required is true, ideally based on the same actual bytes that flush would write, and keep the existing pending_append_bytes contribution intact.Cargo.toml (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
memmap2버전을 최신으로 갱신 고려.현재 지정된
0.9.5보다 최신 버전(0.9.10)이 존재합니다. 버그 수정 및 개선 사항을 반영하기 위해 갱신을 고려하세요.📦 버전 갱신 제안
-memmap2 = "0.9.5" +memmap2 = "0.9.10"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` at line 42, The dependency version for memmap2 in Cargo.toml is outdated; update the memmap2 entry from 0.9.5 to the newer available release. Keep the change isolated to the memmap2 package version so the project uses the latest bug fixes and improvements while preserving the existing dependency name and configuration.src/engine/wal/manager/builder.rs (1)
117-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
used_wal_bytes가BincodeDecoder::decode(src/engine/wal/endec/implements/bincode.rs)와 프레임 경계 스캔 로직을 중복 구현.두 함수 모두 "4바이트 헤더 부족 시 zero-padding 여부로 종료 판단" 및 "frame_len==0이면 종료" 로직을 독립적으로 구현하고 있습니다. 현재는 둘 다 올바르게 동작하지만(각자의 반환값 기준으로는), 향후 한쪽 로직만 수정되면
current_offset복구 값과 실제 디코딩 결과가 어긋날 위험이 있습니다. 공용 헬퍼로 통합하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/wal/manager/builder.rs` around lines 117 - 149, `used_wal_bytes` in `builder.rs` duplicates the WAL frame boundary scan logic already present in `BincodeDecoder::decode`, including the zero-padded short-header exit and the `frame_len == 0` terminator case. Refactor both paths to use a shared helper or shared frame-scan function so the offset calculation and decoding behavior stay in sync; keep the existing symbols `used_wal_bytes` and `BincodeDecoder::decode` as the integration points.src/engine/wal/manager/mod.rs (1)
90-123: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
buffers: Vec<WALEntry>가 이미 mmap에 영속화된 엔트리를 중복 보관.
write_entry는 프레임을 mmap에 기록한 뒤(append_frame_to_mmap) 동일한entry를self.buffers에도 push합니다. 실제 데이터는 이미 mmap을 통해 물리적으로 기록되었으므로,buffers는 오직flush()에서 "pending 여부" 판단용으로만 쓰이는 것으로 보입니다(현재 파일 범위 내에서 다른 읽기 지점 없음). row 데이터(payload)까지 포함한 전체 엔트리를 checkpoint 전까지 메모리에 중복 보관하는 것은 세그먼트 크기(최대 16MB)만큼 불필요한 메모리 낭비입니다.단순
bool/카운터로 대체하면 동일한 목적(“pending 여부” 판단)을 달성하면서 메모리 중복을 제거할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/wal/manager/mod.rs` around lines 90 - 123, `write_entry` is duplicating already-persisted WAL data by pushing each `WALEntry` into `self.buffers` after `append_frame_to_mmap`, even though `buffers` appears to be used only as a pending/write-state signal in `flush()`. Update `Manager` to stop storing full entries in `buffers` and replace that state with a lightweight flag or counter that tracks whether there are unflushed entries. Keep the existing write path and rotation logic in `write_entry` and `rotate_if_needed`, but ensure `flush()` still has enough information to decide whether work is pending without retaining full payloads in memory.src/pgwire/connection/connection.rs (1)
634-719: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win테스트 커버리지 보강 제안.
새 테스트들은 텍스트 이스케이프/리터럴 보존/AST 생성 등 핵심 경로를 잘 커버하지만, 비-ASCII(한글 등) 쿼리 텍스트에 대한 케이스와 숫자/불리언 컬럼에 대한 바인딩 케이스, 다중 행 VALUES 케이스는 다루지 않아 위에서 지적한 회귀를 잡지 못합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pgwire/connection/connection.rs` around lines 634 - 719, The new tests cover escaping, placeholder handling, and insert AST creation, but they still miss the regression cases called out in the review. Extend the existing tests in Connection::bind_query_parameters and parse_parameterized_insert to add coverage for non-ASCII SQL/text (for example Korean literals), numeric and boolean parameter binding without quoting as text, and multi-row VALUES parameterized inserts. Keep the assertions focused on the bound query string and the resulting SQLStatement/DMLStatement::InsertQuery shape so these edge cases are exercised alongside the current coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engine/actions/dml/insert.rs`:
- Around line 164-165: The INSERT WAL payload serialization in insert() was
updated to use EntryType::Insert data, but there is no replay/recovery path that
actually applies that entry type. Add an Insert replay handler in the WAL
recovery flow so saved `(into_table, rows)` entries can be re-applied after a
crash, and include matching recovery tests alongside the existing DML action
code to verify the replay path works end to end.
In `@src/engine/actions/dml/scan.rs`:
- Around line 237-245: In the `scan` write path, the `tokio::fs::OpenOptions`
builder currently uses `create(true).write(true)` without explicitly stating
truncation behavior, which triggers the clippy warning. Update the `OpenOptions`
chain in this block to clearly preserve existing contents by adding
`.truncate(false)`, so the intent is explicit and the `ExecuteError::wrap`
handling remains unchanged.
- Around line 62-87: append_table_rows_with_buffer_limit currently updates
row_buffer_pool without taking row_storage_lock, which can race with
update_table_rows/delete_table_rows and lose newly inserted rows. Update this
path to acquire row_storage_lock before reading the snapshot/segment state and
before appending to row_buffer_pool, using the same locking order as the
existing row mutation flow so INSERTs cannot slip between snapshot reads and
replace_rows/pending_append_rows handling.
In `@src/engine/server/mod.rs`:
- Around line 33-62: The WAL flush loop and row flush loop are currently
independent, which can let `wal_manager.flush()` checkpoint and sync the WAL
before `DBEngine::flush_row_buffers()` has made the corresponding row data
durable. Update `spawn_wal_flush_loop` and `spawn_row_flush_loop` so WAL
checkpointing is gated on row-segment fsync completion, or otherwise combine
them into a single durability flow tied to the row flush completion path in
`apply_row_buffer_write()`/`flush_row_buffers()`.
In `@src/engine/wal/manager/mod.rs`:
- Around line 183-198: sync_current_file에서 current_segment의 mmap.flush()와
file.sync_data()가 Tokio 워커를 블로킹하므로, 이 블로킹 I/O를 async 경계 밖으로 분리하세요.
sync_current_file 내부에서 해당 flush/sync 작업을 block_in_place 또는 spawn_blocking으로 감싸고,
WALError::wrap를 유지한 채 errors::Result<()> 흐름이 그대로 반환되도록 정리하세요.
- Around line 150-181: open_current_segment_if_needed currently performs
blocking filesystem and mmap setup directly in the async WAL path, so move the
create_dir_all, OpenOptions::open, set_len, and MmapOptions::map_mut work off
the tokio worker thread by using spawn_blocking or a separate blocking I/O
boundary. Keep the behavior inside open_current_segment_if_needed and the
append_record -> write_entry -> append_frame_to_mmap call chain the same, but
ensure segment creation/rotation no longer blocks flush() or other async
callers.
In `@src/pgwire/connection/connection.rs`:
- Around line 97-109: The `query_has_parameters` helper is incorrectly treating
`$<digit>` inside string literals as real parameters, so update it to ignore
tokens found within quoted strings. Reuse the same quote/string-state tracking
approach already used in `bind_query_parameters` so detection matches bind
behavior, and make sure the logic in `query_has_parameters` only flags
placeholders that appear outside string literals.
- Around line 118-174: `bind_query_parameters` is reconstructing the query
byte-by-byte and corrupting non-ASCII UTF-8 characters when it falls back to
`bound.push(byte as char)`. Update this logic so the function preserves the
original UTF-8 bytes for all non-placeholder text while still handling `'` and
`$<digit>` tokens, ideally by appending slices from the original query or
building from a byte buffer and converting once at the end. Make the fix inside
`connection::bind_query_parameters` and keep `quote_parameter`/placeholder
substitution behavior unchanged.
- Around line 176-247: The parse_parameterized_insert handler is only consuming
the first VALUES tuple and silently ignoring any remaining text, which can drop
additional rows without error. Update parse_parameterized_insert to either fully
validate that nothing unexpected remains after the first tuple (including extra
tuples or trailing clauses like RETURNING) or explicitly reject multi-row
INSERTs by returning None. Use the existing parse_parameterized_insert,
value_end, and InsertQuery builder path to ensure only fully supported
single-row statements are converted.
- Around line 111-116: The pgwire INSERT fast path is forcing all bound values
through quote_parameter and SQLExpression::String, which makes reduce_expression
preserve them as TableDataFieldType::String and then fails type checks in insert
handling. Update the pgwire connection path in connection.rs (quote_parameter,
the INSERT query assembly, and reduce_expression) so non-string bind values are
mapped to the correct SQLExpression/TableDataFieldType based on their actual
type instead of always being quoted as strings. Ensure the generated expressions
match the target column types expected by insert::insert so INT, BOOLEAN, and
FLOAT parameters can pass through this fast path without triggering a type_code
mismatch.
---
Outside diff comments:
In `@src/engine/wal/manager/mod.rs`:
- Around line 200-219: `flush()` currently calls `checkpoint()` unconditionally,
which closes the active WAL segment and rotates files on every flush. Update
`WALManager::flush` to perform only durability sync via `sync_current_file()`
when there is pending data, and keep `checkpoint()` as a separate rotation path
used only when an actual checkpoint is desired. Make sure `spawn_wal_flush_loop`
still calls `flush()` for fsync-only behavior, while segment reset logic remains
isolated in `checkpoint()`.
In `@src/pgwire/protocol/connection_codec.rs`:
- Around line 202-232: The bind parameter parsing in
ConnectionCodec::read_message currently ignores num_param_format_codes and
always treats values as UTF-8 text, which breaks binary format parameters.
Update the parameter decoding path to inspect the format codes before reading
each value: if only text format is supported, return FEATURE_NOT_SUPPORTED
explicitly from this branch; otherwise, extend the existing parameter loop to
handle binary-decoded values in addition to the current String::from_utf8 path.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 42: The dependency version for memmap2 in Cargo.toml is outdated; update
the memmap2 entry from 0.9.5 to the newer available release. Keep the change
isolated to the memmap2 package version so the project uses the latest bug fixes
and improvements while preserving the existing dependency name and
configuration.
In `@src/engine/row_buffer.rs`:
- Around line 156-172: The rewrite branch in dirty_bytes underestimates memory
because TableDataRow only captures the struct/header size and misses heap-backed
field data, which delays flush triggering in append_rows. Update the RowBuffer
dirty size calculation to use a more accurate serialized/encoded byte estimate
for persisted_rows when segment.rewrite_required is true, ideally based on the
same actual bytes that flush would write, and keep the existing
pending_append_bytes contribution intact.
In `@src/engine/wal/manager/builder.rs`:
- Around line 117-149: `used_wal_bytes` in `builder.rs` duplicates the WAL frame
boundary scan logic already present in `BincodeDecoder::decode`, including the
zero-padded short-header exit and the `frame_len == 0` terminator case. Refactor
both paths to use a shared helper or shared frame-scan function so the offset
calculation and decoding behavior stay in sync; keep the existing symbols
`used_wal_bytes` and `BincodeDecoder::decode` as the integration points.
In `@src/engine/wal/manager/mod.rs`:
- Around line 90-123: `write_entry` is duplicating already-persisted WAL data by
pushing each `WALEntry` into `self.buffers` after `append_frame_to_mmap`, even
though `buffers` appears to be used only as a pending/write-state signal in
`flush()`. Update `Manager` to stop storing full entries in `buffers` and
replace that state with a lightweight flag or counter that tracks whether there
are unflushed entries. Keep the existing write path and rotation logic in
`write_entry` and `rotate_if_needed`, but ensure `flush()` still has enough
information to decide whether work is pending without retaining full payloads in
memory.
In `@src/pgwire/connection/connection.rs`:
- Around line 634-719: The new tests cover escaping, placeholder handling, and
insert AST creation, but they still miss the regression cases called out in the
review. Extend the existing tests in Connection::bind_query_parameters and
parse_parameterized_insert to add coverage for non-ASCII SQL/text (for example
Korean literals), numeric and boolean parameter binding without quoting as text,
and multi-row VALUES parameterized inserts. Keep the assertions focused on the
bound query string and the resulting SQLStatement/DMLStatement::InsertQuery
shape so these edge cases are exercised alongside the current coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84fba58f-b0bd-49ec-a745-db55a98bbad4
📒 Files selected for processing (17)
AGENTS.mdCargo.tomlsrc/engine/AGENTS.mdsrc/engine/actions/dml/insert.rssrc/engine/actions/dml/scan.rssrc/engine/initialize.rssrc/engine/mod.rssrc/engine/row_buffer.rssrc/engine/server/mod.rssrc/engine/wal/endec/implements/bincode.rssrc/engine/wal/manager/builder.rssrc/engine/wal/manager/mod.rssrc/pgwire/connection/connection.rssrc/pgwire/connection/prepared_statement.rssrc/pgwire/engine/rrdb.rssrc/pgwire/protocol/connection_codec.rssrc/pgwire/protocol/message/client/types/bind.rs
WAL checkpoint와 row flush 루프가 독립적으로 동작하면서 row segment가 fsync되기 전에 WAL이 checkpoint로 비워질 수 있었다. row buffer에 비내구 flush된 segment를 추적하고, durability flush에서 row segment sync가 끝난 뒤에만 WAL checkpoint를 수행하도록 루프를 통합했다. INSERT WAL replay 경로도 추가해 재시작 시 pending Insert 엔트리를 row storage로 복원한 뒤 checkpoint하도록 했다. WAL segment 생성과 sync 과정의 create/open/set_len/mmap/flush/sync_data가 async 경로에서 직접 실행되던 문제도 spawn_blocking 경계로 분리했다. 이로써 segment 생성, rotation, checkpoint 시 tokio worker thread를 직접 막는 구간을 줄였다. pgwire bind INSERT fast path는 문자열 리터럴 안의 $숫자를 파라미터로 오인하고, UTF-8 쿼리 텍스트를 byte as char로 재구성해 손상시키며, 모든 bind 값을 문자열 SQLExpression으로 만들어 INT/BOOLEAN/FLOAT 컬럼에서 타입 불일치를 일으킬 수 있었다. 문자열 상태 추적을 파라미터 감지에도 적용하고, bind 치환은 원본 UTF-8 bytes를 보존하도록 바꿨으며, INSERT fast path는 테이블 스키마의 대상 컬럼 타입에 맞춰 bound 값을 Integer/Boolean/Float/String/Null 표현식으로 변환한다. 다중 행 parameterized INSERT에서 첫 VALUES 튜플 이후 텍스트를 무시해 나머지 행을 조용히 버릴 수 있던 문제는 fast path가 trailing tuple/RETURNING 등을 만나면 None을 반환해 일반 파싱 경로로 폴백하도록 막았다.
Owner
Author
|
✅ Total Coverage: 66.36% |
append_table_rows_with_buffer_limit가 row_storage_lock 없이 row_buffer_pool만 갱신해 update/delete의 snapshot 기반 replace_rows와 경합할 수 있었다. update/delete가 snapshot을 읽은 뒤 replace_rows로 pending append를 비우는 사이 INSERT가 끼면 새 행이 snapshot에도 pending에도 남지 않아 유실될 수 있었다. append 경로도 update/delete와 동일하게 row_storage_lock을 잡도록 바꾸고, 버퍼 한도 초과 시에는 이미 보유한 락 안에서 flush_row_buffers_locked(false)를 호출해 재진입 deadlock을 피했다. append가 row_storage_lock을 기다리는 회귀 테스트도 추가했다.
Owner
Author
|
✅ Total Coverage: 66.37% |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/server/mod.rs (1)
238-239: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win동시 기록 중인 WAL 파일 디코딩 실패에 대한 테스트 취약성
spawn_durability_flush_loop가 mmap으로 WAL 파일에 기록하는 동안tokio::fs::read후decoder.decode(&content).unwrap()을 호출하면, 아직 flush 되지 않은 부분 프레임을 읽어 디코딩 실패 시 패닉으로 테스트가 간헐적으로 실패할 수 있습니다. 디코딩 실패 시 재시도하도록 완화하세요.💚 재시도로 완화하는 예시
- let content = tokio::fs::read(&wal_path).await.unwrap(); - let entries = decoder.decode(&content).unwrap(); + let content = tokio::fs::read(&wal_path).await.unwrap(); + let Ok(entries) = decoder.decode(&content) else { + tokio::time::sleep(Duration::from_millis(5)).await; + continue; + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/server/mod.rs` around lines 238 - 239, The WAL decoding in the test path is too brittle because `tokio::fs::read` followed by `decoder.decode(&content).unwrap()` can race with `spawn_durability_flush_loop` and panic on partially flushed frames. Update the logic around `decoder.decode` in `server::mod` to handle decode failures gracefully by retrying after a short delay until the WAL is fully flushed, instead of unwrapping immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/engine/server/mod.rs`:
- Around line 238-239: The WAL decoding in the test path is too brittle because
`tokio::fs::read` followed by `decoder.decode(&content).unwrap()` can race with
`spawn_durability_flush_loop` and panic on partially flushed frames. Update the
logic around `decoder.decode` in `server::mod` to handle decode failures
gracefully by retrying after a short delay until the WAL is fully flushed,
instead of unwrapping immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15e2fd93-4cb2-4d52-8aed-fb7d1e29a8ec
📒 Files selected for processing (7)
src/engine/actions/dml/scan.rssrc/engine/row_buffer.rssrc/engine/server/mod.rssrc/engine/wal/manager/mod.rssrc/engine/wal/mod.rssrc/engine/wal/recovery.rssrc/pgwire/connection/connection.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pgwire/connection/connection.rs
- src/engine/wal/manager/mod.rs
- src/engine/actions/dml/scan.rs
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.
resolves: #226
설명
일단 20000 TPS까지는 어떻게 땡겼음. 이제 인덱스만 예쁘게 붙이면 될듯
Summary by CodeRabbit