Column families support #34

bhartnett · 2024-02-13T09:30:44Z

The changes included in this PR are:

C header and nim wrapper updated to the latest RocksDB version which is v8.10.0.
Added support for column families.
Rewrote library to make it more modular in order to better support adding additional features in the future.
This new version of the library is following the design of the official Java RocksDB library. See the Java library docs here: https://javadoc.io/doc/org.rocksdb/rocksdbjni/latest/org/rocksdb/RocksDB.html

Each Nim type in the library wraps a c pointer from the c library which needs to be freed after use which is done by calling the close proc.

Note that these changes are breaking because the names of the types and procs have changed. I'm planning to add more features shortly to include support for RocksDB iterators, the WriteBatch API and RocksDB transactions.

This change doesn't yet solve the issue of linking to a specific version of RocksDB. Currently the Nim types are only using the c functions that are availabile in RockDB v5.14.2 and greater.

jangko · 2024-02-13T13:44:15Z

rocksdb.nim

+        db.db,
+        db.readOptions,
+        columnFamilyHandle,
+        cast[cstring](unsafeAddr key[0]),


nim 2.x remove support for unsafeAddr, and only support addr. You need to templatize unsafeAddr for nim 2.x.

Thanks, will do.

jangko · 2024-02-13T14:00:34Z

The CI is fixed. it yours now.

bhartnett · 2024-02-13T14:19:33Z

The CI is fixed. it yours now.

Thanks @jangko, much appreciated.

… 'default' column family.

…(temporary fix).

bhartnett · 2024-02-21T08:59:05Z

Ready for another review @jangko

jangko · 2024-02-26T02:42:12Z

rocksdb/rocksdb.nim

+  doAssert not db.isClosed()
+  db.cPtr
+
+proc get*(


This is just my idea. I think when put, get, delete, etc where columnFamily is expected, we can create a ColumnFamily object and use the overloaded version of put, get, delete where user don't have to write the columnFamily.
I think it will be more ergonomic.

Yes good idea, perhaps that can be a secondary way of using a column family. One of my goals when re-writing the library was to follow the style/naming/patterns of the official Java RocksDB library and you can see they supply the column family handle in the get/put/delete methods etc. See here: https://javadoc.io/doc/org.rocksdb/rocksdbjni/latest/org/rocksdb/RocksDB.html. I opted to use a string instead of exposing the handle in the API for simplicity.

For this I've opted to add a way to set the default column family on the RocksDbRef (in the next PR). It will provide a way to interact with a specific column family without having to pass it into every operation.

bhartnett · 2024-02-27T07:53:48Z

rocksdb/lib/headers/c.h

@@ -0,0 +1,3042 @@
+//  Copyright (c) 2011-present, Facebook, Inc.  All rights reserved.


Updated to match the latest RocksDB version which is 8.10.2

bhartnett · 2024-02-27T07:55:28Z

rocksdb/rocksdb.nim

+  RocksDbReadWriteRef* = ref object of RocksDbRef
+    writeOpts: WriteOptionsRef
+
+  DataProc* = proc(val: openArray[byte]) {.gcsafe, raises: [].}


The purpose of using this DataProc callback is to reduce the number of copies when getting data.

bhartnett · 2024-02-27T07:57:07Z

I've updated this one to merge into an integration branch. Additional features and improvements will be added to the next PR.

bhartnett · 2024-02-27T08:00:41Z

rocksdb/columnfamily/cfdescriptor.nim

+    options = defaultColFamilyOptions()): ColFamilyDescriptor =
+  ColFamilyDescriptor(name: name, options: options)
+
+template name*(descriptor: ColFamilyDescriptor): string =


I'm using templates for performance reasons to reduce the overhead of invoking a proc but I'm not sure if my understanding is correct here. Will nim automatically optimize small procs and inline them anyway?

bhartnett · 2024-02-27T08:02:32Z

rocksdb/columnfamily/cfhandle.nim

+
+# TODO: These procs below will not work unless using the latest version of rocksdb
+# Currently, when installing librocksdb-dev on linux the RocksDb version used is 6.11.4
+# Need to complete this task: https://github.com/status-im/nim-rocksdb/issues/10


Planning to get static linking working so we can load the latest version of RocksDb by default. Static linking will be better for security as well.

bhartnett · 2024-02-27T08:06:26Z

rocksdb/options/readopts.nim

+  doAssert not readOpts.isClosed()
+  readOpts.cPtr
+
+# TODO: Add setters and getters for read options properties.


RocksDb is very configurable and so each of these various options types can be updated in the future to support setting the required fields. For now we just use the default settings.

bhartnett · 2024-02-27T08:08:55Z

rocksdb/rocksdb.nim

+proc get*(
+    db: RocksDbRef,
+    key: openArray[byte],
+    columnFamily = "default"): RocksDBResult[seq[byte]] =


This version of get has an additional copy but is still exposed for convenience. The get which passes the data into the DataProc callback should be used if performance is desired.

bhartnett · 2024-02-27T08:12:34Z

rocksdb/rocksdb.nim

+    doAssert len == 0
+    ok(false)
+  else:
+    onData(toOpenArrayByte(data, 0, len.int - 1))


My understanding is that toOpenArrayByte doesn't copy the data and passing an array into a proc openArray parameter doesn't copy either. But turning the openArray into a seq still requires at least one copy. I'm wondering if we can further reduce the number of copies by using the sink and/or lent constructs?

kdeme

Note that these changes are breaking because the names of the types and procs have changed.

I skimmed fairly fast over the code as we are not using rocksdb in fluffy and its quite a chunk of changes to follow, but it seems like a good update & clean-up.

I think it would be nice to already accompany this PR with a PR in nimbus-eth1 that makes (some) of the changes already using the adjusted API (not talking about usage of column family).

In any case @jangko and/or @mjfh should probably give their approval too and not just me considering they are more into that code.

kdeme · 2024-02-27T10:01:16Z

rocksdb.nim

-  if not db.db.isNil:
-    rocksdb_close(db.db)
-    setFieldToNil(db)
+export backup, rocksdb


Add a newline to all files where one is missing (perhaps set-up your editor for this).

Sure, will do. I thought I had already configured this in vscode but apparently I missed that setting. Done now.

bhartnett · 2024-02-27T15:13:51Z

Note that these changes are breaking because the names of the types and procs have changed.

I skimmed fairly fast over the code as we are not using rocksdb in fluffy and its quite a chunk of changes to follow, but it seems like a good update & clean-up.

I think it would be nice to already accompany this PR with a PR in nimbus-eth1 that makes (some) of the changes already using the adjusted API (not talking about usage of column family).

In any case @jangko and/or @mjfh should probably give their approval too and not just me considering they are more into that code.

Thanks Kim. Sure, I'll create a PR on the Nimbus side and update the codebase to use the new version of the library.

* Column families support (#34) * Update library to support column families. If not specified, uses the 'default' column family. * Added tests for column family changes. * Update library version and readme. * Updated the librocksdb c library to the latest stable version. * Started rewrite of library. * Commit library rewrite progress. * Completed initial rewrite and refactored tests. * Completed implementation of backup engine. * Added tests for new types. * Completed tests for existing features. * Remove features not supported by older versions of RocksDB to fix CI (temporary fix). * Remove flush before backup support from BackupEngine to fix CI. * Transactions support (#36) * Update library to support column families. If not specified, uses the 'default' column family. * Added tests for column family changes. * Update library version and readme. * Updated the librocksdb c library to the latest stable version. * Started rewrite of library. * Commit library rewrite progress. * Completed initial rewrite and refactored tests. * Completed implementation of backup engine. * Added tests for new types. * Completed tests for existing features. * Remove features not supported by older versions of RocksDB to fix CI (temporary fix). * Remove flush before backup support from BackupEngine to fix CI. * Implemented RocksDB iterator. * Implemented pairs iterator. * Completed implementation of WriteBatch with tests. * Fix example code. * Completed implementation of TransactionDb. * Support setting default column family. * Remove unneeded usage of var for ref type parameters. * Completed transactiondb tests. * Improve and refactor rocksdb test. * Added support for ingesting sst files using the SstFileWriter. (#37) * Create ColFamilyReadOnly and ColFamilyReadWrite types for using a specific column family. * Use inline pragma for small procs and add lock to RocksDbRef type close to prevent double free. * Added documentation for the public API. * Initial implementation of sst filewriter. * Added tests for sstfilewriter. * Documentation minor improvements.

bhartnett requested review from jangko, mjfh and kdeme February 13, 2024 09:30

jangko reviewed Feb 13, 2024

View reviewed changes

bhartnett added 2 commits February 13, 2024 22:29

Update library to support column families. If not specified, uses the…

6277dbe

… 'default' column family.

Added tests for column family changes.

47deef6

bhartnett force-pushed the column-families-support branch from 01fdcf9 to 47deef6 Compare February 13, 2024 14:30

bhartnett marked this pull request as draft February 14, 2024 04:32

bhartnett added 8 commits February 14, 2024 14:19

Update library version and readme.

0cea46d

Updated the librocksdb c library to the latest stable version.

1e097fb

Started rewrite of library.

55487c8

Commit library rewrite progress.

656e521

Completed initial rewrite and refactored tests.

2fbbe53

Completed implementation of backup engine.

dca162f

Added tests for new types.

6ad22e6

Completed tests for existing features.

2ffcffc

bhartnett marked this pull request as ready for review February 20, 2024 15:55

bhartnett marked this pull request as draft February 20, 2024 16:07

bhartnett added 2 commits February 21, 2024 14:11

Remove features not supported by older versions of RocksDB to fix CI …

55c2b29

…(temporary fix).

Remove flush before backup support from BackupEngine to fix CI.

60db46a

bhartnett marked this pull request as ready for review February 21, 2024 08:42

jangko reviewed Feb 26, 2024

View reviewed changes

bhartnett changed the base branch from master to rocksdb-rewrite-integration February 27, 2024 02:27

bhartnett commented Feb 27, 2024

View reviewed changes

kdeme approved these changes Feb 27, 2024

View reviewed changes

bhartnett merged commit 2587995 into rocksdb-rewrite-integration Feb 28, 2024
12 checks passed

bhartnett deleted the column-families-support branch February 28, 2024 02:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Column families support #34

Column families support #34

bhartnett commented Feb 13, 2024 •

edited

Loading

jangko Feb 13, 2024

bhartnett Feb 13, 2024

jangko commented Feb 13, 2024

bhartnett commented Feb 13, 2024 •

edited

Loading

bhartnett commented Feb 21, 2024

jangko Feb 26, 2024

bhartnett Feb 26, 2024

bhartnett Feb 27, 2024

bhartnett Feb 27, 2024

bhartnett Feb 27, 2024

bhartnett commented Feb 27, 2024

bhartnett Feb 27, 2024 •

edited

Loading

bhartnett Feb 27, 2024

bhartnett Feb 27, 2024 •

edited

Loading

bhartnett Feb 27, 2024

bhartnett Feb 27, 2024

kdeme left a comment

kdeme Feb 27, 2024

bhartnett Feb 27, 2024

bhartnett commented Feb 27, 2024

		@@ -0,0 +1,3042 @@
		// Copyright (c) 2011-present, Facebook, Inc. All rights reserved.

Column families support #34

Column families support #34

Conversation

bhartnett commented Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jangko commented Feb 13, 2024

bhartnett commented Feb 13, 2024 • edited Loading

bhartnett commented Feb 21, 2024

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

bhartnett commented Feb 27, 2024

bhartnett Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhartnett Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdeme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhartnett commented Feb 27, 2024

bhartnett commented Feb 13, 2024 •

edited

Loading

bhartnett commented Feb 13, 2024 •

edited

Loading

bhartnett Feb 27, 2024 •

edited

Loading

bhartnett Feb 27, 2024 •

edited

Loading