From aee23cb028833973e250a8a6b46eed8358abe59a Mon Sep 17 00:00:00 2001 From: Aleksey Shakhmatov Date: Mon, 3 Aug 2026 18:15:21 +0300 Subject: [PATCH] index/db: enforce _id uniqueness through the _id_ index _id uniqueness was a `coll.docs.contains` probe. The docs hashmap is going away (PLAN A3), so it has to move to the _id_ tree -- and the tree answers better, because it is keyed on bson.encode_key, which is canonical where serialize_value is not. int32 1, int64 1 and double 1.0 are now one _id, as they are in MongoDB (A4). _id_ is built and checked first, so a write violating both it and a unique secondary reports _id_, which is what MongoDB reports. It returns error.DuplicateKey with `dup_index` left null, which is exactly what commands.zig's E11000 rendering already treats as "the _id_ index", so the wire-visible message is unchanged and that file needed no edit. check_unique's exclude-self became optional and is null on an insert. That was a latent bug of its own: a replace must ignore its own existing entries, but an insert has none, and passing the document's id there hides a collision whose entry carries that same id -- precisely the case _id_ exists to catch. Only _id_ could reach it, since a secondary collision is between different documents. Two corrections found while doing this, both worth reading: PLAN A4 claimed a database already holding {_id: int32 1} and {_id: int64 1} loses one on reopen. It does not. Replay evicts through the docs map, keyed on serialize_value, so both survive; the tree is bulk-built afterwards with enforcement off, which tolerates duplicate keys and warns. The loss arrives only with the commit that drops the map, and that is where it needs a pre-flight scan. Amended. dispatch_insert asserted only `ok: 1`, but a rejected document comes back as a writeError alongside it -- so the mixed-type corpus silently shrank from ten documents to nine when _id_ became unique, and every test over it still passed. The helper now rejects writeErrors and asserts the inserted count; it caught the shrink immediately. The corpus keeps an int64 _id on a distinct value, and the collision it used to stand in for is asserted directly. Also adds Index.lookup_exact, which the commands that currently probe the docs map will need. Exact byte equality rather than cmp_prefix, because {a: 1}'s encoding is a proper prefix of {a: 1, b: 2}'s and a prefix match would claim a document is present when it is not. Mutation-checked, all three red: unique=false on id_index; exclude=id_key on insert; eql -> cmp_prefix in lookup_exact. --- PLAN.md | 22 +++++++++--- src/commands.zig | 92 +++++++++++++++++++++++++++++++++++++++++++++--- src/db.zig | 56 ++++++++++++++++++++--------- src/index.zig | 62 ++++++++++++++++++++++++++++++-- 4 files changed, 206 insertions(+), 26 deletions(-) diff --git a/PLAN.md b/PLAN.md index 9f057a5..243665c 100644 --- a/PLAN.md +++ b/PLAN.md @@ -301,10 +301,24 @@ in real MongoDB. Making `_id_` a `unique` index is therefore a of `src/index.zig` anticipated when it fenced off the old map fast path. Migration hazard, accepted deliberately: a database written by the current -code may legitimately hold two such documents, and replay evicts by -canonical key, so one is lost on reopen. Mitigation is a loud replay warning -naming the namespace and both `_id` values, plus a documented one-way -migration. Agreed with the human rather than assumed. +code may legitimately hold two such documents. Agreed with the human rather +than assumed. + +*Corrected while implementing:* the hazard does **not** bite when `_id_` +becomes unique, and the reason matters for sequencing. Replay does not +maintain `_id_` entries — `apply_record` evicts through the docs map, keyed on +`serialize_value`, and the tree is bulk-built afterwards by +`build_all_indexes`. That build calls `finish_bulk` with enforcement off, +which tolerates duplicate keys and already warns ("unique index '_id_' has +duplicate keys in existing data"), honouring the "database must always open" +rule. So such a database reopens with both documents intact and a loud +warning, and new colliding inserts are rejected from then on. + +The document *loss* arrives only with the commit that drops the docs hashmap, +because eviction then goes through the canonical tree and the second document +takes the first one's place. That commit is where this needs handling — a +pre-flight scan for compare-equal `_id`s, refusing to drop the map silently +while any exist — not here. --- diff --git a/src/commands.zig b/src/commands.zig index bf9de39..2e7692f 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -1944,6 +1944,16 @@ fn dispatch_insert( defer reply.deinit(); try dispatch(&ctx, &msg, &reply); try testing.expectEqual(@as(f64, 1.0), bson.get_pair(reply.pairs.items, "ok").?.double); + // `ok: 1` is not success for an insert batch: a rejected document comes + // back as a writeError alongside it. Asserting only `ok` let a corpus + // silently shrink -- when _id_ became a unique index, the mixed-type + // corpus below lost its int64 1 document and every test over it still + // passed, over nine documents instead of ten. + if (bson.get_pair(reply.pairs.items, "writeErrors")) |we| { + std.debug.print("dispatch_insert: unexpected writeErrors: {any}\n", .{we}); + return error.TestUnexpectedResult; + } + try testing.expectEqual(@as(i32, @intCast(docs.len)), bson.get_pair(reply.pairs.items, "n").?.int32); } /// Dispatch createIndexes for one spec. @@ -2341,6 +2351,73 @@ test "count_only_pipeline accepts only shapes a count can answer" { try testing.expect(try count_only_pipeline(&reply, &.{}) == null); } +test "compare-equal _id encodings collide under the unique _id_ index" { + // _id uniqueness moved from a docs-map probe keyed on serialize_value to + // the _id_ B+tree, keyed on the canonical bson.encode_key (PLAN A3/A4). + // That changes observable behavior, in MongoDB's direction: int32 1, + // int64 1 and double 1.0 are one _id, not three. + // + // Mutation check: setting `unique = false` back on id_index makes these + // collisions vanish (this test and "insert counts only successful inserts" + // both go red). + // + // The other half of the change is covered elsewhere, which is worth + // knowing so nobody re-checks it here: passing `id_key` instead of null as + // check_unique's exclude on the insert path is caught by "insert counts + // only successful inserts", not by this test. Two documents with the + // *same* encoding share an id_key, so exclude-self hides the collision; + // int32 1 and int64 1 have different id_keys, so this test survives that + // mutation. The two tests are complementary, not redundant. + var threaded: std.Io.Threaded = .init_single_threaded; + defer threaded.deinit(); + const io = threaded.io(); + var tdb = try TestDb.init(io); + defer tdb.deinit(); + + try dispatch_insert(&tdb, io, "ids", &.{ + .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 1 } }} }, + }); + + // Each of these is the same _id as int32 1. + for ([_]bson.Value{ + .{ .doc = &.{.{ .key = "_id", .value = .{ .int64 = 1 } }} }, + .{ .doc = &.{.{ .key = "_id", .value = .{ .double = 1.0 } }} }, + }) |dup| { + var ctx = tdb.ctx(io); + const one = [_]bson.Value{dup}; + var msg = try parse_fake_msg("insert", .{ .string = "ids" }, &.{ + .{ .key = "documents", .value = .{ .array = &one } }, + }); + defer msg.deinit(); + var reply = wire.Reply.init(testing.allocator); + defer reply.deinit(); + try dispatch(&ctx, &msg, &reply); + const we = bson.get_pair(reply.pairs.items, "writeErrors") orelse return error.TestUnexpectedResult; + const first = we.array[0]; + try testing.expectEqual(@as(i32, 11000), bson.get_pair(first.doc, "code").?.int32); + // The index named in the message is the one MongoDB names. + const errmsg = bson.get_pair(first.doc, "errmsg").?.string; + try testing.expect(std.mem.indexOf(u8, errmsg, "_id_") != null); + } + + // A different rank is a different _id: "1" is not 1. + try dispatch_insert(&tdb, io, "ids", &.{ + .{ .doc = &.{.{ .key = "_id", .value = .{ .string = "1" } }} }, + }); + + // And the collision is not merely rejection: a lookup by any of the + // equivalent encodings finds the one stored document. + for ([_]bson.Value{ .{ .int32 = 1 }, .{ .int64 = 1 }, .{ .double = 1.0 } }) |probe| { + var ids: std.ArrayListUnmanaged([]u8) = .empty; + defer { + for (ids.items) |id| testing.allocator.free(id); + ids.deinit(testing.allocator); + } + try dispatch_find_ids(&tdb, io, "ids", &.{.{ .key = "_id", .value = probe }}, &ids); + try testing.expectEqual(@as(usize, 1), ids.items.len); + } +} + test "indexed queries are equivalent to scans over a mixed corpus" { var threaded: std.Io.Threaded = .init_single_threaded; defer threaded.deinit(); @@ -2348,9 +2425,16 @@ test "indexed queries are equivalent to scans over a mixed corpus" { var tdb = try TestDb.init(io); defer tdb.deinit(); - // Corpus deliberately mixes numeric _id encodings (int32 1 and int64 1 - // compare equal but hash differently), arrays, nested docs, missing - // fields, explicit nulls, and duplicate values. + // Corpus deliberately mixes numeric _id encodings, arrays, nested docs, + // missing fields, explicit nulls, and duplicate values. + // + // It used to carry int32 1 and int64 1 as two documents, to exercise the + // old docs-map fast path (they compare equal but serialize differently). + // _id_ is a unique index now, keyed on the canonical bson.encode_key, so + // those two *are* the same _id and the second is rejected -- which is + // MongoDB's behavior. The int64 encoding still appears below, on a + // distinct value; the collision itself is asserted in "compare-equal _id + // encodings collide under the unique _id_ index". const corpus = [_]bson.Value{ .{ .doc = &.{ .{ .key = "_id", .value = .{ .int32 = 1 } }, @@ -2359,7 +2443,7 @@ test "indexed queries are equivalent to scans over a mixed corpus" { .{ .key = "tags", .value = .{ .array = &.{ .{ .string = "a" }, .{ .string = "b" } } } }, } }, .{ .doc = &.{ - .{ .key = "_id", .value = .{ .int64 = 1 } }, + .{ .key = "_id", .value = .{ .int64 = 2 } }, .{ .key = "a", .value = .{ .int32 = 20 } }, .{ .key = "b", .value = .{ .string = "y" } }, .{ .key = "tags", .value = .{ .array = &.{} } }, diff --git a/src/db.zig b/src/db.zig index 678b0e1..a75371d 100644 --- a/src/db.zig +++ b/src/db.zig @@ -65,7 +65,12 @@ pub const Collection = struct { fn init(gpa: std.mem.Allocator) !Collection { var self: Collection = .{ .docs = .empty, .slab = .empty, .seg_starts = .empty, .indexes = .empty, .id_index = undefined }; const keys = [_]index.IndexKey{.{ .path = "_id", .descending = false }}; - self.id_index = try index.Index.init(gpa, "_id_", &keys, false, false, null); + // unique: the tree, not the docs map, is what enforces _id uniqueness + // now (PLAN A3/A4). It is keyed on bson.encode_key, which is canonical + // where serialize_value is not, so int32 1 / int64 1 / double 1.0 + // collide as they do in MongoDB -- see the migration note in + // apply_record. + self.id_index = try index.Index.init(gpa, "_id_", &keys, true, false, null); return self; } @@ -554,6 +559,17 @@ pub const Engine = struct { for (built_list.items) |*b| b.built.deinit(self.gpa); built_list.deinit(self.gpa); } + { + // The implicit _id_ index, through the same protocol: reserved + // before the log append, inserted infallibly after it. Built + // *first* so it is checked first below -- MongoDB reports _id_ + // when a write violates both it and a unique secondary. + var built = try coll.id_index.build_entries(self.gpa, doc_bytes, id_key); + built_list.append(self.gpa, .{ .built = built, .ix = &coll.id_index }) catch |err| { + built.deinit(self.gpa); + return err; + }; + } 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| { @@ -561,24 +577,24 @@ pub const Engine = struct { return err; }; } - { - // The implicit _id_ index, through the same protocol: reserved - // before the log append, inserted infallibly after it. - var built = try coll.id_index.build_entries(self.gpa, doc_bytes, id_key); - built_list.append(self.gpa, .{ .built = built, .ix = &coll.id_index }) catch |err| { - built.deinit(self.gpa); - return err; - }; - } - // 2. The _id check, mirroring the pre-index behavior. - if (mode == .insert and coll.docs.contains(id_key)) return error.DuplicateKey; - - // 3. Unique secondary-index checks; a rejected write never reaches - // the log. + // 2. Unique-index checks, _id_ included; a rejected write never + // reaches the log. `_id` uniqueness used to be a `docs.contains` + // probe here, which the docs map will not be around to answer + // (PLAN amendment A3) -- and the tree answers it better, since it + // is keyed on the canonical encode_key rather than serialize_value + // (A4). Exclude-self is null for an insert: the document has no + // entries yet, and passing its id would hide precisely the + // same-_id collision this must catch. + const exclude: ?[]const u8 = if (mode == .replace) id_key else null; for (built_list.items) |*b| { if (!b.ix.unique) continue; - b.ix.check_unique(b.built.entries.items, id_key) catch { + b.ix.check_unique(b.built.entries.items, exclude) catch { + // The implicit index keeps its own error identity, so + // commands.zig renders E11000 with index "_id_" exactly as + // before and needs no change; `dup_index` stays null, which is + // what that rendering treats as "the _id_ index". + if (b.ix == &coll.id_index) return error.DuplicateKey; coll.dup_index = b.ix.name; return error.DuplicateKeyIndex; }; @@ -1261,6 +1277,14 @@ fn apply_record(ctx: *anyopaque, record: storage.Record, doc: *bson.Document) an key_owned = true; // The _id_ entry is added after replay, in build_all_indexes, // together with the secondary indexes. + // + // Which is why making _id_ unique cannot lose a document here: + // eviction above goes through the docs map, keyed on + // serialize_value, so a database holding both {_id: int32 1} and + // {_id: int64 1} keeps both. The bulk build then finds duplicate + // canonical keys, tolerates them and warns (rule: the database + // must always open). The commit that drops the docs map is where + // that stops being true -- see PLAN amendment A4. }, storage.record_type_delete => self.evict_doc(coll, id_key), else => {}, diff --git a/src/index.zig b/src/index.zig index 9b28b5a..f15d214 100644 --- a/src/index.zig +++ b/src/index.zig @@ -541,20 +541,38 @@ pub const Index = struct { /// Reject when any of `new_entries` has a key already present under a /// different id. Entries with `exclude_id` (the replacing document's /// own old entries) are allowed. + /// `exclude_id` is the document whose own existing entries do not count as + /// duplicates -- a replace re-inserts its entries, so its old ones must be + /// ignored. It must be **null for an insert**, where the document has no + /// entries yet: passing its id there would make a colliding entry with the + /// *same* id invisible, which is exactly the case the implicit `_id_` index + /// has to catch. pub fn check_unique( self: *const Index, new_entries: []const Entry, - exclude_id: []const u8, + exclude_id: ?[]const u8, ) error{DuplicateKeyIndex}!void { for (new_entries) |e| { var it = self.seek(e.key); while (it.next()) |have| { if (cmp_prefix(e.key, have.key) != .eq) break; - if (!std.mem.eql(u8, have.id, exclude_id)) return error.DuplicateKeyIndex; + const is_self = exclude_id != null and std.mem.eql(u8, have.id, exclude_id.?); + if (!is_self) return error.DuplicateKeyIndex; } } } + /// The id stored under exactly `key`, or null. Exact byte equality rather + /// than `cmp_prefix`, because this answers "is this document present" + /// rather than "what is in this key band". On a non-unique index it + /// returns the first entry of the band. + pub fn lookup_exact(self: *const Index, key: []const u8) ?[]const u8 { + var it = self.seek(key); + const e = it.next() orelse return null; + if (!std.mem.eql(u8, e.key, key)) return null; + return e.id; + } + // -- search ------------------------------------------------------------- /// All ids whose key equals `key` (component-wise). For a partial key @@ -2201,6 +2219,46 @@ test "unique conflict across documents, replace of own entries allowed" { try expect_ids(gpa, &ix, &.{.{ .int32 = 10 }}, &.{}); } +test "lookup_exact matches whole keys only" { + // The point of exact byte equality rather than cmp_prefix: this answers + // "is this document present", which is what the engine will ask once the + // docs hashmap is gone, and a prefix match would answer yes for a longer + // key that merely starts the same way. Compound keys make that concrete -- + // the encoding of {a: 1} is a prefix of the encoding of {a: 1, b: 2}. + const gpa = testing.allocator; + var ix = try simple_index(gpa, &.{ "a", "b" }, false, false); + defer ix.deinit(gpa); + + const d = try bytes_of(gpa, &.{ + .{ .key = "_id", .value = .{ .int32 = 1 } }, + .{ .key = "a", .value = .{ .int32 = 1 } }, + .{ .key = "b", .value = .{ .int32 = 2 } }, + }); + defer gpa.free(d); + _ = try ix.add_doc(gpa, d, "e1", false); + + var enc_full: std.ArrayListUnmanaged(u8) = .empty; + defer enc_full.deinit(gpa); + try bson.encode_key(.{ .int32 = 1 }, gpa, &enc_full); + var enc_prefix: std.ArrayListUnmanaged(u8) = .empty; + defer enc_prefix.deinit(gpa); + try enc_prefix.appendSlice(gpa, enc_full.items); + try bson.encode_key(.{ .int32 = 2 }, gpa, &enc_full); + + // The full two-column key hits. + try testing.expect(ix.lookup_exact(enc_full.items) != null); + try testing.expectEqualStrings("e1", ix.lookup_exact(enc_full.items).?); + // Its proper prefix must not, even though seek lands on the same entry. + // Mutation check: swapping the eql for cmp_prefix == .eq makes this line + // return "e1". + try testing.expect(ix.lookup_exact(enc_prefix.items) == null); + // And an absent key misses. + var enc_other: std.ArrayListUnmanaged(u8) = .empty; + defer enc_other.deinit(gpa); + try bson.encode_key(.{ .int32 = 9 }, gpa, &enc_other); + try testing.expect(ix.lookup_exact(enc_other.items) == null); +} + test "range bounds inclusive and exclusive" { const gpa = testing.allocator; var ix = try simple_index(gpa, &.{"a"}, false, false);