db: fix WAL replay in read-only mode

[petermattis]

The addition of memTable.logSeqNum broke WAL replay in read-only mode
because we were initializing memTable.logSeqNum too early and setting
it to the visible sequence number rather than the first sequence number
in the WAL being replayed. This would result in an error during Open
that looked like:

   pebble: batch seqnum 2 is less than memtable creation seqnum 3

[petermattis]

This change is 

[petermattis]

The new TestOpenWALReplayReadOnly currently fails with:

    open_test.go:484: pebble: batch seqnum 2 is less than memtable creation seqnum 3

The problem is that we're creating the memtable in Open with the value of versionSet.logSeqNum which is larger than the seqnum in the log we're replaying. I initially started to fix this by reworking startup so that we wouldn't create the mutable memtable in read-only mode until we know the first seqnum in the log.

As I looked more, I think there is a more serious problem that could affect compatibility with RocksDB. versionSet.logAndApply sets VersionEdit.LastSeqNum = atomic.LoadUint64(&vs.logSeqNum). When we're rotating the memtable/WAL because the memtable is full, we've already incremented vs.logSeqNum. So we're setting VersionEdit.LastSeqNum to a value that is larger than the first batch that will be written to the new log. We're not strict about filtering out these batches during replay, but I think RocksDB might be.

I suspect what we have to do is to pass in the last unflushed seqnum to logAndApply. Conveniently we now have this available in memTable.logSeqNum.

@itsbilal and @sumeerbhola can you scrutinize this logic and see if I'm missing anything.

[petermattis]

I suspect what we have to do is to pass in the last unflushed seqnum to logAndApply. Conveniently we now have this available in memTable.logSeqNum.

This was remarkably easy to do.

[petermattis]

Oops, accidentally clicked the close button some how.

This was remarkably easy to do.

I spoke too soon. This actually has a lot of negative effects. Surprisingly many. Which makes me think RocksDB is not doing this. I'm going to dive into the RocksDB code and figure out exactly how and when it sets LastSeqNum.

[petermattis]

Ok, this is all fixed up and ready for a look. I left a comment on where we assign VersionEdit.LastSeqNum explaining my learnings from the rabbit hole I went down and also that the current assignment is exactly what RocksDB does.

[petermattis]

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

---

db.go, line 1366 at r1 (raw file):

		if b != nil {
			logSeqNum = b.SeqNum()
			if b.flushable != nil {

This appears to have been a bug. I'm still trying to write a test which triggers it.

[petermattis]

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @itsbilal and @sumeerbhola)

---

db.go, line 1366 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This appears to have been a bug. I'm still trying to write a test which triggers it.

Ok, test added. I believe these was innocuous. The effect was that memTable.logSeqNum would be smaller than the minimum sequence number added to the memTable. This would cause the memTable to prevent L0->L0 compactions in very rare circumstances, and cause the memTable to be accessed on reads unnecessarily in very rare circumstances.

[itsbilal]

Reviewed 2 of 5 files at r1, 4 of 4 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sumeerbhola)

[petermattis]

TFTR!