Update VersionSet last seqno after LogAndApply

[cbi42]

This PR fixes the issue of unstable snapshot during external SST file ingestion. Credit @ajkr for the following walk through: consider these relevant steps for of IngestExternalFile():

(1) increase seqno while holding mutex --

rocksdb/db/db_impl/db_impl.cc

Line 4768 in 677d2b4

versions_->SetLastSequence(last_seqno + consumed_seqno_count);

(2) LogAndApply() --

rocksdb/db/db_impl/db_impl.cc

Lines 4797 to 4798 in 677d2b4

	versions_->LogAndApply(cfds_to_commit, mutable_cf_options_list,
	edit_lists, &mutex_, directories_.GetDbDir());

(a) write to MANIFEST with mutex released

rocksdb/db/version_set.cc

Line 4407 in a96a4a2

mu->Unlock();

(b) apply to in-memory state with mutex held

A snapshot taken during (2a) will be unstable. In particular, queries against that snapshot will not include data from the ingested file before (2b), and will include data from the ingested file after (2b).

Test Plan:
Added a new unit test: ExternalSSTFileBasicTest.WriteAfterReopenStableSnapshotWhileLoggingToManifest.

make external_sst_file_basic_test
./external_sst_file_basic_test

[facebook-github-bot]

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ajkr]

LGTM, thanks for showing a simple solution can work!

I thought for a while about what we can do to increase concurrency of live writes with file ingestion. Nothing to do now; recording my notes here for future reference.

• There is a way we can start working on the live writes earlier (cockroachdb/pebble does it) by decoupling the seqno allocated to writers from the seqno visible to readers, but still we would have to wait for the ingestion to finish before parallel live writes can be made visible to readers.
• We could try only making writes that overlap with an ingested file wait, or return TryAgain if a write overlapping with an ingested file happened during the flushing stage. Currently we block all writes from even starting while we flush memtables and write to manifest which feels excessive.

[facebook-github-bot]

@cbi42 has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ajkr]

I thought for a while about what we can do to increase concurrency of live writes with file ingestion. Nothing to do now; recording my notes here for future reference.

One more, just because it's an interesting problem to think about:

• The ingestion path could simply insert a special range key [ingested file smallest key, ingested file largest key] -> ingested filename
  • When readers need data for such a range key in memtable, they would look into ingested filename
  • When background flush encounters such a range key, it can do the work of assigning the ingested file to a level and committing it to the LSM