Experimental support for SST unique IDs #8990

pdillinger · 2021-10-06T05:54:09Z

Summary:

New public header unique_id.h and function GetUniqueIdFromTableProperties
which computes a universally unique identifier based on table properties
of table files from recent RocksDB versions.
Generation of DB session IDs is refactored so that they are
guaranteed unique in the lifetime of a process running RocksDB.
(SemiStructuredUniqueIdGen, new test included.) Along with file numbers,
this enables SST unique IDs to be guaranteed unique among SSTs generated
in a single process, and "better than random" between processes.
See https://github.com/pdillinger/unique_id
In addition to public API producing 'external' unique IDs, there is a function
for producing 'internal' unique IDs, with functions for converting between the
two. In short, the external ID is "safe" for things people might do with it, and
the internal ID enables more "power user" features for the future. Specifically,
the external ID goes through a hashing layer so that any subset of bits in the
external ID can be used as a hash of the full ID, while also preserving
uniqueness guarantees in the first 128 bits (bijective both on first 128 bits
and on full 192 bits).

Intended follow-up:

Use the internal unique IDs in cache keys. (Avoid conflicts with Introduce a mechanism to dump out blocks from block cache and re-insert to secondary cache #8912) (The file offset can be XORed into
the third 64-bit value of the unique ID.)
Publish the external unique IDs in FileStorageInfo (Add (Live)FileStorageInfo API #8968)

Test Plan: Unit tests added, and checking of unique ids in stress test.
NOTE in stress test we do not generate nearly enough files to thoroughly
stress uniqueness, but the test trims off pieces of the ID to check for
uniqueness so that we can infer (with some assumptions) stronger
properties in the aggregate.

Summary: * New public header unique_id.h and function GetUniqueIdFromTableProperties which computes a universally unique identifier based on table properties of table files from recent RocksDB versions. * Generation of DB session IDs is refactored so that they are guaranteed unique in the lifetime of a process running RocksDB. (SemiStructuredUniqueIdGen, new test included.) Along with file numbers, this enables SST unique IDs to be guaranteed unique among SSTs generated in a single process, and "better than random" between processes. See https://github.com/pdillinger/unique_id * TODO: explain internal vs. external ID Intended follow-up (avoid conflicts with facebook#8912): use the internal unique IDs in cache keys. (The file offset can be XORed into the third 64-bit value of the unique ID.) Test Plan: Unit tests added TODO: stress test support

facebook-github-bot · 2021-10-12T19:13:28Z

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

facebook-github-bot · 2021-10-14T04:44:22Z

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

facebook-github-bot · 2021-10-14T16:45:57Z

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

facebook-github-bot · 2021-10-14T17:14:39Z

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

facebook-github-bot · 2021-10-14T17:25:55Z

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

facebook-github-bot · 2021-10-14T18:04:15Z

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

mrambacher · 2021-10-15T10:41:55Z

db_stress_tool/db_stress_listener.cc

+    }
+  }
+  fprintf(stdout, "(Re-)verified %zu unique IDs\n", id_set_.size());
+  s = fs.ReopenWritableFile(path_, FileOptions(), &data_file_writer_,


Will this always work? You are opening a file for write that is still open for read. Not sure that all OS (e.g. Windows) supports that.

Maybe clear/close the readable file first?

Good catch, sure I can reset the reader

mrambacher · 2021-10-15T10:44:36Z

db_stress_tool/db_stress_listener.cc

+
+namespace ROCKSDB_NAMESPACE {
+
+#ifdef GFLAGS


Is there a reason this file depends on GFLAGS or is it just everything in db_stress_tool does?

Yes, everything in db_stress

mrambacher · 2021-10-15T10:45:23Z

db_stress_tool/db_stress_listener.h

 #include "file/filename.h"
+#include "rocksdb/file_system.h"
+#include "rocksdb/table_properties.h"
+#include "rocksdb/unique_id.h"
 #ifdef GFLAGS
 #pragma once


Shouldn't the pragma once be first in the file?

IDE has a mind of its own ;)

mrambacher · 2021-10-15T15:10:12Z

table/unique_id.cc

+  BijectiveHash2x64((*in_out)[1] + 17391078804906429400U,
+                    (*in_out)[0] + 6417269962128484497U, &hi, &lo);


Can you make these constants into constexpr or other constants? They are used at least a few times and it might be nice to somewhere (in a comment) outline the math behind them.

mrambacher · 2021-10-15T15:12:55Z

table/unique_id_impl.h

+
+#include <array>
+
+#include "rocksdb/unique_id.h"


Oddly enough, I am not sure this include is necessary. What is being used from unique_id here?

This header is a logical extension of that one, even if there isn't currently a dependency.

mrambacher · 2021-10-15T15:15:23Z

table/unique_id_impl.h

+
+namespace ROCKSDB_NAMESPACE {
+
+// Helper for GetUniqueIdFromTableProperties. This function can also be used


I am uncertain if these methods belong here, versus being static/private to the source file. Just checking if you mean for them to be semi-public and accessible outside of the source. At least a few of them do not sound as if they are meant for consumption outside of the file.

If I don't put it in a .h file in this PR, it will be put in .h in the next PR. ("Intended follow-up: Use the internal unique IDs in cache keys.")

mrambacher · 2021-10-15T15:16:57Z

table/unique_id_impl.h

+Status GetSstInternalUniqueId(const std::string &db_id,
+                              const std::string &db_session_id,
+                              uint64_t file_number,
+                              std::array<uint64_t, 3> *out);


It it worth defining this std::array as its own type (even just via using)

Sure. A previous draft of this work was very type-heavy (struct RocksUuid, etc.) and it made a mass of code. I may have over-corrected, and a type-using for this one seems good.

mrambacher · 2021-10-15T15:19:59Z

table/unique_id_impl.h

+// Convert numerical format to byte format for public API
+std::string EncodeUniqueIdBytes(const std::array<uint64_t, 3> &in);
+
+// Reformat a random value down to a compact but friendly format, 20 chars


This comment talks more about implementation details rather than the use of this function. This comment probably belongs in the C++ file rather than here. The comment does not seem to match what the function appears as if should be doing (generating a session id from two inputs?).

mrambacher · 2021-10-15T15:27:49Z

util/string_util.h

+// result is accurate mod 2^64.
+template <int kBase>
+inline bool ParseBaseChars(const char** buf, size_t n, uint64_t* v) {
+  while (n) {


What is the expectation of "v" on input? Does it need to be 0 or should it be cleared here?

I'll add a comment 👍

facebook-github-bot · 2021-10-16T16:05:35Z

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

facebook-github-bot · 2021-10-16T16:11:14Z

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

facebook-github-bot · 2021-10-16T16:13:30Z

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

mrambacher · 2021-10-17T20:27:39Z

include/rocksdb/unique_id.h

+// for an SST file from TableProperties. This is supported for block-based
+// table files created with RocksDB 6.24 and later. NotSupported will be


Is this support a function of properties that only exist in BB tables > 6.24? I see nothing here that appears specific to block-based files.

mrambacher · 2021-10-17T20:30:28Z

include/rocksdb/unique_id.h

+// first 128 bits. See https://github.com/pdillinger/unique_id
+// Using the full 192 bits, we expect to generate roughly 2^96 * sqrt(n)
+// files before first collision.
+Status GetUniqueIdFromTableProperties(const TableProperties &props,


Is there a reason for the numeric version of the string rather than always using/returning the human readable one? If there are issues with .c_str and the like, the human readable one seems much safer to use and manage under most scenarios and if the numeric one is required for something, it could be translated internally.

Alternatively, maybe it is better to flip them around and have the human readable the "default" and have a means to convert it to the numeric representation on demand.

I am always annoyed when an API only gives me access to "raw data" by going through "pretty printed" data. And users tend to be lazy and they're likely to use the human-readable version as key in a map<string, whatever> without thinking about the performance.

I could go back to something like #8362 with a class for unique IDs, providing operators, hash, ToHumanString, etc. Thoughts?

My personal preference is to have a "UniqueId" class and have methods off of it (ToHumanString, RawData, static FromTableProperties, etc) rather than having methods "floating" in namespace. This model better associates the functionality IMO and helps eliminate "bad" data passed into some of these methods.

Having said that, it is probably more of a stylistic question than a correctness question. If you think the class style would have benefits, please mark these methods as "experimental" so they can be more easily changed in a future release.

Marking experimental and will likely revise API in follow-up PR

mrambacher · 2021-10-17T20:32:26Z

table/table_properties.cc

+  AppendProperty(result, "unique ID",
+                 s.ok() ? UniqueIdToHumanString(id) : "N/A", prop_delim,
+                 kv_delim);
+


Would it be better to NOT add the unique id at all to the properties file if the GetUnique method failed? Otherwise, if you needed to compare it, would we need to check for "N/A" as a special case?

This is only used for human-readable "dump" operations, as in sst_dump --show_properties. Listing "N/A" provides more information because it can indicate that the sst_dump tool supports dumping unique IDs but that the file does not (have one).

Just for clarity purposes, this "unique ID" int he properties file will not be used by the existing code but is more of a forensics/debugging property? It will not be used as a cache key or the like? I am wondering if the SW will ever need to parse it (rather than simply display it).

I don't know what you are referring to as "properties file" but this is for forensics / debugging. We have some test code that parses this output, but it would be odd for others to parse it. Even if they do, the likelihood that they feed it old table files--and without noticing collisions on "N/A"--is low IMHO.

Sorry, I meant in the "TableProperties", not "properties file". Too much context switching. The question was if any code is going to read that "unique id" property from an SST file and attempt to do anything with it, other than print it out. Anything for correlation or for caching? If not, then N/A is fine.

mrambacher · 2021-10-17T20:35:18Z

table/table_test.cc

+}
+}  // namespace
+
+TEST_F(TablePropertyTest, UniqueIdsSchemaAndQuality) {


Should there be a test that the unique ID can be successfully retrieved from an existing SST file via its properties?

mrambacher

LGTM

mrambacher · 2021-10-18T17:55:52Z

include/rocksdb/unique_id.h

+// first 128 bits. See https://github.com/pdillinger/unique_id
+// Using the full 192 bits, we expect to generate roughly 2^96 * sqrt(n)
+// files before first collision.
+Status GetUniqueIdFromTableProperties(const TableProperties &props,


My personal preference is to have a "UniqueId" class and have methods off of it (ToHumanString, RawData, static FromTableProperties, etc) rather than having methods "floating" in namespace. This model better associates the functionality IMO and helps eliminate "bad" data passed into some of these methods.

Having said that, it is probably more of a stylistic question than a correctness question. If you think the class style would have benefits, please mark these methods as "experimental" so they can be more easily changed in a future release.

mrambacher · 2021-10-18T17:58:02Z

table/table_properties.cc

+  AppendProperty(result, "unique ID",
+                 s.ok() ? UniqueIdToHumanString(id) : "N/A", prop_delim,
+                 kv_delim);
+


Just for clarity purposes, this "unique ID" int he properties file will not be used by the existing code but is more of a forensics/debugging property? It will not be used as a cache key or the like? I am wondering if the SW will ever need to parse it (rather than simply display it).

facebook-github-bot · 2021-10-18T18:50:43Z

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

zhichao-cao · 2021-10-18T18:58:37Z

table/unique_id_impl.h

+// unique id, so can be manipulated in more ways but very carefully.
+// These must be long term stable to ensure GetUniqueIdFromTableProperties
+// is long term stable.
+Status GetSstInternalUniqueId(const std::string &db_id,


What's the relationship between the internal unique ID and the public one we can get belonging to the same SST table?

zhichao-cao · 2021-10-18T20:49:46Z

util/hash.h

+
+// Hash 128 bits to 128 bits, guaranteed not to lose data (equivalent to
+// Hash2x64 on 16 bytes little endian)
+void BijectiveHash2x64(uint64_t in_high64, uint64_t in_low64,


Maybe it would be better to add a little bit more explanation about this hash algorithm and the potential usage in the .h header?

zhichao-cao

LGTM in general and make sure the windows build failures are fixed.

facebook-github-bot · 2021-10-19T02:26:01Z

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

facebook-github-bot · 2021-10-19T02:45:26Z

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

facebook-github-bot · 2021-10-19T03:34:23Z

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

facebook-github-bot · 2021-10-19T04:05:55Z

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

facebook-github-bot · 2021-10-19T04:06:14Z

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

facebook-github-bot · 2021-10-19T04:44:39Z

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

facebook-github-bot · 2021-10-19T06:32:12Z

@pdillinger merged this pull request in ad5325a.

Summary: * New public header unique_id.h and function GetUniqueIdFromTableProperties which computes a universally unique identifier based on table properties of table files from recent RocksDB versions. * Generation of DB session IDs is refactored so that they are guaranteed unique in the lifetime of a process running RocksDB. (SemiStructuredUniqueIdGen, new test included.) Along with file numbers, this enables SST unique IDs to be guaranteed unique among SSTs generated in a single process, and "better than random" between processes. See https://github.com/pdillinger/unique_id * In addition to public API producing 'external' unique IDs, there is a function for producing 'internal' unique IDs, with functions for converting between the two. In short, the external ID is "safe" for things people might do with it, and the internal ID enables more "power user" features for the future. Specifically, the external ID goes through a hashing layer so that any subset of bits in the external ID can be used as a hash of the full ID, while also preserving uniqueness guarantees in the first 128 bits (bijective both on first 128 bits and on full 192 bits). Intended follow-up: * Use the internal unique IDs in cache keys. (Avoid conflicts with facebook/rocksdb#8912) (The file offset can be XORed into the third 64-bit value of the unique ID.) * Publish the external unique IDs in FileStorageInfo (facebook/rocksdb#8968) Pull Request resolved: facebook/rocksdb#8990 Test Plan: Unit tests added, and checking of unique ids in stress test. NOTE in stress test we do not generate nearly enough files to thoroughly stress uniqueness, but the test trims off pieces of the ID to check for uniqueness so that we can infer (with some assumptions) stronger properties in the aggregate. Reviewed By: zhichao-cao, mrambacher Differential Revision: D31582865 Pulled By: pdillinger fbshipit-source-id: 1f620c4c86af9abe2a8d177b9ccf2ad2b9f48243

facebook-github-bot added the CLA Signed label Oct 6, 2021

pdillinger added 2 commits October 6, 2021 09:38

Some fixes and adjustments

132b4f9

Another fix for rare compiler

5039fc1

pdillinger requested a review from zhichao-cao October 7, 2021 21:23

pdillinger added 7 commits October 8, 2021 08:50

Fix weird msvc issue; insert a static_cast for 32-bit

5bf8309

Merge remote-tracking branch 'origin/main' into sst_unique_id

5cd76f7

Merge remote-tracking branch 'origin/main' into sst_unique_id

b4b648e

Add stress test of unique IDs

ea8c60e

Merge remote-tracking branch 'origin/main' into sst_unique_id

880914e

Fix LITE, add a firewall

3dc3f2a

Fix !GFLAGS

f9add36

pdillinger added 3 commits October 13, 2021 12:11

Fix some lints

edfb52e

Use bijective XXH3_128bits instead

43e77e3

Add printing and reserve "all zeros" value

cef6777

pdillinger added 4 commits October 14, 2021 08:39

Minor cleanup

e3266dc

Public API: simplify, add human string

901d061

Merge remote-tracking branch 'origin/main' into sst_unique_id

41c46c4

Add to HISTORY

42b2de2

Fix for some compilers

142ab9a

mrambacher reviewed Oct 15, 2021

View reviewed changes

pdillinger added 2 commits October 16, 2021 08:23

Merge remote-tracking branch 'origin/main' into sst_unique_id

a0df30f

Some clean-up from review

16210e5

make format

71efdb0

mrambacher reviewed Oct 17, 2021

View reviewed changes

mrambacher approved these changes Oct 18, 2021

View reviewed changes

pdillinger changed the title ~~Support SST unique IDs~~ Experimental support for SST unique IDs Oct 18, 2021

pdillinger added 2 commits October 18, 2021 11:47

Improve testing, mark EXPERIMENTAL

5f1edf0

Merge remote-tracking branch 'origin/main' into sst_unique_id

4fdead7

zhichao-cao reviewed Oct 18, 2021

View reviewed changes

zhichao-cao approved these changes Oct 18, 2021

View reviewed changes

pdillinger added 2 commits October 18, 2021 19:20

Fix Windows

3797b71

Merge remote-tracking branch 'origin/main' into sst_unique_id

7b55920

HISTORY oops

e85f72b

Merge remote-tracking branch 'origin/main' into sst_unique_id

49fbea9

facebook-github-bot closed this in ad5325a Oct 19, 2021

facebook-github-bot added the Merged label Oct 19, 2021

aravind-wdc mentioned this pull request Oct 28, 2021

db_stress failure westerndigitalcorporation/zenfs#72

Closed

		BijectiveHash2x64((*in_out)[1] + 17391078804906429400U,
		(*in_out)[0] + 6417269962128484497U, &hi, &lo);


		namespace ROCKSDB_NAMESPACE {

		// Helper for GetUniqueIdFromTableProperties. This function can also be used

		// for an SST file from TableProperties. This is supported for block-based
		// table files created with RocksDB 6.24 and later. NotSupported will be

Experimental support for SST unique IDs #8990

Experimental support for SST unique IDs #8990

Conversation

pdillinger commented Oct 6, 2021 • edited Loading

facebook-github-bot commented Oct 12, 2021

facebook-github-bot commented Oct 14, 2021

facebook-github-bot commented Oct 14, 2021

facebook-github-bot commented Oct 14, 2021

facebook-github-bot commented Oct 14, 2021

facebook-github-bot commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 16, 2021

facebook-github-bot commented Oct 16, 2021

facebook-github-bot commented Oct 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhichao-cao left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 19, 2021

facebook-github-bot commented Oct 19, 2021

facebook-github-bot commented Oct 19, 2021

facebook-github-bot commented Oct 19, 2021

facebook-github-bot commented Oct 19, 2021

facebook-github-bot commented Oct 19, 2021

facebook-github-bot commented Oct 19, 2021

pdillinger commented Oct 6, 2021 •

edited

Loading