diff --git a/src/commands.zig b/src/commands.zig index 75ee4d5..b3e6685 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -4291,7 +4291,9 @@ test "a rebuild kills an offsets cursor and spares a streaming one" { // // A rebuild is triggered directly rather than through churn, because whether // churn crosses the compaction threshold is not something a test should have - // to guess at. + // to guess at. The garbage below is still needed: `compact` now skips a + // collection with nothing to reclaim, so a clean one is not rewritten at + // all and there would be no rebuild to observe. var threaded: std.Io.Threaded = .init_single_threaded; defer threaded.deinit(); const io = threaded.io(); @@ -4315,6 +4317,25 @@ test "a rebuild kills an offsets cursor and spares a streaming one" { try testing.expectEqual(@as(i32, 0), try dispatch_get_more(&ctx, "c", walk.id)); try testing.expectEqual(@as(i32, 0), try dispatch_get_more(&ctx, "c", narrowed.id)); + // Rewrite every document, so the collection has as many dead bytes as live + // ones and is worth rebuilding. A replace rather than a delete: the count + // stays at 60, which is what the drain below checks. + try ctx.engine.lock(); + var again: i32 = 1; + while (again <= 60) : (again += 1) { + const pairs = try testing.allocator.alloc(bson.Pair, 3); + defer testing.allocator.free(pairs); + pairs[0] = .{ .key = "_id", .value = .{ .int32 = again } }; + // A different value: an identical replace is deliberately not a write. + pairs[1] = .{ .key = "a", .value = .{ .int32 = @mod(again, 5) + 100 } }; + pairs[2] = .{ .key = "pad", .value = .{ .string = seed_pad } }; + var doc: bson.Document = .{ .arena = std.heap.ArenaAllocator.init(testing.allocator), .pairs = pairs }; + defer doc.arena.deinit(); + _ = try ctx.engine.replace("test", "c", &doc, ctx.oid_gen); + } + try ctx.engine.commit(); + ctx.engine.unlock(); + try ctx.engine.compact(); // The stream remembers key bytes, which a repack does not change. diff --git a/src/db.zig b/src/db.zig index d3a8717..9d75958 100644 --- a/src/db.zig +++ b/src/db.zig @@ -1930,6 +1930,32 @@ pub const Engine = struct { try self.checkpoint(); } + /// Whether rewriting this collection would pay for itself. The caller holds + /// its lock. + /// + /// A rebuild copies a collection's live bytes to reclaim its dead ones, so + /// the one thing it must not do is copy a collection that has none. It used + /// to: `compact` walked every collection unconditionally, so garbage in one + /// paid for a full copy of the other thirty-nine. + /// + /// The share is the same one `note_compact` applies to the engine's totals, + /// and that is what keeps the two from disagreeing. If no collection passes + /// this test then `dead_i < live_i / 4` for every one of them, so + /// `sum(dead) < sum(live) / 4` and the engine's trigger could not have fired + /// either. So a compaction that runs always rebuilds at least one + /// collection, and cannot spin re-arming itself over garbage no rebuild will + /// take. An absolute floor per collection would break exactly that: forty + /// collections each under the floor can sum to well over it. + fn wants_rebuild(coll: *const Collection) bool { + assert_msg( + coll.slab_used >= coll.live_bytes, + "a collection cannot hold more live bytes than it ever appended", + ); + const dead = coll.slab_used - coll.live_bytes; + if (dead == 0) return false; + return dead * 4 >= coll.live_bytes; + } + /// Copy one collection's live documents into fresh extents and rebuild every /// index against the new offsets. /// @@ -1940,6 +1966,7 @@ pub const Engine = struct { fn rebuild_collection(self: *Engine, coll: *Collection) !void { try coll.lock.lock(self.io); defer coll.lock.unlock(self.io); + if (!wants_rebuild(coll)) return; var old_extents = try self.gpa.alloc(pgr.Extent, coll.slab_runs.items.len); defer self.gpa.free(old_extents); @@ -3752,6 +3779,62 @@ test "a churning collection reuses its slab instead of growing the file" { try testing.expect(grew < 3 * per_round * doc_size / pgr.page_size / 4); } +test "a rebuild copies only the collections that have garbage" { + // `compact` walked every collection unconditionally, so garbage in one paid + // for a full copy of all the others -- and a copy is not free even when it + // reclaims nothing: it rewrites every document and every index, and it + // bumps the layout epoch, which kills every open cursor on a collection + // that had no reason to be touched. + // + // The epoch is the observable, and it is also the user-visible harm: the + // clean collection's cursors survive. + // + // Mutation check: delete the `wants_rebuild` guard. Red -- the clean + // collection's epoch moves too. + var threaded: std.Io.Threaded = .init_single_threaded; + defer threaded.deinit(); + var 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(); + engine.compact_threshold = std.math.maxInt(u64); // rebuild only when told to + try engine.lock(); + + var i: i32 = 0; + while (i < 40) : (i += 1) { + var d = try make_padded(gpa, i, 3000); + defer d.deinit(); + try engine.insert("app", "dirty", &d, &env.gen); + var c = try make_padded(gpa, i, 3000); + defer c.deinit(); + try engine.insert("app", "clean", &c, &env.gen); + } + // Only one of them loses anything. + i = 0; + while (i < 40) : (i += 2) _ = try engine.remove_by_id("app", "dirty", .{ .int32 = i }); + try engine.commit(); + + const dirty = engine.get_collection("app", "dirty").?; + const clean = engine.get_collection("app", "clean").?; + const dirty_epoch = dirty.layout_epoch; + const clean_epoch = clean.layout_epoch; + const clean_used = clean.slab_used; + engine.unlock(); + + try engine.compact(); + + try testing.expect(dirty.layout_epoch != dirty_epoch); + try testing.expectEqual(clean_epoch, clean.layout_epoch); + // And it was not rewritten: a repack starts the slab over, so its byte + // count would not survive one unchanged. + try testing.expectEqual(clean_used, clean.slab_used); + try testing.expectEqual(@as(u64, 0), clean.slab_used - clean.live_bytes); +} + test "a replace that changes nothing is not a write" { // Mutation check: delete the byte comparison in `upsert`'s `.replace` arm. // Red on all three: the log grows, the document is superseded so the engine @@ -5513,6 +5596,11 @@ test "the epochs that invalidate a cursor move exactly when they must" { defer d.deinit(); try engine.insert("app", "c", &d, &env.gen); } + // Garbage, so the collection is worth rewriting: `compact` skips a + // collection with nothing to reclaim. + i = 0; + while (i < 20) : (i += 1) _ = try engine.remove_by_id("app", "c", .{ .int32 = i }); + try engine.commit(); const before = engine.get_collection("app", "c").?.layout_epoch; engine.unlock(); try testing.expect(before != 0);