feat(db): mdbx integration & table skeletons #15

joshieDo · 2022-10-05T12:38:41Z

I think it's a good first step.

Next follow-ups:

Start implementing the actual model types alongside their encoders/decoders.
Improve/add functions to access/manage libmdbx cursor as it is needed.
page_size/sizes/ open params

Theoretically, you insert stuff already, as long as it is a Vec<u8>. Check tests at crates/db/src/kv/mod.rs and some table declarations at crates/db/src/kv/tables.rs

crates/db/src/kv/cursor.rs

crates/db/src/kv/mod.rs

rakita · 2022-10-07T11:05:07Z

This is very nice and clean, amazing work.

We start with the Env which can be NonWrite or Write (this is EnvironmentKind) depending in what mode db was open.

Env can spawn transactions Tx that can be ReadOnly or ReadWrite (this is TransactionKind). Internally it needs Environment maybe to spawn additional Tx, not sure. That is why we have both generics in Tx structure.

Tx have generic functions over Tables.
For RO and RW we have get and cursor and for RW we have put, delete and clear functions

Table is a placeholder that specifies Key and Value that are inside db and how to parse them (SeekKey is used inside Cursor for seeking the exact key)

and Cursor that takes TransactionType and Table allows us iteration over values.

rakita · 2022-10-07T11:16:51Z

Only question, that is work for later, out of scope for this, is how are we going to abstract this, as in defining some trait that can use both mdbx and our In memory implementation (for example).

Maybe the abstraction cut is between Env and Tx as in define trait that has get,cursor,put, etc.?

gakonst · 2022-10-07T20:50:06Z

Maybe the abstraction cut is between Env and Tx as in define trait that has get,cursor,put, etc.?

This makes sense as a direction.

gakonst

this is looking great. a few nits.

gakonst · 2022-10-09T05:47:41Z

crates/db/src/kv/mod.rs

+                })
+                .set_flags(EnvironmentFlags {
+                    mode,
+                    no_rdahead: true,


the Erigon team on Discord had indicated that read ahead might be useful in some cases

added a TODO to reevaluate later

gakonst · 2022-10-09T05:50:40Z

crates/db/src/kv/mod.rs

+            let result = env.update(|tx| {
+                tx.put(PlainState, key, value.clone()).expect(ERROR_PUT);
+                200
+            });


this API is really sick.

gakonst · 2022-10-09T05:51:14Z

crates/db/src/kv/table.rs

@@ -0,0 +1,131 @@
+//! Table traits.
+#![allow(missing_docs)]


can remove and just add the docs to the encode/decode functions below?

gakonst · 2022-10-09T05:53:35Z

crates/db/src/kv/tables.rs

+//
+// TODO: Temporary types, until they're properly defined alongside with the Encode and Decode Trait
+//
+
+type ConfigKey = Vec<u8>;
+type ConfigValue = Vec<u8>;
+#[allow(non_camel_case_types)]
+type BNum_BHash = Vec<u8>;
+#[allow(non_camel_case_types)]
+type BNum_BHash_TxId = Vec<u8>;
+type RlpHeader = Vec<u8>;
+type RlpTotalDifficulty = Vec<u8>;
+type RlpTxBody = Vec<u8>;
+type Receipt = Vec<u8>;
+type NumTxesInBlock = u16; // TODO can it be u16
+type BNum = u64; // TODO check size
+type TxId = u64; // TODO check size
+type HeaderHash = U256;
+type PlainStateKey = Address; // TODO new type will have to account for address_incarna_skey as well
+type TxIdList = Vec<u8>;
+#[allow(non_camel_case_types)]
+type Address_StorageKey = Vec<u8>;
+type AccountBeforeTx = Vec<u8>;
+type StorageKeyBeforeTx = Vec<u8>;


what is your plan here?

the plan is to start implementing proper types for them, and remove them from here on further PRs

they're here now just as placeholders, and can be used by other crates if really necessary

gakonst · 2022-10-09T05:56:56Z

crates/db/src/kv/tx.rs

+    /// Deletes the `(key, value)` entry on `table`.
+    pub fn delete<T>(&self, table: T, key: T::Key, value: Option<T::Value>) -> Result<bool, KVError>


~~Why would we need to provide value as Some? isn't the key always enough to delete both of them?~~

nvm from the docs

The data parameter is NOT ignored regardless the database does support sorted duplicate data items or not. If the data parameter is Some only the matching data item will be deleted. Otherwise, if data parameter is None, any/all value(s) for specified key will be deleted.

Let's add that note about value: Option<_> in the function doc?

gakonst · 2022-10-09T06:00:08Z

crates/db/src/lib.rs

-
-//! Database bindings for reth.
-// TODO: Actually provide database bindings. For now, this is just a re-export of MDBX.
+//! DB primitives for reth.


i'd just delete this, if you have the 1st line above

gakonst · 2022-10-09T06:00:31Z

crates/db/src/utils.rs

+    let libmdbx_max_page_size = 0x10000;
+
+    if os_page_size < 4096 {
+        // May lead to errors if it's reduced further because of the potential size of the data.
+        4096
+    } else if os_page_size > libmdbx_max_page_size {
+        libmdbx_max_page_size
+    } else {
+        os_page_size
+    }


how does this work?

added a source on the libmdbx max page size.

about the minimum value... that seems to have been the experience of erigon.

in my understanding, as long as it's supported by the system, a bigger page results in better performance

onbjerg

marking here that it looks ok from my pov modulo georgios' comments 😄

gakonst

LGTM. Merging so we can iterate on it.

Please remove the static.rs in a follow-up PR, and add any glue tests as recommended so we don't have weird regressions later on.

gakonst · 2022-10-10T19:17:25Z

crates/db/src/kv/cursor.rs

+        }
+        self.cursor.next_dup_val().transpose()
+    }
+}


Maybe OK to not add tests given this mostly forwards to LibMDBX, but if you don't think adding some regression tests to make sure we don't break anything later on (or if we want to e.g. optimize / bench) that'd help

gakonst · 2022-10-10T19:24:17Z

crates/db/src/kv/mod.rs

+                    size: Some(0..0x100000),     // TODO: reevaluate
+                    growth_step: Some(0x100000), // TODO: reevaluate
+                    shrink_threshold: None,
+                    page_size: Some(PageSize::Set(default_page_size())),
+                })
+                .set_flags(EnvironmentFlags {
+                    mode,
+                    no_rdahead: true, // TODO: reevaluate
+                    coalesce: true,
+                    ..Default::default()
+                })


Let's track in an issue that we may want to make these configurable via CLI. Suspect we'll end up running benchmarks with various configs to compare perf.

joshieDo added 3 commits October 5, 2022 10:49

wip

b77281b

add table macro

2426fbf

add simple put get test with Address

70eb292

joshieDo changed the title ~~wip: kv mdbx~~ wip: feat(db) mdbx Oct 5, 2022

joshieDo added 5 commits October 6, 2022 09:07

add Env.view and Env.update

0159538

docs

14d7c06

slightly change the test

2458373

add initial table initialization and placeholders

5b9dc7c

lint & some

ef76ca2

joshieDo changed the title ~~wip: feat(db) mdbx~~ feat(db): mdbx integration & table skeletons Oct 6, 2022

joshieDo requested review from onbjerg and rakita and removed request for onbjerg and rakita October 6, 2022 12:24

replace String with str

c130ad3

onbjerg reviewed Oct 6, 2022

View reviewed changes

crates/db/src/kv/cursor.rs Outdated Show resolved Hide resolved

crates/db/src/kv/cursor.rs Show resolved Hide resolved

crates/db/src/kv/mod.rs Outdated Show resolved Hide resolved

Merge branch 'main' into joshie/kv_mdbx

a7f2f8f

add error.rs

a36cb71

gakonst requested changes Oct 9, 2022

View reviewed changes

onbjerg approved these changes Oct 9, 2022

View reviewed changes

joshieDo added 4 commits October 10, 2022 06:41

add docs to encode

940cb87

add docs

3cdc13d

clamp

86c216a

add source on libmdbx_max_page_size

bd5c9b9

gakonst approved these changes Oct 10, 2022

View reviewed changes

gakonst merged commit 60d3c64 into main Oct 10, 2022

gakonst deleted the joshie/kv_mdbx branch October 10, 2022 19:35

gakonst mentioned this pull request Oct 10, 2022

db: supply table as Type not as fn param #32

Merged

joshieDo mentioned this pull request Oct 14, 2022

Tracking: DB #66

Closed

6 tasks

anonymousGiga added a commit to anonymousGiga/reth that referenced this pull request Feb 20, 2024

support test oom th (paradigmxyz#15)

342d09a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(db): mdbx integration & table skeletons #15

feat(db): mdbx integration & table skeletons #15

joshieDo commented Oct 5, 2022 •

edited

Loading

rakita commented Oct 7, 2022 •

edited

Loading

rakita commented Oct 7, 2022

gakonst commented Oct 7, 2022

gakonst left a comment

gakonst Oct 9, 2022

joshieDo Oct 10, 2022

gakonst Oct 9, 2022

gakonst Oct 9, 2022

gakonst Oct 9, 2022

joshieDo Oct 10, 2022 •

edited

Loading

gakonst Oct 9, 2022

gakonst Oct 9, 2022

gakonst Oct 9, 2022

joshieDo Oct 10, 2022 •

edited

Loading

onbjerg left a comment

gakonst left a comment

gakonst Oct 10, 2022

gakonst Oct 10, 2022

		/// Deletes the `(key, value)` entry on `table`.
		pub fn delete<T>(&self, table: T, key: T::Key, value: Option<T::Value>) -> Result<bool, KVError>

feat(db): mdbx integration & table skeletons #15

feat(db): mdbx integration & table skeletons #15

Conversation

joshieDo commented Oct 5, 2022 • edited Loading

rakita commented Oct 7, 2022 • edited Loading

rakita commented Oct 7, 2022

gakonst commented Oct 7, 2022

gakonst left a 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

joshieDo Oct 10, 2022 • edited Loading

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

joshieDo Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

onbjerg left a comment

Choose a reason for hiding this comment

gakonst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshieDo commented Oct 5, 2022 •

edited

Loading

rakita commented Oct 7, 2022 •

edited

Loading

joshieDo Oct 10, 2022 •

edited

Loading

joshieDo Oct 10, 2022 •

edited

Loading