feat(ledger): support LMDB as a blockstore database backend #352

dnut · 2024-11-01T22:59:27Z

The bulk of this PR is an implementation for the blockstore's Database interface using LMDB, and adding more tests for the interface.

I also added a build option to customize the database backend, for example:

zig build -Dblockstore-db=lmdb

RocksDB remains the default for now.

For any build, only the necessary dependencies will be compiled. For example, lmdb will never be compiled, unless you specify -Dblockstore-db=lmdb.

A few bugs were uncovered in the hashmap db when testing the databases more strictly, so I fixed those in here as well.

src/ledger/database/interface.zig

Rexicon226

I've just gone through it with a glance, I haven't looked into it too deep. I will return to my laptop and do a second review pass.

build.zig

src/ledger/blockstore.zig

src/ledger/database/lmdb.zig

Rexicon226 · 2024-11-04T17:48:31Z

src/ledger/database/lmdb.zig

+    c.mdb_txn_abort(txn);
+}
+
+fn ret(constructor: anytype, args: anytype) LmdbError!TypeToCreate(constructor) {


This, and the functions under it are clearly relying on some behavior that makes no sense to me, a casual reader.
Could you document them better?

Rexicon226 · 2024-11-04T17:49:45Z

src/ledger/database/lmdb.zig

+                // open cf/database, creating if necessary
+                dbis[i] = try ret(c.mdb_dbi_open, .{
+                    txn,
+                    @as([*c]const u8, @ptrCast(cf.name)),


This is not a translated file, it should never use C pointers.

Two things here.

I took a quick glance at lmbd, and it calls strlen on the name passed in. This should be a [*:0]const u8 type. Either call allocator.dupeZ on the name, or just store cf.name as a [:0]const u8.

There is no need for a @ptrCast here, doing slice.ptr yields [*]T.

EDIT: lol, Sebastian has pointed out these exact same points in his review.

ac1e07d
b8adf69

Rexicon226 · 2024-11-04T17:54:12Z

src/ledger/database/lmdb.zig

+fn toVal(bytes: []const u8) c.MDB_val {
+    return .{
+        .mv_size = bytes.len,
+        .mv_data = @constCast(@ptrCast(bytes.ptr)),


I dunno about this. While I see what you're doing with this constCast, it can lead to very illegal behavior. One of the only use cases of constCast is to mitigate incorrect APIs, which I actually don't think this is.
I bet the implementation stores a mutable pointer because it frees it at some point. I have not looked into lmdb, so don't quote me on that.

Please take a []u8 here and duplicate constant strings or string literals instead. I refuse to use constCast anywhere in this codebase.

Please take a []u8 here and duplicate and constant strings or string literals instead.

This seems doable. I probably won't need to dupe anything. It will require some changes to preexisting code, but shouldn't be too bad.

I bet the implementation stores a mutable pointer because it frees it at some point.

toVal is used for inputs to LMDB. These are pointers that are allocated by the caller (in zig) and passed into an LMDB function as an input. They are not freed by LMDB.

One of the only use cases of constCast is to mitigate incorrect APIs, which I actually don't think this is.

My understanding is that when MDB_val is passed as an input into LMDB, the data behind the pointer is never mutated. It only reads the data.

Would you call that an incorrect API? If it says it takes mutable pointers but never mutates them?

it can lead to very illegal behavior

What kind of illegal behavior could happen in this situation?

Yeah lmdb's api is a bit odd w.r.t parameters there. As far as I understand it, yes, it will not modify that value at most call sites. The reason for this is some functions & options will use the key as an out param, e.g. mdb_cursor_get with MDB_GET_CURRENT will write to both the key and value params.

If you want to assert that it's const, I think it's best to be super explicit at the callsite of toVal. We wouldn't want to e.g. assert that something's const when it isn't (even if the memory is safely writeable).

Weighing in:

Would you call that an incorrect API? If it says it takes mutable pointers but never mutates them?

Yeah, this is explicitly an incorrect API; it doesn't adequately communicate its intent.

Thanks for explaining everyone, I agree lmdb is in the wrong here. Nevertheless, we should not use @constCast if we can avoid it.

What kind of illegal behavior could happen in this situation?

I originally assumed that either

lmdb is freeing the data at some point, since free takes a mutable pointer. freeing a piece of data in a rodata section is invalid and would result in a panic.

it's writing to the data somehow. writing to data in rodata is also invalid and would cause a fault.

Changing this to require a mutable pointer without adding any extra allocations is doable but awkward. It requires some extra complexity to distinguish between input types, which would need to be mutable, and output types, which should probably be immutable.

It's strange to require callers to always pass mutable pointers into the database, even though it will never be mutated. It might encourage callers to allocate dupes before passing it in, which would be pointless.

src/ledger/database/lmdb.zig

Rexicon226 · 2024-11-04T17:58:25Z

src/ledger/database/lmdb.zig

+    prev_multiple = 18,
+};
+
+fn result(int: isize) LmdbError!void {


I don't think result is the best name for this.

These seems more like a "convert errno to error set" function, so I'd recommend it returns an error set and then be wrapped in a "check for error return code" function if that makes sense.

I'd love to find a better name for this, but I don't see the point of splitting it into two functions.

Assuming the name is descriptive, I feel that it helps readability for the name to be concise, since it's used for every call to lmdb. I'm struggling to find a name that feels satisfying in that regard. I think "lmdbReturnCodeToErrorUnion" is sufficiently descriptive, but it feels like clutter.

How about maybeError, which reads nicely in try maybeError(int), ie "try unwrap this which is maybe an error".

Sobeston

Broad strokes seems good, got a few comments for now

Sobeston · 2024-11-04T20:01:03Z

src/ledger/database/lmdb.zig

+fn toVal(bytes: []const u8) c.MDB_val {
+    return .{
+        .mv_size = bytes.len,
+        .mv_data = @constCast(@ptrCast(bytes.ptr)),


Yeah lmdb's api is a bit odd w.r.t parameters there. As far as I understand it, yes, it will not modify that value at most call sites. The reason for this is some functions & options will use the key as an out param, e.g. mdb_cursor_get with MDB_GET_CURRENT will write to both the key and value params.

If you want to assert that it's const, I think it's best to be super explicit at the callsite of toVal. We wouldn't want to e.g. assert that something's const when it isn't (even if the memory is safely writeable).

src/ledger/database/lmdb.zig

src/ledger/database/hashmap.zig

src/ledger/database/interface.zig

dadepo · 2024-11-06T13:21:24Z

src/ledger/database/lmdb.zig

+const key_serializer = database.interface.key_serializer;
+const value_serializer = database.interface.value_serializer;
+
+pub fn LMDB(comptime column_families: []const ColumnFamily) type {


The RocksDB implementation has the logger as part of the struct's states. Any reason not to have same for the LMDB?

Also, the logger can be scoped.

The only reason rocksdb gets a logger is to communicate its errors through strings. Any time it returns an error, it also logs an error message, so the cause of the error is clear. LMDB only communicates errors through various error codes, which can be represented with zig errors.

dadepo · 2024-11-06T13:51:22Z

src/ledger/database/lmdb.zig

+        }
+
+        pub fn deinit(self: *Self) void {
+            self.allocator.free(self.dbis);


More of a question. There is also self.env, which is a pointer. What takes care of freeing that?

Yeah it should be closed here.

Adding this also revealed another bug, which I fixed in the same commit. Oddly, aborting write transactions seems to lead to memory corruption, which is detected as a double free when closing the env.

2b7c2da

src/ledger/database/hashmap.zig

Rexicon226 · 2024-11-06T17:19:19Z

Note that the "check_style" step that isn't "passing", is because #360 needs to be merged first, and this branch rebased onto it.
I changed the CI step name, and Github's branch protection rules work on specific names.

…d leads to memory leaks

the problem was in insertShreds because it was copying the write batch instead of getting a pointer. so it would only insert the things that are inserted by the pending state, not insertShreds

…quire all dependencies during partial test runs

There is a bizarre behavior of lmdb that is not documented anywhere. LMDB always reuses the same transaction state for every write transaction. A pointer to it is stored in the env struct and recycled. Aborting a transaction frees the transaction pointer. So if you abort a write transaction, it frees the only write transaction pointer. This corrupts the memory of lmdb because it will try to use the same pointer later as if it is valid. I can't understand how this behavior of lmdb is in any way sane or reasonable, so maybe I'm missing something. Anyway, when you close the env, it tries to free the write transaction, leading to a double free if you already aborted the transaction. that's why it cropped up during this change. so I'm just having it reset write transactions now, instead of abort, which should be fine

allocator was misused for generic recycling of resources. this broke with lmdb because it segfaults when Allocator.free attempts to overwrite the with `undefined`

dnut force-pushed the dnut/lmdb branch from e9db4ff to fc66788 Compare November 1, 2024 23:03

dnut marked this pull request as ready for review November 1, 2024 23:27

dnut requested review from Sobeston, dadepo, InKryption, Rexicon226 and 0xNineteen November 1, 2024 23:27

Rexicon226 reviewed Nov 2, 2024

View reviewed changes

src/ledger/database/interface.zig Outdated Show resolved Hide resolved

dnut force-pushed the dnut/lmdb branch from 83352f2 to e2910ce Compare November 4, 2024 17:18

0xNineteen removed request for InKryption and 0xNineteen November 4, 2024 17:19

Rexicon226 requested changes Nov 4, 2024

View reviewed changes

Sobeston requested changes Nov 4, 2024

View reviewed changes

dnut force-pushed the dnut/lmdb branch from 52d06dc to eaac69f Compare November 5, 2024 16:04

0xNineteen assigned dnut Nov 5, 2024

Sobeston reviewed Nov 5, 2024

View reviewed changes

src/ledger/database/lmdb.zig Outdated Show resolved Hide resolved

dadepo reviewed Nov 5, 2024

View reviewed changes

src/ledger/database/lmdb.zig Outdated Show resolved Hide resolved

dadepo reviewed Nov 5, 2024

View reviewed changes

src/ledger/database/lmdb.zig Outdated Show resolved Hide resolved

dnut force-pushed the dnut/lmdb branch from 0537d4d to f086a56 Compare November 5, 2024 21:32

dadepo reviewed Nov 6, 2024

View reviewed changes

src/ledger/database/hashmap.zig Show resolved Hide resolved

dadepo reviewed Nov 6, 2024

View reviewed changes

src/ledger/database/hashmap.zig Show resolved Hide resolved

dadepo reviewed Nov 6, 2024

View reviewed changes

src/ledger/database/interface.zig Outdated Show resolved Hide resolved

dadepo reviewed Nov 6, 2024

View reviewed changes

Rexicon226 reviewed Nov 6, 2024

View reviewed changes

src/ledger/database/hashmap.zig Outdated Show resolved Hide resolved

dnut added 4 commits November 6, 2024 18:25

feat(ledger): lmdb wip

17070a7

feat(ledger): wip lmdb

427fce7

feat(ledger): write last lmdb methods, but still doesn't compile

f1fd0cf

feat(ledger): lmdb compiles. tests fail

3fc08c7

dnut added 28 commits November 6, 2024 18:25

feat(ledger): get all the tests working for lmdb and add some more

3f0fe86

fix(ledger): remove unused

4c9a312

fix(ledger): misc bugs found when testing as blockstore db

8be72a0

feat(ledger): customizable blockstore database backend

d03475c

fix(ledger): hashmapdb compile error when used as blockstore database

d4682f1

test(ledger): migrate new database tests to normal zig tests

e1a261c

refactor(build.zig): use switch instead of ifs for database dependency

6f3bb7b

refactor(ledger): extract out import to top

876eb2d

refactor(ledger): remove c pointers from lmdb

79427fc

refactor(ledger): use flags from c import

1c22591

fix(ledger): lmdb error conversion needs platform specific logic

cd49700

refactor(ledger): remove unnecessary ptrCast from lmdb

c01b74f

refactor(ledger): rename lmdb "result" to "maybeError"

54975b2

fix(ledger): strings should be 0-terminated

11cd6cc

docs(ledger): document txn-based allocator for lmdb

e9d84f9

refactor(ledger): rename ret to returnOutput and add docs

2ed42a0

fix(ledger): reusing hashmap write batch after execution is a bug, an…

038f9e5

…d leads to memory leaks

fix(ledger): leak in purgeSlots

1808660

fix(ledger): serializeAlloc for raw bytes should not use bincode

8add233

fix(ledger): deinit shred bytes in test

dbe786a

fix(ledger): write batch copy/pointer mismanagement

e1d5fdc

the problem was in insertShreds because it was copying the write batch instead of getting a pointer. so it would only insert the things that are inserted by the pending state, not insertShreds

fix(ledger): memory leaks in tests

4710c13

ci: explicitly test ledger databases in github workflow, and don't re…

25bd099

…quire all dependencies during partial test runs

fix(ledger): memory bugs in the shred inserter

ac6ba87

test(ledger): improve deleteRange test

385aa53

test(ledger): run testDatabase(hashmap) tests no matter what

42e7608

fix(ledger): allocator misuse in BytesRef

53a9ea9

allocator was misused for generic recycling of resources. this broke with lmdb because it segfaults when Allocator.free attempts to overwrite the with `undefined`

dnut force-pushed the dnut/lmdb branch from 4075ff3 to 53a9ea9 Compare November 6, 2024 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ledger): support LMDB as a blockstore database backend #352

feat(ledger): support LMDB as a blockstore database backend #352

dnut commented Nov 1, 2024 •

edited

Loading

Rexicon226 left a comment

Rexicon226 Nov 4, 2024

dnut Nov 5, 2024

Rexicon226 Nov 4, 2024

dnut Nov 5, 2024

Rexicon226 Nov 5, 2024 •

edited

Loading

dnut Nov 5, 2024

Rexicon226 Nov 4, 2024 •

edited

Loading

dnut Nov 4, 2024

Sobeston Nov 4, 2024

InKryption Nov 4, 2024

Rexicon226 Nov 4, 2024

dnut Nov 5, 2024

Rexicon226 Nov 4, 2024

dnut Nov 4, 2024

InKryption Nov 4, 2024

dnut Nov 5, 2024

Sobeston left a comment

Sobeston Nov 4, 2024

dadepo Nov 6, 2024

dnut Nov 6, 2024

dadepo Nov 6, 2024

dnut Nov 6, 2024

Rexicon226 commented Nov 6, 2024 •

edited

Loading

feat(ledger): support LMDB as a blockstore database backend #352

Are you sure you want to change the base?

feat(ledger): support LMDB as a blockstore database backend #352

Conversation

dnut commented Nov 1, 2024 • edited Loading

Rexicon226 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

Rexicon226 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rexicon226 Nov 4, 2024 • 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

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

Sobeston 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

Rexicon226 commented Nov 6, 2024 • edited Loading

dnut commented Nov 1, 2024 •

edited

Loading

Rexicon226 Nov 5, 2024 •

edited

Loading

Rexicon226 Nov 4, 2024 •

edited

Loading

Rexicon226 commented Nov 6, 2024 •

edited

Loading