db: fix WAL checksum error handling during upgrade

[jbowens]

A database that was created using RocksDB or a previous version of
Pebble does not guarantee that its closed WALs were closed cleanly.
Detect these cases by the absence of a special private
'strict_wal_tail' option. If this special option is missing from the
OPTIONS file, interpret WALs permissively as they're interpreted in the
RocksDB and Pebble versions that wrote them.

See cockroachdb/cockroach#52729.

[cockroach-teamcity]

This change is 

[petermattis]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @itsbilal, @jbowens, and @petermattis)

---

open.go, line 276 at r1 (raw file):

	for i, lf := range logFiles {
		lastWAL := i == len(logFiles)-1
		maxSeqNum, err := d.replayWAL(jobID, &ve, opts.FS, opts.FS.PathJoin(d.walDirname, lf.name), lf.num, strictWALTail && !lastWAL)

Can you format this across multiple lines? The single line has got a bit long.

---

open_test.go, line 624 at r1 (raw file):

	t.Logf("zeored four bytes in %s at offset %d\n", logs[len(logs)-2], off)

	// Remove the strict_wal_tail option from the OPTIONS file.

Isn't it sufficient to just remove the OPTIONS file? Or you could write a new one where Options.private.strictWALTail = false (because we can access that variable in this test).

---

open_test.go, line 634 at r1 (raw file):

	require.NoError(t, ioutil.WriteFile(filepath.Join(dir, optionFilename), buf.Bytes(), os.ModePerm))

	// Re-opening the database should detect and report the corruption.

s/should detect and report the corruption/should not report the corruption/g

---

options.go, line 923 at r1 (raw file):

}

func (o *Options) checkOptions(s string) (strictWALTail bool, err error) {

Specializing the return of strictWALTail feels a bit awkward to me. This is fine for this PR, but I wonder if we should pull out the closure below to some facility where you can chain functions to be called on unknown values. Then Check would remain unchanged, but we'd be able to extend its functionality for use during Open. Not sure if any of that made sense. Probably easier for me to take a crack at it once this PR merges.

[jbowens]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @itsbilal and @petermattis)

---

open.go, line 276 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Can you format this across multiple lines? The single line has got a bit long.

Done.

---

open_test.go, line 624 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Isn't it sufficient to just remove the OPTIONS file? Or you could write a new one where Options.private.strictWALTail = false (because we can access that variable in this test).

Yeah, that's sufficient. Updated to just remove the file.

---

open_test.go, line 634 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/should detect and report the corruption/should not report the corruption/g

Whoops, done.

---

options.go, line 923 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Specializing the return of strictWALTail feels a bit awkward to me. This is fine for this PR, but I wonder if we should pull out the closure below to some facility where you can chain functions to be called on unknown values. Then Check would remain unchanged, but we'd be able to extend its functionality for use during Open. Not sure if any of that made sense. Probably easier for me to take a crack at it once this PR merges.

Yeah, agreed, it's kinda hacked into there. Yeah, once this lands feel free, or if you don't get around to it I'll look at it. Added a TODO.

[petermattis]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @itsbilal and @jbowens)

---

options.go, line 923 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Yeah, agreed, it's kinda hacked into there. Yeah, once this lands feel free, or if you don't get around to it I'll look at it. Added a TODO.

Sounds good.