Skip to content
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

Fix windows dataloss issue #1134

Merged
merged 3 commits into from Dec 6, 2019
Merged

Fix windows dataloss issue #1134

merged 3 commits into from Dec 6, 2019

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Nov 25, 2019

Windows doesn't allow memory mapping a file to a size greater than the file's actual size. To circumvent this, we increase the file size by truncating it.

badger/y/mmap_windows.go

Lines 41 to 48 in f5b6321

// In windows, we cannot mmap a file more than it's actual size.
// So truncate the file to the size of the mmap.
if fi.Size() < size {
if err := fd.Truncate(size); err != nil {
return nil, fmt.Errorf("truncate: %s", err)
}
}

When badger would re-open, we try to replay this "truncated" file. Since this truncated file consists of all zeros, the replay would return the last offset as zero and then we would truncate the original file to size zero.

The replay function would return zero as the last valid offset which was wrong. The last valid offset is start offset plus the forward movements of the file offset.
So instead of

badger/value.go

Line 433 in f5b6321

var validEndOffset uint32

var validEndOff uint32 // notice we're starting from zero, not the start point.

we should be doing

var validEndOff uint32 = offset

Fixes - #1126


This change is Reviewable

@coveralls
Copy link

coveralls commented Nov 25, 2019

Coverage Status

Coverage increased (+0.4%) to 77.942% when pulling d300dbd on ibrahim/windows-dataloss into f5b6321 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 77.974% when pulling d300dbd on ibrahim/windows-dataloss into f5b6321 on master.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ashish-goswami)

@jarifibrahim jarifibrahim merged commit 969a8e8 into master Dec 6, 2019
@jarifibrahim jarifibrahim deleted the ibrahim/windows-dataloss branch December 6, 2019 10:26
jarifibrahim pushed a commit that referenced this pull request Mar 12, 2020
Windows doesn't allow memory mapping a file to a size greater than the
file's actual size. To circumvent this, we increase the file size by
truncating it.
https://github.com/dgraph-io/badger/blob/f5b63211d7f3e2f5f8b698893313b2a54e4df7de/y/mmap_windows.go#L41-L48

When badger would re-open, we try to replay this "truncated" file.
Since this truncated file consists of all zeros, the replay would
return the last offset as `zero` and then we would truncate the
original file to size `zero`.

The replay function would return `zero` as the last valid offset
which was wrong. The last valid offset is start offset plus the
forward movements of the file offset.
So instead of
https://github.com/dgraph-io/badger/blob/f5b63211d7f3e2f5f8b698893313b2a54e4df7de/value.go#L433
```go
var validEndOff uint32 // notice we're starting from zero, not the start point.
```
we should be doing
```go
var validEndOff uint32 = offset
```
Fixes - #1126

(cherry picked from commit 969a8e8)
@jarifibrahim jarifibrahim mentioned this pull request Mar 18, 2020
@zhenruyan
Copy link

zhenruyan commented Jan 20, 2024

I see that this issue has been resolved, but currently I am encountering the same phenomenon on Windows 10 Golang 1.21.0. When the program interrupts abnormally and fails to close properly, the vlog size becomes 1kb and prompts "Value log truncate required to run DB. This might result in data loss" @jarifibrahim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants