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

@@ -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;

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).
+    /// Secondary indexes (persisted through the log). Heap-allocated, so an
-    indexes: std.ArrayListUnmanaged(index.Index),
+    /// `*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);
+                _ = self.indexes.orderedRemove(i);
-                removed.deinit(gpa);
+                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();

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 {
+pub fn find_by_key_pattern(indexes: []const *Index, key_pairs: []const bson.Pair) ?*const Index {
-    for (indexes) |*ix| {
+    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);
     }
 }