Badger open data directory without any error with vlog file what contains broken value data

[nordicdyno]

What version of Go are you using (go version)?

$ go version
1.13

What version of Badger are you using?

• v1.6
• v2.0.0-rc2

Does this issue reproduce with the latest master?

yes

What are the hardware specifications of the machine (RAM, OS, Disk)?

MacBook Pro (15-inch, 2018), 2,2 GHz i7, 16Gb, APPLE SSD AP0256M

What did you do?

I've spoiled data in vlog file in data directory (emulate real world issues with hardware) and badger just starts with this directory with any error.

What did you expect to see?

Error on database open.

What did you see instead?

Badger works without any error if spoiled data are in values body.

Extra

More info and reproducible tests are here:
https://github.com/nordicdyno/badger-spoiler

I'm not sure, but probably it's related to #601

[jarifibrahim]

@nordicdyno The test you're referring to corrupts a bit in the middle of the file. This is an unlikely scenario. AFAIK, a file cannot be corrupted in the middle. It can be corrupted only from the end.

If the corruption was at the end, badger would've recognized it. We perform checksum verification of the entries in vlog after a specific point (called as head point).

[nordicdyno]

I disagree. It's very likely scenario if we speak about disk corruption after badger stop*. I don't see any reason why it could NOT happen. And problem is what we never know about it as badger users.

* I don't want to speak about probability here, but real world could be a messy place

[nordicdyno]

Why not just add checksum for every item in vlog and checking it against replayed values on start? (or smth like that)

[jarifibrahim]

This should be fixed by #1052 .

Thanks for writing a test @nordicdyno :)

[afiskon]

For the record - this might be not a problem if the data is corrupted before the checkpoint (meaning the record that means that everything before it was synced to the heap), e.g. the record will not be used anyway during the recovery process after the restart. If the vlog entry is used during the recovery then I agree with @nordicdyno it's checksum should be checked. This should be a default behavior.

While working on PostgreSQL I had an unpleasant experience of recovering data from databases/backups which random entries were corrupted. Sadly sometimes this really happens in real world due to hardware failures.

[jarifibrahim]

For the record - this might be not a problem if the data is corrupted before the checkpoint (meaning the record that means that everything before it was synced to the heap), e.g. the record will not be used anyway during the recovery process after the restart.

Even if the record before the checkpoint (head) is corrupted, we'll have the same problem.
The record might not be replayed, but if the value is stored in the vlog then the user might get corrupted data. Badger stores the value in either the SST or the value log depending upon the value of ValueThreshold

badger/options.go

Lines 268 to 278 in 64eb1df

	// WithValueThreshold returns a new Options value with ValueThreshold set to the given value.
	//
	// ValueThreshold sets the threshold used to decide whether a value is stored directly in the LSM
	// tree or separatedly in the log value files.
	//
	// The default value of ValueThreshold is 32, but LSMOnlyOptions sets it to 65500.
	func (opt Options) WithValueThreshold(val int) Options {
	opt.ValueThreshold = val
	return opt
	}

If the vlog entry is used during the recovery then I agree with @nordicdyno it's checksum should be checked. This should be a default behavior.

We already verify the checksum when we replay the entries. See

badger/value.go

Lines 382 to 391 in e3b5652

	if _, err := io.ReadFull(reader, crcBuf[:]); err != nil {
	if err == io.EOF {
	err = errTruncate
	}
	return nil, err
	}
	crc := y.BytesToU32(crcBuf[:])
	if crc != tee.Sum32() {
	return nil, errTruncate
	}