New API to get all merge operands for a Key #5604

vjnadimpalli · 2019-07-22T18:28:04Z

This is a new API added to db.h to allow for fetching all merge operands associated with a Key. The main motivation for this API is to support use cases where doing a full online merge is not necessary as it is performance sensitive. Example use-cases:

Update subset of columns and read subset of columns -
Imagine a SQL Table, a row is encoded as a K/V pair (as it is done in MyRocks). If there are many columns and users only updated one of them, we can use merge operator to reduce write amplification. While users only read one or two columns in the read query, this feature can avoid a full merging of the whole row, and save some CPU.
Updating very few attributes in a value which is a JSON-like document -
Updating one attribute can be done efficiently using merge operator, while reading back one attribute can be done more efficiently if we don't need to do a full merge.

API :
Status GetMergeOperands(
const ReadOptions& options, ColumnFamilyHandle* column_family,
const Slice& key, PinnableSlice* merge_operands,
GetMergeOperandsOptions* get_merge_operands_options,
int* number_of_operands)

Example usage :
int size = 100;
int number_of_operands = 0;
std::vector values(size);
GetMergeOperandsOptions merge_operands_info;
db_->GetMergeOperands(ReadOptions(), db_->DefaultColumnFamily(), "k1", values.data(), merge_operands_info, &number_of_operands);

Description :
Returns all the merge operands corresponding to the key. If the number of merge operands in DB is greater than merge_operands_options.expected_max_number_of_operands no merge operands are returned and status is Incomplete. Merge operands returned are in the order of insertion.
merge_operands-> Points to an array of at-least merge_operands_options.expected_max_number_of_operands and the caller is responsible for allocating it. If the status returned is Incomplete then number_of_operands will contain the total number of merge operands found in DB for key.

Test Plan:
Added unit test and perf test in db_bench that can be run using the command:
./db_bench -benchmarks=getmergeoperands --merge_operator=sortlist

vjnadimpalli · 2019-07-22T20:44:47Z

Some open questions:

Based on the current API users have to use trial and error to get the num_records to be exactly or more than the number of merge_records for a key in DB which may not be a pleasant experience to use the API. Ideally users shouldn't need to give num_records and just fetch all the merge operands. The reason its needed is because PinnableSlice doesn't support copy constructor hence dynamically sized std::vector can't be used to add new PinnableSlice's to the vector. To get around this a fixed size array is used so all the PinnableSlice's are pre-initialized in user code and a pointer to that is passed in the API along with its size to prevent overflow. (Resolved by returning the actual number of merge operands found)
Its possible that the Merge Operator is coded in a way that doesn't need to explore all the merge operands in which case the question is should the API still return all the merge operands (till a PUT/DELETE record is encountered) or respect the Merge Operator and check when to stop looking for more merge operands.

include/rocksdb/db.h

db/memtable.cc

table/get_context.cc

table/get_context.h

db/version_set.cc

db/version_set.h

db/memtable.cc

db/version_set.cc

table/get_context.cc

vjnadimpalli · 2019-07-29T23:19:26Z

utilities/merge_operators/sortlist.cc

+    try {
+      operand.push_back(std::stoi(std::string(begin, slice.data_)));
+    } catch (...) {
+      //			std::cout << "Malformed string: " <<


Had to add this because sometimes the data is garbage, not sure why. I also see compilation failed in one of the jobs with error: utilities/merge_operators/sortlist.cc:72:14: error: exception handling disabled, use -fexceptions to enable. Any suggestions?

Does it feel like a bug in test code or bug in code?
By putting the merge operator here, the code is compiled and built into production code (though not all users will be able to use it). So I hope we fix the code, and/or move it to the test file using it.

The bug is fixed and does this file still need to be moved to a different location?

siying

I'm glad the progress we've made to the PR and we are very closed.
I left some comments. Except the question about re-pinning merge operands, they are mostly code style comments.

include/rocksdb/db.h

db/db_impl/db_impl.h

db/db_impl/db_impl.cc

db/db_merge_operand_test.cc

utilities/merge_operators/sortlist.cc

utilities/merge_operators/sortlist.h

db/db_merge_operand_test.cc

siying

Good job!
Some extra comments are mostly coding style comments, and it's still nice to be addressed.

db/db_blob_index_test.cc

utilities/merge_operators/sortlist.h

siying · 2019-08-01T21:27:00Z

utilities/merge_operators/sortlist.h

+
+  void MakeVector(std::vector<int>& operand, Slice slice) const;
+
+  std::vector<int> Merge(std::vector<int>& left, std::vector<int>& right) const;


Can MakeVector() and Merge() private?

I can't make MakeVector private because I am using it in db_dench_tool.

siying · 2019-08-01T21:33:15Z

utilities/merge_operators/sortlist.cc

+  do {
+    const char* begin = slice.data_;
+    while (*slice.data_ != ',' && *slice.data_) slice.data_++;
+    operand.push_back(std::stoi(std::string(begin, slice.data_)));


Not a big deal since it's just a test code, but it makes more sense to have the test case to manage fixed 64 uint. You can find encoding of decoding fixed 64 uint in coding.h.

tools/db_bench_tool.cc

…ning the KV that was put. Added more test cases

… set

facebook-github-bot