Add support for timestamp in Get/Put #5079

riversand963 · 2019-03-18T17:47:19Z

It's useful to be able to (optionally) associate key-value pairs with user-provided timestamps. This PR is an early effort towards this goal and continues the work of #4942. A suite of new unit tests exist in DBBasicTestWithTimestampWithParam. Support for timestamp requires the user to provide timestamp as a slice in ReadOptions and WriteOptions. All timestamps of the same database must share the same length, format, etc. The format of the timestamp is the same throughout the same database, and the user is responsible for providing a comparator function (Comparator) to order the <key, timestamp> tuples. Once created, the format and length of the timestamp cannot change (at least for now).

Test plan (on devserver):

$COMPILE_WITH_ASAN=1 make -j32 all
$./db_basic_test --gtest_filter=Timestamp/DBBasicTestWithTimestampWithParam.PutAndGet/*
$make check

All tests must pass.

We also run the following db_bench tests to verify whether there is regression on Get/Put while timestamp is not enabled.

$TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillseq,readrandom -num=1000000
$TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillrandom -num=1000000

Repeat for 6 times for both versions.

Results are as follows:

|        | readrandom | fillrandom |
| master | 16.77 MB/s | 47.05 MB/s |
| PR5079 | 16.44 MB/s | 47.03 MB/s |

sagar0

Thanks for working on this! This would be a great feature to have in RocksDB.

include/rocksdb/comparator.h

siying

Awesome! We need to document in some comments that it is just a feature in-progress, what is supported, not supported, and mention that format compatibility is not guaranteed and API is applicable to change. And I think it's pretty close to be merged!

siying · 2019-04-25T18:39:03Z

db/db_impl.cc

+  if (read_options.timestamp != nullptr) {
+    Slice akey;
+    std::string buf;
+    Status s = AppendTimestamp(key, *(read_options.timestamp), &akey, &buf);


Maybe a TODO: in GetImpl() we already copy the key once to LookupKey lkey. We should do the copying together.

Yes I have TODOs for both Write/Put and Get. Will be addressed in another PR. This one focuses more on API and tries to demonstrate how the internal can work with timestamp.

include/rocksdb/comparator.h

table/block_based_table_reader.cc

table/get_context.cc

db/db_basic_test.cc

siying · 2019-04-25T21:36:11Z

table/get_context.cc

@@ -182,7 +182,8 @@ bool GetContext::SaveValue(const ParsedInternalKey& parsed_key,
  assert(matched);
  assert((state_ != kMerge && parsed_key.type != kTypeMerge) ||
         merge_context_ != nullptr);
-  if (ucmp_->Equal(parsed_key.user_key, user_key_)) {
+  if (ucmp_->Equal(parsed_key.user_key, user_key_) ||
+      ucmp_->CompareWithoutTimestamp(parsed_key.user_key, user_key_) == 0) {


While it is correct to check only key without timestamp for hit, I think merge should only happen if both of user key and timestamp are the same.

Yes, merge should also look at timestamp. To RocksDB, two equal keys with different user-timestamps are just two different keys. Without further instruction from the user, e.g. merge policy, RocksDB needs to treat them as separate keys.
I didn't implement merge in this PR though.

siying · 2019-04-25T21:42:13Z

db/db_impl.cc

@@ -1352,6 +1352,15 @@ ColumnFamilyHandle* DBImpl::DefaultColumnFamily() const {
 Status DBImpl::Get(const ReadOptions& read_options,
                   ColumnFamilyHandle* column_family, const Slice& key,
                   PinnableSlice* value) {
+  if (read_options.timestamp != nullptr) {


Maybe not in this PR, but we should think about returning which timestamp for the entry returned.

Do you mean we return the timestamp to user? Say if user specifies 55 as timestamp, and the db has a record with ts 54, we need to return 54 as well as the key-value pair?

Yes. We have been talking about a hidden column or something like that in SQL tables which is timestamp.

include/rocksdb/comparator.h

table/block_based_table_reader.cc

include/rocksdb/comparator.h

db/memtable.cc

siying

I think we are very close to get it committed!

include/rocksdb/options.h

include/rocksdb/comparator.h

siying

Let's land the first commit!!! Please mention it in HISTORY.md

siying · 2019-04-29T20:07:47Z

Failed tests are legitimate though.

riversand963 · 2019-04-29T20:09:45Z

Failed tests are legitimate though.

Taking a look now.

facebook-github-bot

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

riversand963 · 2019-05-01T00:28:51Z

@siying, I think we should also persist timestamp size info to the MANIFEST using the safe ignore mask.

riversand963 · 2019-05-06T17:12:49Z

Discussed with @siying offline. For now we can still rely on comparator name to identify comparators without persisting timestamp size in the MANIFEST. It may be a good idea to postpone MANIFEST format change (although can be forward-compatible) until we are ready to officially release timestamp support.

facebook-github-bot · 2019-05-06T22:22:08Z

@riversand963 has updated the pull request. Re-import the pull request

riversand963 · 2019-05-07T00:07:08Z

@siying. Since we may likely allow users to construct their own WriteBatch with augmented keys (key + timestamp), I think we probably need to perform some argument sanity checks in DBImpl::Write anyway. Therefore, we should not perform the check in DB::Put in order to reduce duplicate code. Then we may need to add some state information to WriteBatch to indicate whether timestamp(s) has been assigned to the records in the write batch.

facebook-github-bot

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

facebook-github-bot · 2019-05-07T16:24:38Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-05-17T00:58:30Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2019-05-17T16:49:31Z

@riversand963 has updated the pull request. Re-import the pull request

Summary: Explicitly override ComparaWithoutTimestamp in BytewiseComparator and ReverseBytewiseComparator to avoid an extra virtual function call incurred by Comparator::CompareWithoutTimestamp.

Summary: We need to make timestamp_size_ non const because we implicitly use assignment operator. Also make it private to disable subclass modifying its value. Finally, add an explicit overloaded assignment.

facebook-github-bot · 2019-06-05T00:13:44Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-06-05T17:42:49Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2019-06-06T04:35:42Z

@riversand963 has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2019-06-06T07:38:41Z

@riversand963 merged this pull request in 340ed4f.

Summary: It's useful to be able to (optionally) associate key-value pairs with user-provided timestamps. This PR is an early effort towards this goal and continues the work of facebook#4942. A suite of new unit tests exist in DBBasicTestWithTimestampWithParam. Support for timestamp requires the user to provide timestamp as a slice in `ReadOptions` and `WriteOptions`. All timestamps of the same database must share the same length, format, etc. The format of the timestamp is the same throughout the same database, and the user is responsible for providing a comparator function (Comparator) to order the <key, timestamp> tuples. Once created, the format and length of the timestamp cannot change (at least for now). Test plan (on devserver): ``` $COMPILE_WITH_ASAN=1 make -j32 all $./db_basic_test --gtest_filter=Timestamp/DBBasicTestWithTimestampWithParam.PutAndGet/* $make check ``` All tests must pass. We also run the following db_bench tests to verify whether there is regression on Get/Put while timestamp is not enabled. ``` $TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillseq,readrandom -num=1000000 $TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillrandom -num=1000000 ``` Repeat for 6 times for both versions. Results are as follows: ``` | | readrandom | fillrandom | | master | 16.77 MB/s | 47.05 MB/s | | PR5079 | 16.44 MB/s | 47.03 MB/s | ``` Pull Request resolved: facebook#5079 Differential Revision: D15132946 Pulled By: riversand963 fbshipit-source-id: 833a0d657eac21182f0f206c910a6438154c742c

Summary: In previous #5079, we added user-specified timestamp to `DB::Get()` and `DB::Put()`. Limitation is that these two functions may cause extra memory allocation and key copy. The reason is that `WriteBatch` does not allocate extra memory for timestamps because it is not aware of timestamp size, and we did not provide an API to assign/update timestamp of each key within a `WriteBatch`. We address these issues in this PR by doing the following. 1. Add a `timestamp_size_` to `WriteBatch` so that `WriteBatch` can take timestamps into account when calling `WriteBatch::Put`, `WriteBatch::Delete`, etc. 2. Add APIs `WriteBatch::AssignTimestamp` and `WriteBatch::AssignTimestamps` so that application can assign/update timestamps for each key in a `WriteBatch`. 3. Avoid key copy in `GetImpl` by adding new constructor to `LookupKey`. Test plan (on devserver): ``` $make clean && COMPILE_WITH_ASAN=1 make -j32 all $./db_basic_test --gtest_filter=Timestamp/DBBasicTestWithTimestampWithParam.PutAndGet/* $make check ``` If the API extension looks good, I will add more unit tests. Some simple benchmark using db_bench. ``` $rm -rf /dev/shm/dbbench/* && TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillseq,readrandom -num=1000000 $rm -rf /dev/shm/dbbench/* && TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillrandom -num=1000000 -disable_wal=true ``` Master is at a78503b. ``` | | readrandom | fillrandom | | master | 15.53 MB/s | 25.97 MB/s | | PR5502 | 16.70 MB/s | 25.80 MB/s | ``` Pull Request resolved: #5502 Differential Revision: D16340894 Pulled By: riversand963 fbshipit-source-id: 51132cf792be07d1efc3ac33f5768c4ee2608bb8

…k#5502) Summary: In previous facebook#5079, we added user-specified timestamp to `DB::Get()` and `DB::Put()`. Limitation is that these two functions may cause extra memory allocation and key copy. The reason is that `WriteBatch` does not allocate extra memory for timestamps because it is not aware of timestamp size, and we did not provide an API to assign/update timestamp of each key within a `WriteBatch`. We address these issues in this PR by doing the following. 1. Add a `timestamp_size_` to `WriteBatch` so that `WriteBatch` can take timestamps into account when calling `WriteBatch::Put`, `WriteBatch::Delete`, etc. 2. Add APIs `WriteBatch::AssignTimestamp` and `WriteBatch::AssignTimestamps` so that application can assign/update timestamps for each key in a `WriteBatch`. 3. Avoid key copy in `GetImpl` by adding new constructor to `LookupKey`. Test plan (on devserver): ``` $make clean && COMPILE_WITH_ASAN=1 make -j32 all $./db_basic_test --gtest_filter=Timestamp/DBBasicTestWithTimestampWithParam.PutAndGet/* $make check ``` If the API extension looks good, I will add more unit tests. Some simple benchmark using db_bench. ``` $rm -rf /dev/shm/dbbench/* && TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillseq,readrandom -num=1000000 $rm -rf /dev/shm/dbbench/* && TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillrandom -num=1000000 -disable_wal=true ``` Master is at a78503b. ``` | | readrandom | fillrandom | | master | 15.53 MB/s | 25.97 MB/s | | PR5502 | 16.70 MB/s | 25.80 MB/s | ``` Pull Request resolved: facebook#5502 Differential Revision: D16340894 Pulled By: riversand963 fbshipit-source-id: 51132cf792be07d1efc3ac33f5768c4ee2608bb8

Summary: Fixes facebook#5079. So what happens was 1. Durable uploads CLOUDMANIFEST 2. Non-durable downloads CLOUDMANIFEST 3. Non-durable tried to open MANIFEST, couldn't find it locally nor in the cloud, assuming it's a new DB. 4. Durable uploads new MANIFEST 5. Non-durable wrote new epoch to its local CLOUDMANIFEST 6. Non-durable failed to open MANIFEST, restarts. At restart, it resuses its local CLOUDMANIFEST without compatible MANIFEST, goes into crash loop. If 3 and 4 reverses order, non-durable will be able to open the DB. This diff ensures that if this scenario occurs, non-durable will reinitializes the entire directory. Test Plan: added a unittest to describe such scenario. Reviewers: dhruba, igor Reviewed By: igor Differential Revision: https://rockset.phacility.com/D5571

include/rocksdb/options.h

riversand963 requested review from sagar0 and siying March 18, 2019 17:47

facebook-github-bot added the CLA Signed label Mar 18, 2019

sagar0 reviewed Mar 18, 2019

View reviewed changes

include/rocksdb/comparator.h Outdated Show resolved Hide resolved

riversand963 force-pushed the ts_v4 branch from 8d49c74 to e23f1fe Compare April 1, 2019 17:21

riversand963 force-pushed the ts_v4 branch from e23f1fe to c2e6097 Compare April 17, 2019 23:45

siying mentioned this pull request Apr 23, 2019

Add iterate_prefix for ReadOption and construct internal iterators lazily #5235

Closed

siying reviewed Apr 25, 2019

View reviewed changes

db/db_basic_test.cc Show resolved Hide resolved

siying reviewed Apr 25, 2019

View reviewed changes

include/rocksdb/comparator.h Outdated Show resolved Hide resolved

table/block_based_table_reader.cc Outdated Show resolved Hide resolved

include/rocksdb/comparator.h Show resolved Hide resolved

db/memtable.cc Outdated Show resolved Hide resolved

riversand963 force-pushed the ts_v4 branch from c2e6097 to 80a5db1 Compare April 26, 2019 22:18

siying reviewed Apr 26, 2019

View reviewed changes

include/rocksdb/options.h Show resolved Hide resolved

include/rocksdb/options.h Show resolved Hide resolved

include/rocksdb/comparator.h Show resolved Hide resolved

include/rocksdb/comparator.h Outdated Show resolved Hide resolved

include/rocksdb/comparator.h Outdated Show resolved Hide resolved

riversand963 force-pushed the ts_v4 branch from 75d368e to 075044f Compare April 29, 2019 18:39

siying approved these changes Apr 29, 2019

View reviewed changes

facebook-github-bot reviewed Apr 29, 2019

View reviewed changes

riversand963 force-pushed the ts_v4 branch from 5dbda3a to 73de7db Compare May 6, 2019 22:18

facebook-github-bot reviewed May 7, 2019

View reviewed changes

riversand963 force-pushed the ts_v4 branch from 4c8f19b to e236ec0 Compare May 17, 2019 00:58

facebook-github-bot reviewed May 17, 2019

View reviewed changes

riversand963 added 9 commits June 4, 2019 17:12

Address review comments on naming and code comments

1cb64da

Explicitly override CompareWithtoutTimestamp

96be975

Summary: Explicitly override ComparaWithoutTimestamp in BytewiseComparator and ReverseBytewiseComparator to avoid an extra virtual function call incurred by Comparator::CompareWithoutTimestamp.

Make timestamp_size_ non-const

ea99119

Summary: We need to make timestamp_size_ non const because we implicitly use assignment operator. Also make it private to disable subclass modifying its value. Finally, add an explicit overloaded assignment.

Add option app_clock

2f423d5

Fix test after adding clock

e546128

Minor fix to address linter warning

987b804

Revert the addition of app_clock

8f13293

Minor fix

fe0cfcc

Minor fix

0fb5748

riversand963 force-pushed the ts_v4 branch from c7a70af to 0fb5748 Compare June 5, 2019 00:13

riversand963 added 2 commits June 4, 2019 17:23

Change method signature use use const ref

a7b4338

Remove checks from Get/Put

9b3242a

riversand963 removed the WIP Work in progress label Jun 5, 2019

facebook-github-bot reviewed Jun 5, 2019

View reviewed changes

Update HISTORY

3acf5a7

facebook-github-bot reviewed Jun 6, 2019

View reviewed changes

facebook-github-bot closed this in 340ed4f Jun 6, 2019

riversand963 deleted the ts_v4 branch June 6, 2019 06:24

facebook-github-bot added the Merged label Jun 6, 2019

riversand963 mentioned this pull request Jun 24, 2019

Avoid user key copying for Get/Put/Write with user-timestamp #5502

Closed

baiwfg2 reviewed Nov 26, 2020

View reviewed changes

include/rocksdb/options.h Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for timestamp in Get/Put #5079

Add support for timestamp in Get/Put #5079

riversand963 commented Mar 18, 2019 •

edited

Loading

sagar0 left a comment

siying left a comment

siying Apr 25, 2019

riversand963 Apr 26, 2019

siying Apr 25, 2019

riversand963 Apr 26, 2019

siying Apr 25, 2019

riversand963 Apr 26, 2019

siying Apr 26, 2019

siying left a comment

siying left a comment

siying commented Apr 29, 2019

riversand963 commented Apr 29, 2019

facebook-github-bot left a comment

riversand963 commented May 1, 2019 •

edited

Loading

riversand963 commented May 6, 2019

facebook-github-bot commented May 6, 2019

riversand963 commented May 7, 2019

facebook-github-bot left a comment

facebook-github-bot commented May 7, 2019

facebook-github-bot commented May 17, 2019

facebook-github-bot left a comment

facebook-github-bot commented May 17, 2019

facebook-github-bot commented Jun 5, 2019

facebook-github-bot commented Jun 5, 2019

facebook-github-bot left a comment

facebook-github-bot commented Jun 6, 2019

facebook-github-bot left a comment

facebook-github-bot commented Jun 6, 2019

Add support for timestamp in Get/Put #5079

Add support for timestamp in Get/Put #5079

Conversation

riversand963 commented Mar 18, 2019 • edited Loading

sagar0 left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

siying Apr 25, 2019

Choose a reason for hiding this comment

riversand963 Apr 26, 2019

Choose a reason for hiding this comment

siying Apr 25, 2019

Choose a reason for hiding this comment

riversand963 Apr 26, 2019

Choose a reason for hiding this comment

siying Apr 25, 2019

Choose a reason for hiding this comment

riversand963 Apr 26, 2019

Choose a reason for hiding this comment

siying Apr 26, 2019

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

siying commented Apr 29, 2019

riversand963 commented Apr 29, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 commented May 1, 2019 • edited Loading

riversand963 commented May 6, 2019

facebook-github-bot commented May 6, 2019

riversand963 commented May 7, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 7, 2019

facebook-github-bot commented May 17, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 17, 2019

facebook-github-bot commented Jun 5, 2019

facebook-github-bot commented Jun 5, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 6, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 6, 2019

riversand963 commented Mar 18, 2019 •

edited

Loading

riversand963 commented May 1, 2019 •

edited

Loading