Logically strip timestamp during flush

[jowlyzhang]

Logically strip the user-defined timestamp when L0 files are created during flush when AdvancedColumnFamilyOptions.persist_user_defined_timestamps is false. Logically stripping timestamp here means replacing the original user-defined timestamp with a mininum timestamp, which for now is hard coded to be all zeros bytes.

While working on this, I caught a missing piece on the BlockBuilder level for this feature. The current quick path std::min(buffer_size, last_key_size) needs a bit tweaking to work for this feature. When user-defined timestamp is stripped during block building, on writing first entry or right after resetting, buffer is empty and buffer_size is zero as usual. However, in follow-up writes, depending on the size of the stripped user-defined timestamp, and the size of the value, what's in buffer can sometimes be smaller than last_key_size, leading std::min(buffer_size, last_key_size) to truncate the last_key. Previous test doesn't caught the bug because in those tests, the size of the stripped user-defined timestamps bytes is smaller than the length of the value. In order to avoid the conditional operation, this PR changed the original trivial std::min operation into an arithmetic operation. Since this is a change in a hot and performance critical path, I did the following benchmark to check no observable regression is introduced.
TEST_TMPDIR=/dev/shm/rocksdb1 ./db_bench -benchmarks=fillseq -memtablerep=vector -allow_concurrent_memtable_write=false -num=50000000
Compiled with DEBUG_LEVEL=0
Test vs. control runs simulaneous for better accuracy, units = ops/sec
PR vs base:
Round 1: 350652 vs 349055
Round 2: 365733 vs 364308
Round 3: 355681 vs 354475

Test Plan:
New timestamp specific test added or existing tests augmented, both are parameterized with UserDefinedTimestampTestMode:
UserDefinedTimestampTestMode::kNormal -> UDT feature enabled, write / read with min timestamp
UserDefinedTimestampTestMode::kStripUserDefinedTimestamps -> UDT feature enabled, write / read with min timestamp, set Options.persist_user_defined_timestamps to false.

make all check
./db_wal_test --gtest_filter="*WithTimestamp*"
./flush_job_test --gtest_filter="*WithTimestamp*"
./repair_test --gtest_filter="*WithTimestamp*"
./block_based_table_reader_test

[facebook-github-bot]

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

[pdillinger]

I find it confusing that some places talk about stripping or removing the timestamps while some talk about replacing with minimum timestamp. I might expect that if I don't persist timestamps, I could re-open the DB without timestamps configured at all, but I don't think that is true. I think we need to be more clear and careful about describing what the persist_user_defined_timestamps option does.

[facebook-github-bot]

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

[jowlyzhang]

I find it confusing that some places talk about stripping or removing the timestamps while some talk about replacing with minimum timestamp. I might expect that if I don't persist timestamps, I could re-open the DB without timestamps configured at all, but I don't think that is true. I think we need to be more clear and careful about describing what the persist_user_defined_timestamps option does.

Thank you for this suggestion! I took a look at the existing comment for persist_user_defined_timestamps flag and realized it indeed needed more clarification. I added some comment there.

Specifically on this flush path, what not persist_user_defined_timestamps does is indeed removing the timestamps and do not persist them. The reason there are some places that logically strip (replace with min timestamp), and some places that actually strip (or remove) is because there are still some key comparisons happening at different levels during flush, for example, in BlockBasedTableBuilder, IndexBuilder etc. To ensure the user keys still have a compatible format as the timestamp aware user comparator, the user-defined timestamp part of the user key is not physically removed until the very last minute before it's building into a block in BlockBuilder, and before that, it's only logically removed.

Because these SST files are created without any user-defined timestamps, users indeed can re-open the DB without timestamps configured at all. At that time, they provide a user comparator that has timestamp_size set to 0, and the user keys in these SST files are instantly compatible with that format.

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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

[pdillinger]

LGTM except for the described hole in testing. Could be fixed in this PR or another.

[pdillinger]

Don't forget to re-import :)

[jowlyzhang]

LGTM except for the described hole in testing. Could be fixed in this PR or another.

Thank you for your review! I will add that test coverage in a follow up.

[facebook-github-bot]

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

[facebook-github-bot]

@jowlyzhang merged this pull request in 15053f3.