From f61416f44a2c11122df92c0bb281ac82eb823de5 Mon Sep 17 00:00:00 2001 From: Aleksey Shakhmatov Date: Mon, 3 Aug 2026 17:21:33 +0300 Subject: [PATCH] index/db: heap-allocate secondary indexes Collection.indexes held Index by value, so orderedRemove memmoved the whole ~5 KB struct and every *Index already handed out referred to a different index afterwards -- a query plan's `index` field, or a slice into an index's promoted-key buffer. The collection's own bookkeeping stayed consistent, which is why nothing noticed: only a caller holding a pointer across a drop could see it, and no test did. The new test does, and it is mutation-checked against the by-value code that this commit replaces: holding pointers to b_1 and c_1, then dropping a_1, the b_1 pointer reads "c_1". Now orderedRemove moves 8-byte pointers, the surviving indexes do not move, and only the removed one is freed. M0 needs this independently: an Index will own a file mapping once the node arena moves into the data file, and copying one by value would duplicate that ownership. Not done, though the milestone plan listed it: moving Index's inline scratch and promo buffers out of the struct. Their stated purpose was to keep those 5 KB out of a file-resident Index and to stop the memmove -- but only the node arena and overflow slab become file-resident, not the Index metadata, and boxing already fixed the memmove. Moving them would be churn with nothing left to buy. --- src/commands.zig | 2 +- src/db.zig | 120 ++++++++++++++++++++++++++++++++++++++++------- src/index.zig | 28 +++++------ 3 files changed, 117 insertions(+), 33 deletions(-) diff --git a/src/commands.zig b/src/commands.zig index 3499028..bf9de39 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -481,7 +481,7 @@ fn cmd_list_indexes(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void id_pairs[0] = .{ .key = "v", .value = .{ .int32 = 2 } }; id_pairs[1] = .{ .key = "key", .value = .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 1 } }} } }; values[0] = .{ .doc = try index_pairs_append(reply, id_pairs, "_id_") }; - for (coll.indexes.items, 0..) |*ix, i| { + for (coll.indexes.items, 0..) |ix, i| { // The pairs live in the reply arena (freed with it); the values // array below references them. var pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; diff --git a/src/db.zig b/src/db.zig index e211bde..678b0e1 100644 --- a/src/db.zig +++ b/src/db.zig @@ -35,8 +35,15 @@ pub const Collection = struct { slab: std.ArrayListUnmanaged(std.ArrayListUnmanaged(u8)), /// Flat offset where each segment begins; doc_bytes binary-searches it. seg_starts: std.ArrayListUnmanaged(u64), - /// Secondary indexes (persisted through the log). - indexes: std.ArrayListUnmanaged(index.Index), + /// Secondary indexes (persisted through the log). Heap-allocated, so an + /// `*Index` handed out by `find_index` or `create_index` stays valid when + /// a sibling index is dropped. Held by value, `orderedRemove` memmoved the + /// whole ~5 KB struct and every live pointer into the list -- a query + /// plan's `index` field, or a slice into an index's promoted-key buffer -- + /// silently aimed at a different index or past the end. Nothing exercised + /// that concurrently yet; the mmap work makes it worse, since an Index + /// will own a mapping. + indexes: std.ArrayListUnmanaged(*index.Index), /// Guards this collection's docs/slab/indexes. Writers take it /// exclusive, readers shared; never held while taking the catalog lock, /// and never more than one collection lock at a time. @@ -66,7 +73,7 @@ pub const Collection = struct { /// lookup: index lifetime (who calls Index.deinit, and when) is decided /// here rather than at each caller. pub fn find_index(self: *Collection, name: []const u8) ?*index.Index { - for (self.indexes.items) |*ix| { + for (self.indexes.items) |ix| { if (std.mem.eql(u8, ix.name, name)) return ix; } return null; @@ -114,11 +121,15 @@ pub const Collection = struct { } /// Remove and free the index with this name. Returns whether it existed. + /// `orderedRemove` now moves 8-byte pointers rather than whole Index + /// structs, so the surviving indexes do not move and pointers to them stay + /// valid; only the removed one dies, here. fn remove_index(self: *Collection, gpa: std.mem.Allocator, name: []const u8) bool { for (self.indexes.items, 0..) |ix, i| { if (std.mem.eql(u8, ix.name, name)) { - var removed = self.indexes.orderedRemove(i); - removed.deinit(gpa); + _ = self.indexes.orderedRemove(i); + ix.deinit(gpa); + gpa.destroy(ix); return true; } } @@ -232,7 +243,10 @@ pub const Engine = struct { self.live_docs -= coll.docs.count(); self.dead_docs += coll.docs.count(); coll.id_index.deinit(self.gpa); - for (coll.indexes.items) |*ix| ix.deinit(self.gpa); + for (coll.indexes.items) |ix| { + ix.deinit(self.gpa); + self.gpa.destroy(ix); + } coll.indexes.deinit(self.gpa); var doc_it = coll.docs.iterator(); while (doc_it.next()) |doc_entry| { @@ -269,7 +283,7 @@ pub const Engine = struct { // index removal, so the slice is safe for the call. const old_bytes = coll.doc_bytes(old.value); coll.id_index.remove_doc(self.gpa, old_bytes, old.key); - for (coll.indexes.items) |*ix| ix.remove_doc(self.gpa, old_bytes, old.key); + for (coll.indexes.items) |ix| ix.remove_doc(self.gpa, old_bytes, old.key); self.gpa.free(old.key); // This document's log record (and its slab bytes) just became garbage. // The fetchRemove above succeeded, so a live document was counted. @@ -540,7 +554,7 @@ pub const Engine = struct { for (built_list.items) |*b| b.built.deinit(self.gpa); built_list.deinit(self.gpa); } - for (coll.indexes.items) |*ix| { + for (coll.indexes.items) |ix| { var built = try ix.build_entries(self.gpa, doc_bytes, id_key); built_list.append(self.gpa, .{ .built = built, .ix = ix }) catch |err| { built.deinit(self.gpa); @@ -677,14 +691,27 @@ pub const Engine = struct { spec_doc: *const bson.Document, ) !*index.Index { const coll = try self.get_or_create_collection(db_name, coll_name); - var ix = try index.parse_spec(self.gpa, spec_doc); + const parsed = try index.parse_spec(self.gpa, spec_doc); + // Boxed before anything is built into it, so publishing is a pointer + // append rather than a struct copy. An Index will own a mapping once + // the arena is file-backed, and copying one then would duplicate that + // ownership. + const ix = self.gpa.create(index.Index) catch |err| { + var dead = parsed; + dead.deinit(self.gpa); + return err; + }; + ix.* = parsed; var committed = false; // Runs on every return path (including the idempotent no-op): the // parsed spec is only owned by the collection once committed. - defer if (!committed) ix.deinit(self.gpa); + defer if (!committed) { + ix.deinit(self.gpa); + self.gpa.destroy(ix); + }; if (coll.find_index(ix.name)) |existing| { - if (index.Index.spec_equal(existing, &ix)) return existing; + if (index.Index.spec_equal(existing, ix)) return existing; return error.IndexOptionsConflict; } @@ -709,7 +736,7 @@ pub const Engine = struct { coll.indexes.appendAssumeCapacity(ix); committed = true; - return &coll.indexes.items[coll.indexes.items.len - 1]; + return ix; } /// Remove a secondary index by name, persisting a drop record first. @@ -784,7 +811,7 @@ pub const Engine = struct { ids.deinit(self.gpa); } - for (coll.indexes.items) |*ix| { + for (coll.indexes.items) |ix| { const ttl = ix.ttl orelse continue; const cutoff: i128 = @as(i128, now_ms) - @as(i128, ttl) * 1000; // bson compare order ranks datetime above null, numbers @@ -1067,7 +1094,7 @@ pub const Engine = struct { defer coll.lock.unlock(self.io); // Re-emit the index definitions first: a compacted log that dropped // them would resurrect the collections without indexes on replay. - for (coll.indexes.items) |*ix| { + for (coll.indexes.items) |ix| { var spec_bytes: std.ArrayListUnmanaged(u8) = .empty; defer spec_bytes.deinit(self.gpa); try ix.write_spec(self.gpa, &spec_bytes); @@ -1093,7 +1120,7 @@ pub const Engine = struct { while (db_it.next()) |db_entry| { var coll_it = db_entry.value_ptr.collections.iterator(); while (coll_it.next()) |coll_entry| { - for (coll_entry.value_ptr.*.indexes.items) |*ix| { + for (coll_entry.value_ptr.*.indexes.items) |ix| { try self.rebuild_index(coll_entry.value_ptr.*, ix); } try self.rebuild_index(coll_entry.value_ptr.*, &coll_entry.value_ptr.*.id_index); @@ -1144,9 +1171,18 @@ pub const Engine = struct { coll: *Collection, spec_doc: *const bson.Document, ) !void { - var ix = try index.parse_spec(self.gpa, spec_doc); + const parsed = try index.parse_spec(self.gpa, spec_doc); + const ix = self.gpa.create(index.Index) catch |err| { + var dead = parsed; + dead.deinit(self.gpa); + return err; + }; + ix.* = parsed; var committed = false; - defer if (!committed) ix.deinit(self.gpa); + defer if (!committed) { + ix.deinit(self.gpa); + self.gpa.destroy(ix); + }; if (coll.find_index(ix.name) != null) return; try coll.indexes.append(self.gpa, ix); committed = true; @@ -1773,7 +1809,7 @@ fn index_count( key_value: bson.Value, ) !usize { const coll = engine.get_collection(db_name, coll_name) orelse return 0; - for (coll.indexes.items) |*ix| { + for (coll.indexes.items) |ix| { if (std.mem.eql(u8, ix.name, name)) { var out: std.ArrayListUnmanaged([]const u8) = .empty; defer out.deinit(gpa); @@ -1951,6 +1987,54 @@ test "index drop survives reopen" { engine2.unlock(); } +test "dropping an index does not move its siblings" { + // Indexes used to be stored by value, so `orderedRemove` memmoved the + // whole list and every `*Index` already handed out -- notably a query + // plan's `index` field -- silently referred to a *different* index + // afterwards. Nothing caught it: the collection's own bookkeeping stayed + // consistent, so only a caller holding a pointer across a drop would see + // it, and none of the tests did. + // + // Mutation check: restore `indexes` to ArrayListUnmanaged(index.Index) + // (with the by-value append/remove that goes with it) and the b_1 + // assertion below reads "c_1", because slot 1 now holds what used to be in + // slot 2. + var threaded: std.Io.Threaded = .init_single_threaded; + defer threaded.deinit(); + const env = test_env(&threaded); + const io = env.io; + const gpa = testing.allocator; + + var tmp = try TmpLog.init(gpa); + defer tmp.deinit(gpa); + var engine = try Engine.open(gpa, io, tmp.path); + defer engine.deinit(); + try engine.lock(); + defer engine.unlock(); + + for ([_][]const u8{ "a", "b", "c" }) |field| { + const name = try std.fmt.allocPrint(gpa, "{s}_1", .{field}); + defer gpa.free(name); + var spec = try index_spec(gpa, field, name, false, false, null); + defer spec.deinit(); + _ = try engine.create_index("app", "users", &spec); + } + + const coll = engine.get_collection("app", "users").?; + // Hold pointers across the drop, which is the whole point. + const b_ix = coll.find_index("b_1").?; + const c_ix = coll.find_index("c_1").?; + + try testing.expect(try engine.drop_index("app", "users", "a_1")); + + try testing.expectEqual(@as(usize, 2), coll.indexes.items.len); + try testing.expectEqualStrings("b_1", b_ix.name); + try testing.expectEqualStrings("c_1", c_ix.name); + // And they are still the collection's own indexes, not detached copies. + try testing.expectEqual(b_ix, coll.find_index("b_1").?); + try testing.expectEqual(c_ix, coll.find_index("c_1").?); +} + test "drop_collection frees indexes; log without index records replays" { var threaded: std.Io.Threaded = .init_single_threaded; defer threaded.deinit(); diff --git a/src/index.zig b/src/index.zig index da02a70..9b28b5a 100644 --- a/src/index.zig +++ b/src/index.zig @@ -1489,8 +1489,8 @@ pub const Index = struct { /// The index whose key pattern is exactly `key_pairs` (same paths, same /// order, same directions), or null. Keeps the IndexKey layout — and what /// counts as a match — inside this module. -pub fn find_by_key_pattern(indexes: []const Index, key_pairs: []const bson.Pair) ?*const Index { - for (indexes) |*ix| { +pub fn find_by_key_pattern(indexes: []const *Index, key_pairs: []const bson.Pair) ?*const Index { + for (indexes) |ix| { if (ix.keys.len != key_pairs.len) continue; var match = true; for (ix.keys, key_pairs) |k, kp| { @@ -1838,7 +1838,7 @@ pub const Plan = struct { pub fn plan( gpa: std.mem.Allocator, id_ix: ?*const Index, - indexes: []const Index, + indexes: []const *Index, filter: []const bson.Pair, sort: []const query.SortKey, ) !?Plan { @@ -1853,7 +1853,7 @@ pub fn plan( best = cand; } } - for (indexes) |*ix| { + for (indexes) |ix| { var cand = (try evaluate_index(gpa, ix, clauses.items, sort)) orelse continue; if (best) |b| { if (plan_better(&cand, &b)) { @@ -2883,7 +2883,7 @@ test "planner picks eq run, ranges, and bails on sparse null" { .{ .key = "a", .value = .{ .int32 = 1 } }, .{ .key = "b", .value = .{ .int32 = 2 } }, }; - var p = (try plan(gpa, null, &.{ix}, &f, &.{})).?; + var p = (try plan(gpa, null, &.{&ix}, &f, &.{})).?; defer p.deinit(gpa); try testing.expectEqual(@as(usize, 2), p.key_len()); try testing.expect(p.lo == null and p.hi == null); @@ -2894,7 +2894,7 @@ test "planner picks eq run, ranges, and bails on sparse null" { .{ .key = "a", .value = .{ .int32 = 1 } }, .{ .key = "b", .value = .{ .doc = &.{.{ .key = "$gt", .value = .{ .int32 = 2 } }} } }, }; - var p = (try plan(gpa, null, &.{ix}, &f, &.{})).?; + var p = (try plan(gpa, null, &.{&ix}, &f, &.{})).?; defer p.deinit(gpa); try testing.expectEqual(@as(usize, 1), p.key_len()); try testing.expect(p.hi == null and p.lo != null and !p.lo_incl); @@ -2902,14 +2902,14 @@ test "planner picks eq run, ranges, and bails on sparse null" { // {a: 1} only → prefix run of 1. { const f = [_]bson.Pair{.{ .key = "a", .value = .{ .int32 = 1 } }}; - var p = (try plan(gpa, null, &.{ix}, &f, &.{})).?; + var p = (try plan(gpa, null, &.{&ix}, &f, &.{})).?; defer p.deinit(gpa); try testing.expectEqual(@as(usize, 1), p.key_len()); } // Pure range on the first key → key_len 0 with a bound. { const f = [_]bson.Pair{.{ .key = "a", .value = .{ .doc = &.{.{ .key = "$gte", .value = .{ .int32 = 1 } }} } }}; - var p = (try plan(gpa, null, &.{ix}, &f, &.{})).?; + var p = (try plan(gpa, null, &.{&ix}, &f, &.{})).?; defer p.deinit(gpa); try testing.expectEqual(@as(usize, 0), p.key_len()); try testing.expect(p.lo != null and p.lo_incl); @@ -2917,23 +2917,23 @@ test "planner picks eq run, ranges, and bails on sparse null" { // Unusable filter → no plan. { const f = [_]bson.Pair{.{ .key = "a", .value = .{ .doc = &.{.{ .key = "$regex", .value = .{ .string = "^x" } }} } }}; - try testing.expect((try plan(gpa, null, &.{ix}, &f, &.{})) == null); + try testing.expect((try plan(gpa, null, &.{&ix}, &f, &.{})) == null); const or_f = [_]bson.Pair{.{ .key = "$or", .value = .{ .array = &.{ .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} }, } } }}; - try testing.expect((try plan(gpa, null, &.{ix}, &or_f, &.{})) == null); + try testing.expect((try plan(gpa, null, &.{&ix}, &or_f, &.{})) == null); } // Sparse index bails on a null component. { const f = [_]bson.Pair{.{ .key = "a", .value = .null }}; - try testing.expect((try plan(gpa, null, &.{sp}, &f, &.{})) == null); + try testing.expect((try plan(gpa, null, &.{&sp}, &f, &.{})) == null); // Non-sparse is fine with null. - var p = (try plan(gpa, null, &.{ix}, &f, &.{})).?; + var p = (try plan(gpa, null, &.{&ix}, &f, &.{})).?; defer p.deinit(gpa); try testing.expect(p.key_len() == 1); // A null inside $in bails too. const fin = [_]bson.Pair{.{ .key = "a", .value = .{ .doc = &.{.{ .key = "$in", .value = .{ .array = &.{ .{ .int32 = 1 }, .null } } }} } }}; - try testing.expect((try plan(gpa, null, &.{sp}, &fin, &.{})) == null); + try testing.expect((try plan(gpa, null, &.{&sp}, &fin, &.{})) == null); } // $in cartesian product is capped. { @@ -2944,6 +2944,6 @@ test "planner picks eq run, ranges, and bails on sparse null" { .{ .key = "b", .value = .{ .doc = &.{.{ .key = "$in", .value = .{ .array = &members } }} } }, }; // 20 * 20 = 400 > 100 → fall back to a scan. - try testing.expect((try plan(gpa, null, &.{ix}, &f, &.{})) == null); + try testing.expect((try plan(gpa, null, &.{&ix}, &f, &.{})) == null); } }