fix(core): fix rare bug that can potentially write invalid data in mat view by ideoma · Pull Request #6628 · questdb/questdb

ideoma · 2026-01-12T11:57:28Z

Modifies fix in #6519 for some edge case scenarios.

When a fixed column is merged with the column top, the data copied should be

// Copy data to the end of old data + column tops
Vect.memcpy(srcDataFixAddr + srcDataActualBytesOld + (srcDataTop << shl), srcDataFixAddr, srcDataActualBytesNew);

// instead of copying to the end of the newly calculated data size
Vect.memcpy(srcDataFixAddr + wouldBeDataBytes, srcDataFixAddr, srcCopyActualBytes);

Var column types appear not to have the same bug, only comments modified.

…t view

coderabbitai · 2026-01-12T11:57:33Z

Walkthrough

Refactors the mergeVarColumn method in O3OpenColumnJob to replace a single auxRowCount variable with separate auxRowCountOld and auxRowCountNew variants, alongside corresponding auxSizeOld and auxSizeNew tracking. Updates downstream size calculations, memory mapping offsets, and copy/shift operations throughout the merge logic.

Changes

Cohort / File(s)	Summary
Aux/Data Size Handling Refactor `core/src/main/java/io/questdb/cairo/O3OpenColumnJob.java`	Split single `auxRowCount` variable into `auxRowCountOld` and `auxRowCountNew` to track distinct row counts; refactored size calculations for aux and data portions; updated conditional logic for srcDataTop scenarios; adjusted memory mapping offsets and dstOffset calculations to reflect old/new data boundary handling

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

fix(core): fix potential table suspended when replace commit is used on a table with a column top #6026: Both PRs modify the mergeVarColumn method in O3OpenColumnJob; this PR extends internal logic changes to variable naming and size calculation beyond the prior rename.

Suggested labels

storage

Suggested reviewers

bluestreak01
RaphDal
jerrinot

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing a rare bug in column merge operations that could write invalid data to materialized views.
Description check	✅ Passed	The description is directly related to the changeset, explaining the specific bug fix for column merge operations and referencing the prior PR it modifies.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between daf6d1c and d48fa32.

📒 Files selected for processing (1)

core/src/main/java/io/questdb/cairo/O3OpenColumnJob.java

🧰 Additional context used

🧬 Code graph analysis (1)

core/src/main/java/io/questdb/cairo/O3OpenColumnJob.java (3)

core/src/main/java/io/questdb/std/MemoryTag.java (1)

MemoryTag (27-184)

core/src/main/java/io/questdb/std/Files.java (1)

Files (43-805)

core/src/main/java/io/questdb/std/Vect.java (1)

Vect (27-507)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (35)

GitHub Check: New pull request (Coverage Report Coverage Report)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
GitHub Check: New pull request (Hosted Running tests on windows-other-2)
GitHub Check: New pull request (Hosted Running tests on windows-other-1)
GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
GitHub Check: New pull request (Hosted Running tests on mac-other)
GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
GitHub Check: New pull request (Hosted Running tests on mac-cairo)
GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
GitHub Check: New pull request (Trigger Enterprise CI Trigger Enterprise Pipeline)
GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
GitHub Check: New pull request (Hosted Running tests on mac-griffin)
GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
GitHub Check: New pull request (Check Changes Check changes)

🔇 Additional comments (5)

core/src/main/java/io/questdb/cairo/O3OpenColumnJob.java (5)
274-296: LGTM on the old/new row count separation.

The split of auxRowCount into auxRowCountOld (before commit-replace adjustments) and auxRowCountNew (after adjustments) is correct and addresses the edge case described in the PR.

Minor observation: The comment on lines 293-295 states "this size DOES NOT INCLUDE N+1, so it is N here," but getAuxVectorSize(srcDataMax) typically returns the N+1 size for var-size columns. The actual calculation appears correct for the extended buffer allocation at line 309; consider updating the comment for clarity.

302-364: Null materialization logic is correct.

The separation of old/new row counts ensures:

Existing data is read using auxRowCountOld (lines 313-315)

New data operations use auxRowCountNew (lines 326-341)

Shift operations position correctly at auxSizeOld offset (line 346)

srcDataFixOffset is set to auxSizeOld for downstream copy tasks (line 364)

This matches the fix description: data is now copied to the end of old data plus column tops, rather than to the end of newly calculated data size.

376-384: Correct use of auxRowCountNew in the column top preservation branch.

When the column top remains (srcDataTop <= prefixHi), using auxRowCountNew for size calculations is appropriate since it reflects the actual data rows after any commit-replace adjustments.

2451-2503: Core fix for the invalid data write bug is correct.

The key change at line 2485:
Vect.memcpy(srcDataFixAddr + srcDataActualBytesOld + (srcDataTop << shl), srcDataFixAddr, srcDataActualBytesNew);
This correctly copies merged fixed-column data to oldDataEnd + columnTopNullSpace, ensuring:

Nulls are materialized at srcDataActualBytesOld offset (line 2483)

Actual data is copied immediately after the null space

srcDataFixOffset correctly points to where the merged data begins (line 2487)

This addresses the rare bug where data could be written to incorrect offsets when column top needed materialization during merge.

2488-2503: LGTM for the column top preservation branch.

When column top is within prefix bounds (srcDataTop <= prefixHi), using srcDataActualBytesNew for the file size is correct since no null materialization is needed—the column top metadata handles the logical nulls.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

core/src/main/java/io/questdb/cairo/O3OpenColumnJob.java

glasstiger · 2026-01-12T17:23:28Z

[PR Coverage check]

😍 pass : 24 / 24 (100.00%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/cairo/O3OpenColumnJob.java	24	24	100.00%

ideoma · 2026-01-12T17:29:07Z

@coderabbitai review

coderabbitai · 2026-01-12T17:29:15Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

fix(core): fix rare bug that can potentially write invalid data in ma…

c784d29

…t view

revert fix for var col types

a41233d

puzpuzpuz added Bug Incorrect or unexpected behavior Core Related to storage, data type, etc. labels Jan 12, 2026

puzpuzpuz self-requested a review January 12, 2026 12:47

puzpuzpuz reviewed Jan 12, 2026

View reviewed changes

core/src/main/java/io/questdb/cairo/O3OpenColumnJob.java Outdated Show resolved Hide resolved

core/src/main/java/io/questdb/cairo/O3OpenColumnJob.java Show resolved Hide resolved

puzpuzpuz added 3 commits January 12, 2026 15:31

Improve comments

44a148a

More consistent code style

c225c0c

One more renaming

0d745c0

puzpuzpuz reviewed Jan 12, 2026

View reviewed changes

core/src/main/java/io/questdb/cairo/O3OpenColumnJob.java Outdated Show resolved Hide resolved

ideoma added 2 commits January 12, 2026 16:50

use auxRowCountNew in var col merge as suggested in PR comments

08faaed

Merge branch 'master' into fix-col-top-replace-commit-merge

d48fa32

puzpuzpuz approved these changes Jan 12, 2026

View reviewed changes

bluestreak01 merged commit 6438235 into master Jan 12, 2026
43 checks passed

bluestreak01 deleted the fix-col-top-replace-commit-merge branch January 12, 2026 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): fix rare bug that can potentially write invalid data in mat view#6628

fix(core): fix rare bug that can potentially write invalid data in mat view#6628
bluestreak01 merged 7 commits intomasterfrom
fix-col-top-replace-commit-merge

ideoma commented Jan 12, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Jan 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glasstiger commented Jan 12, 2026

Uh oh!

ideoma commented Jan 12, 2026

Uh oh!

coderabbitai bot commented Jan 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ideoma commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glasstiger commented Jan 12, 2026

[PR Coverage check]

file detail

Uh oh!

ideoma commented Jan 12, 2026

Uh oh!

coderabbitai bot commented Jan 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ideoma commented Jan 12, 2026 •

edited

Loading

coderabbitai bot commented Jan 12, 2026 •

edited

Loading