diff --git a/src/commands.zig b/src/commands.zig index 11f687b..f2d1b0a 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -920,7 +920,7 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"), else => return err, }; - ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen) catch |err| switch (err) { + const written = ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen) catch |err| switch (err) { error.DuplicateKey, error.DuplicateKeyIndex => { const e = try reply.arena_alloc().alloc(bson.Pair, 3); e[0] = .{ .key = "index", .value = .{ .int32 = @intCast(si) } }; @@ -931,7 +931,10 @@ fn cmd_update(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !void { }, else => return err, }; - n_modified += 1; + // `n` counts matches, `nModified` counts documents the update + // actually altered. A write that would store the same bytes is + // neither logged nor counted here. + if (written == .modified) n_modified += 1; } } @@ -1040,7 +1043,9 @@ fn cmd_find_and_modify(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !v error.ImmutableId, error.InvalidUpdate => return bad_value(reply, "bad update"), else => return err, }; - try ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen); + // findAndModify reports `n` (matched) and `updatedExisting`, neither of + // which distinguishes a no-op, so whether it wrote is not needed here. + _ = try ctx.engine.replace(db_name, coll_name, copy, ctx.oid_gen); n = 1; updated_existing = true; value = if (ret_new) try project_doc(reply, copy, proj_pairs) else .{ .doc = before }; diff --git a/src/db.zig b/src/db.zig index 3d7929d..1049ab9 100644 --- a/src/db.zig +++ b/src/db.zig @@ -685,17 +685,24 @@ pub const Engine = struct { doc: *const bson.Document, oid_gen: *bson.ObjectIdGen, ) !void { - return self.upsert(db_name, coll_name, doc, oid_gen, .insert); + _ = try self.upsert(db_name, coll_name, doc, oid_gen, .insert); } + /// Whether a write changed anything. A replace whose result is byte-identical + /// to what is stored is not an error and not a write: MongoDB reports it as + /// matched but not modified, and writes no oplog entry for it. + pub const Written = enum { modified, unchanged }; + /// Insert or replace a document by _id (upsert without existence check). + /// Returns `.unchanged` when the stored document already had these exact + /// bytes -- see `Written`. pub fn replace( self: *Engine, db_name: []const u8, coll_name: []const u8, doc: *const bson.Document, oid_gen: *bson.ObjectIdGen, - ) !void { + ) !Written { return self.upsert(db_name, coll_name, doc, oid_gen, .replace); } @@ -716,7 +723,7 @@ pub const Engine = struct { doc: *const bson.Document, oid_gen: *bson.ObjectIdGen, mode: enum { insert, replace }, - ) !void { + ) !Written { const coll = try self.get_or_create_collection(db_name, coll_name); const doc_bytes = try self.serialize_with_id(doc, oid_gen); defer self.gpa.free(doc_bytes); @@ -759,6 +766,20 @@ pub const Engine = struct { }; } + // A replace that would store the same bytes is not a write at all. It + // has to be decided here -- after the document is serialized, so the + // comparison is against what would actually be stored, and before the + // log append, so a no-op costs no log record, no fsync, no slab bytes + // and no garbage. `nModified` is the visible half of this: MongoDB + // counts a document as modified only if the update altered it, so + // `$set: {x: 11}` on a document already holding `x: 11` is matched and + // not modified. + if (mode == .replace) { + if (coll.id_index.lookup_exact(id_enc)) |old_off| { + if (std.mem.eql(u8, coll.doc_bytes(old_off), doc_bytes)) return .unchanged; + } + } + // 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 @@ -818,6 +839,7 @@ pub const Engine = struct { self.release_write_reservations(coll); self.note_compact(); self.note_checkpoint(); + return .modified; } /// Remove a document by its `_id` value. Returns true if it existed. @@ -1117,12 +1139,33 @@ pub const Engine = struct { doc: *const bson.Document, oid_gen: *bson.ObjectIdGen, ) ![]u8 { - if (doc.get("_id") != null) return serialize_doc(self.gpa, doc); + // `_id` first, always -- generated here, or moved if the client put it + // later. MongoDB stores it first whatever order it arrives in, and the + // Node driver arrives in the other order: it fills a missing `_id` by + // assigning the property, which in JavaScript appends it, so an + // `insertOne({name, age})` reaches us as `{name, age, _id}`. + // + // Two things depend on this beyond field order in results. A replacement + // keeps `_id` at the front, so storing it elsewhere made replacing a + // document with itself a byte-level change and therefore a write. And + // the position is part of the stored bytes, so it has to be settled once, + // here, rather than by every reader. + if (doc.pairs.len > 0 and std.mem.eql(u8, doc.pairs[0].key, "_id")) { + return serialize_doc(self.gpa, doc); + } var pairs: std.ArrayListUnmanaged(bson.Pair) = .empty; defer pairs.deinit(self.gpa); - const oid = oid_gen.new(self.io); - try pairs.append(self.gpa, .{ .key = "_id", .value = .{ .object_id = oid } }); - try pairs.appendSlice(self.gpa, doc.pairs); + if (doc.get("_id")) |id| { + try pairs.append(self.gpa, .{ .key = "_id", .value = id }); + for (doc.pairs) |p| { + if (std.mem.eql(u8, p.key, "_id")) continue; + try pairs.append(self.gpa, p); + } + } else { + const oid = oid_gen.new(self.io); + try pairs.append(self.gpa, .{ .key = "_id", .value = .{ .object_id = oid } }); + try pairs.appendSlice(self.gpa, doc.pairs); + } var out: std.ArrayListUnmanaged(u8) = .empty; defer out.deinit(self.gpa); @@ -1974,7 +2017,7 @@ test "live/dead doc accounting drives compaction" { // A replace supersedes one record: live is unchanged, garbage grows. var d1b = try make_doc(gpa, 1, "alice2"); defer d1b.deinit(); - try engine.replace("app", "users", &d1b, &env.gen); + _ = try engine.replace("app", "users", &d1b, &env.gen); try testing.expectEqual(@as(u64, 2), engine.live_docs); try testing.expectEqual(@as(u64, 1), engine.dead_docs); @@ -2030,7 +2073,7 @@ test "compaction reclaims garbage but leaves a garbage-free log alone" { for (0..200) |i| { var d = try make_doc(gpa, @intCast(i), if (round % 2 == 0) "yy" else "z"); defer d.deinit(); - try engine.replace("app", "c", &d, &env.gen); + _ = try engine.replace("app", "c", &d, &env.gen); } try engine.commit(); if (engine.log.data_bytes >= after_insert * 2) break; @@ -2044,6 +2087,63 @@ test "compaction reclaims garbage but leaves a garbage-free log alone" { try testing.expect(engine.log.data_bytes < after_insert * 2); } +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 + // counts garbage that does not exist, and `replace` claims `.modified` -- + // which is what `nModified` reports to the client. + // + // MongoDB counts a document as modified only if the update altered it, and + // writes no oplog entry when it did not. `$set: {x: 11}` on a document + // already holding `x: 11` is matched and not modified. + 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); + try engine.lock(); + defer engine.unlock(); + + var d = try make_doc(gpa, 1, "alice"); + defer d.deinit(); + try engine.insert("app", "users", &d, &env.gen); + try engine.commit(); + const log_after_insert = engine.log.data_bytes; + + // The same document again, byte for byte. + var same = try make_doc(gpa, 1, "alice"); + defer same.deinit(); + try testing.expectEqual(Engine.Written.unchanged, try engine.replace("app", "users", &same, &env.gen)); + try engine.commit(); + try testing.expectEqual(log_after_insert, engine.log.data_bytes); + try testing.expectEqual(@as(u64, 0), engine.dead_docs); + try testing.expectEqual(@as(u64, 0), engine.dead_bytes); + try testing.expectEqual(@as(u64, 1), engine.live_docs); + + // A different one is a write, and is reported as one. + var changed = try make_doc(gpa, 1, "bob"); + defer changed.deinit(); + try testing.expectEqual(Engine.Written.modified, try engine.replace("app", "users", &changed, &env.gen)); + try engine.commit(); + try testing.expect(engine.log.data_bytes > log_after_insert); + try testing.expectEqual(@as(u64, 1), engine.dead_docs); + try testing.expectEqual(@as(u64, 1), engine.live_docs); + + // And the skipped write left the document readable and correctly indexed. + const id_enc = try id_key_for(gpa, bson.Value{ .int32 = 1 }); + defer gpa.free(id_enc); + const coll = engine.get_collection("app", "users").?; + const off = coll.id_index.lookup_exact(id_enc).?; + const stored = try bson.get_at(gpa, coll.doc_bytes(off), "name"); + try testing.expectEqualStrings("bob", stored.?.string); +} + test "compaction still triggers after a checkpoint has truncated the log" { // Mutation: gate `note_compact` on `self.log.data_bytes` (what it read // before the checkpoint existed) instead of `self.dead_bytes`. Red, because @@ -2090,7 +2190,7 @@ test "compaction still triggers after a checkpoint has truncated the log" { for (0..200) |i| { var d = try make_doc(gpa, @intCast(i), "yy"); defer d.deinit(); - try engine.replace("app", "c", &d, &env.gen); + _ = try engine.replace("app", "c", &d, &env.gen); } try engine.commit(); @@ -2138,7 +2238,7 @@ test "a rebuild leaves the space it reclaimed ready to reuse" { for (0..300) |i| { var d = try make_doc(gpa, @intCast(i), "yy"); defer d.deinit(); - try engine.replace("app", "c", &d, &env.gen); + _ = try engine.replace("app", "c", &d, &env.gen); } try engine.commit(); try testing.expect(engine.take_compact()); @@ -2153,7 +2253,7 @@ test "a rebuild leaves the space it reclaimed ready to reuse" { for (0..300) |i| { var d = try make_doc(gpa, @intCast(i), "zzz"); defer d.deinit(); - try engine.replace("app", "c", &d, &env.gen); + _ = try engine.replace("app", "c", &d, &env.gen); } try engine.commit(); try testing.expect(engine.pager.alloc_tail < tail_before + engine.pager.free_ready_pages() + 64); @@ -2184,7 +2284,7 @@ test "reopen carries the garbage counter across a restart" { for (0..50) |i| { var d = try make_doc(gpa, @intCast(i), "yy"); defer d.deinit(); - try engine.replace("app", "c", &d, &env.gen); + _ = try engine.replace("app", "c", &d, &env.gen); } try engine.commit(); try engine.checkpoint(); @@ -2431,7 +2531,7 @@ test "compact yields to a compaction already in flight" { var doc2 = try make_doc(gpa, 1, "alice-again"); defer doc2.deinit(); // A replace supersedes the first record, leaving it behind as garbage. - try engine.replace("app", "users", &doc2, &env.gen); + _ = try engine.replace("app", "users", &doc2, &env.gen); try engine.commit(); engine.unlock(); try testing.expect(engine.dead_docs > 0); @@ -2629,7 +2729,7 @@ test "unique index enforced on insert, replace, and upsert-conflict" { // A replace that keeps its own email is fine (own entries excluded). var d1b = try make_user(gpa, 1, "a@x.io"); defer d1b.deinit(); - try engine.replace("app", "users", &d1b, &env.gen); + _ = try engine.replace("app", "users", &d1b, &env.gen); try testing.expectEqual(@as(usize, 1), try index_count(gpa, &engine, "app", "users", "email_1", .{ .string = "a@x.io" })); // An update that would collide is rejected. @@ -2672,7 +2772,7 @@ test "index maintained across update and delete" { // Replace doc 1 with a new value: old entry gone, new entry present. var d1b = try doc_with_a(gpa, 1, 30); defer d1b.deinit(); - try engine.replace("app", "items", &d1b, &env.gen); + _ = try engine.replace("app", "items", &d1b, &env.gen); try testing.expectEqual(@as(usize, 0), try index_count(gpa, &engine, "app", "items", "a_1", .{ .int32 = 10 })); try testing.expectEqual(@as(usize, 1), try index_count(gpa, &engine, "app", "items", "a_1", .{ .int32 = 30 })); @@ -2946,7 +3046,7 @@ test "a rebuild reclaims dead document bytes and keeps every index valid" { while (i < 60) : (i += 1) { var d = try make_user(gpa, i, "b@x.io"); defer d.deinit(); - try engine.replace("app", "users", &d, &env.gen); + _ = try engine.replace("app", "users", &d, &env.gen); } engine.unlock(); diff --git a/tests/e2e/e2e.js b/tests/e2e/e2e.js index 351e5f2..cb57e78 100644 --- a/tests/e2e/e2e.js +++ b/tests/e2e/e2e.js @@ -60,6 +60,30 @@ async function main() { const um = await users.updateMany({}, { $set: { seen: true } }); check('updateMany', um.modifiedCount === 4, um); + // `_id` comes back first, whatever order the client sent it in. The driver + // fills a missing `_id` by assigning the property, which in JavaScript appends + // it, so this document reaches the server as {name, age, _id}. + const ordered = db.collection('ordering'); + await ordered.drop().catch(() => {}); + await ordered.insertOne({ zzz: 1, aaa: 2 }); + const orderedDoc = await ordered.findOne({}); + check('_id is stored first however the client ordered it', Object.keys(orderedDoc)[0] === '_id', Object.keys(orderedDoc).join(',')); + await ordered.insertOne({ mid: 1, _id: 'explicit', tail: 2 }); + const explicitDoc = await ordered.findOne({ _id: 'explicit' }); + check( + '_id moves to the front and the other fields keep their order', + Object.keys(explicitDoc).join(',') === '_id,mid,tail', + Object.keys(explicitDoc).join(','), + ); + + // Matched but not modified: MongoDB counts a document as modified only if the + // update altered it, and writes nothing when it did not. Setting a field to + // the value it already holds is the plain case. + const noop = await users.updateMany({}, { $set: { seen: true } }); + check('a no-op update matches without modifying', noop.matchedCount === 4 && noop.modifiedCount === 0, JSON.stringify(noop)); + const noopRep = await users.replaceOne({ name: 'alice' }, await users.findOne({ name: 'alice' })); + check('replacing a document with itself modifies nothing', noopRep.matchedCount === 1 && noopRep.modifiedCount === 0, JSON.stringify(noopRep)); + const push = await users.updateOne({ name: 'dave' }, { $push: { tags: 'x' } }); check('$push', push.modifiedCount === 1); check('$push visible', (await users.findOne({ name: 'dave' })).tags.length === 1); diff --git a/tests/spec/scorecard.txt b/tests/spec/scorecard.txt index 5a0d678..a182ab2 100644 --- a/tests/spec/scorecard.txt +++ b/tests/spec/scorecard.txt @@ -13,7 +13,7 @@ # semantics; ignoring them makes some cases pass that a full runner would # fail, so treat `pass` as an upper bound until M1 wires events up. -total 161 pass 131 fail 195 skip 175 files 0 errored +total 163 pass 129 fail 195 skip 175 files 0 errored # per-file: name pass fail skip aggregate-allowdiskuse.json 3 0 0 @@ -57,7 +57,7 @@ bulkWrite-updateOne-let.json 0 1 1 bulkWrite-updateOne-pipeline.json 0 1 0 bulkWrite-updateOne-rawdata.json 0 1 1 bulkWrite-updateOne-sort.json 1 0 1 -bulkWrite.json 8 2 0 +bulkWrite.json 10 0 0 bypassDocumentValidation.json 8 1 0 client-bulkWrite-delete-options.json 0 0 2 client-bulkWrite-delete-rawdata.json 0 0 2 @@ -213,7 +213,7 @@ aggregate.json SKIP aggregate with a document comment - pre 4.4 needs server <= aggregate.json SKIP aggregate with comment does not set comment on getMore - pre 4.4 needs server <= 4.3.99 bulkWrite-arrayFilters.json FAIL BulkWrite updateOne with arrayFilters outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}} bulkWrite-arrayFilters.json FAIL BulkWrite updateMany with arrayFilters outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}} -bulkWrite-arrayFilters.json FAIL BulkWrite with arrayFilters outcome crud-tests.test[0].y: expected an array, got {"$[i]":{"b":2}} +bulkWrite-arrayFilters.json FAIL BulkWrite with arrayFilters bulkWrite.modifiedCount: expected 3, got 2 bulkWrite-collation.json FAIL BulkWrite with delete operations and collation bulkWrite.deletedCount: expected 4, got 0 bulkWrite-collation.json FAIL BulkWrite with update operations and collation bulkWrite.matchedCount: expected 6, got 2 bulkWrite-comment.json SKIP BulkWrite with comment - pre 4.4 needs server <= 4.2.99 @@ -266,8 +266,6 @@ bulkWrite-updateOne-pipeline.json FAIL UpdateOne in bulk write using pipelines M bulkWrite-updateOne-rawdata.json SKIP BulkWrite updateOne with rawData option needs server >= 8.2.0 bulkWrite-updateOne-rawdata.json FAIL BulkWrite updateOne with rawData option on less than 8.2.0 - ignore argument MongoBulkWriteError: update spec requires u bulkWrite-updateOne-sort.json SKIP BulkWrite updateOne with sort option needs server >= 8.0 -bulkWrite.json FAIL BulkWrite with updateOne operations bulkWrite.modifiedCount: expected 1, got 2 -bulkWrite.json FAIL BulkWrite with updateMany operations bulkWrite.modifiedCount: expected 2, got 4 bypassDocumentValidation.json FAIL Aggregate with $out passes bypassDocumentValidation: false MongoServerError: Unrecognized pipeline stage name: '$out' client-bulkWrite-delete-options.json SKIP * needs server >= 8.0 client-bulkWrite-delete-rawdata.json SKIP client bulk write delete with rawData option needs server >= 8.2.0