diff --git a/src/commands.zig b/src/commands.zig index a0e304b..0571523 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -81,6 +81,11 @@ pub const ErrorCode = enum(i32) { /// stage leaves the document with a different `_id` than it started with. immutable_field = 66, index_options_conflict = 85, + /// `IndexKeySpecsConflict`, measured on mongod 8.3.7: the same index name + /// over a different *set of documents*, which is what a differing + /// `partialFilterExpression` is -- as against a differing option, which is + /// 85 next door. + index_key_specs_conflict = 86, cannot_create_index = 67, invalid_index_specification_option = 197, // Cursor codes. Taken from MongoDB's own error_codes.js rather than @@ -934,39 +939,30 @@ fn cmd_create_indexes(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !vo if (name == .string and std.mem.eql(u8, name.string, "_id_")) { return bad_value(reply, "cannot create index with name '_id_'"); } - // Accepted and ignored until this commit, which is the worst of the - // three possible answers -- the same judgement `cmd_update` already - // makes about an update spec's `sort`. - // - // An index built over every document instead of the filtered subset - // still answers reads correctly: it holds a superset, never a subset. - // `unique` is where that stops being true. Measured on mongod 8.3.7: - // - // createIndex({a: 1}, {unique: true, partialFilterExpression: {t: true}}) - // insertMany([{a: 1, t: false}, {a: 1, t: false}]) - // - // mongod accepts both -- neither document is in the index, so neither - // collides -- and this server answered E11000. Unique-within-a-subset - // is the whole point of the option, so every use of it was a legal - // insert refused. The other two answers available were to keep doing - // that, or to echo the option back from `listIndexes` while not - // honouring it, which is a larger lie than saying no. - // - // See docs/M3_INDEX_TYPES_DESIGN_REVIEW.md. The implementation is the - // rest of M3's last row; this is what stands in until then. - if (bson.get_pair(spec, "partialFilterExpression") != null) { - return reply.put_error( - @intFromEnum(ErrorCode.cannot_create_index), - "CannotCreateIndex", - "partialFilterExpression is not implemented by this server: it would be " ++ - "accepted and ignored, and a unique index would then be enforced over " ++ - "documents the filter excludes", - ); - } - const spec_doc = bson.Document{ .arena = undefined, .pairs = spec }; _ = ctx.engine.create_index(db_name, coll_name, &spec_doc) catch |err| switch (err) { error.InvalidIndexSpec => return bad_value(reply, "invalid index spec"), + // A filter mongod would not accept either. It restricts the + // operators because every one of them narrows a set in a way + // another predicate can be checked against -- which is what a + // future implication test needs. `$ne` and `$regex` do not. + error.PartialFilterUnsupported => return reply.put_error( + @intFromEnum(ErrorCode.cannot_create_index), + "CannotCreateIndex", + "unsupported expression in partialFilterExpression", + ), + error.PartialFilterNotDocument => return reply.put_error( + @intFromEnum(ErrorCode.type_mismatch), + "TypeMismatch", + "partialFilterExpression must be a document", + ), + error.PartialAndSparse => return reply.put_error( + @intFromEnum(ErrorCode.cannot_create_index), + "CannotCreateIndex", + "cannot mix sparse and partialFilterExpression: a sparse index is a partial " ++ + "one whose filter is {: {$exists: true}}, and a document satisfying " ++ + "one and not the other has no defined answer", + ), error.TtlOnCompoundIndex => return reply.put_error( @intFromEnum(ErrorCode.cannot_create_index), "CannotCreateIndex", @@ -979,6 +975,11 @@ fn cmd_create_indexes(ctx: *Context, msg: *wire.Message, reply: *wire.Reply) !vo "between 0 and 2147483647", ), error.IndexOptionsConflict => return reply.put_error(@intFromEnum(ErrorCode.index_options_conflict), "IndexOptionsConflict", "index already exists with a different specification"), + error.IndexKeySpecsConflict => return reply.put_error( + @intFromEnum(ErrorCode.index_key_specs_conflict), + "IndexKeySpecsConflict", + "an index with the same name exists over a different set of documents", + ), error.DuplicateKeyIndex => { const ix_name = if (name == .string) name.string else "index"; const msg_text = try e11000_message(reply, db_name, coll_name, ix_name, try render_spec_key(reply, key_pairs)); @@ -5684,19 +5685,23 @@ test "TTL index round-trips through createIndexes/listIndexes; bad specs give 67 .{ .key = "key", .value = .{ .doc = &.{.{ .key = "_id", .value = .{ .int32 = 1 } }} } }, .{ .key = "expireAfterSeconds", .value = .{ .int32 = 60 } }, } } }, - // Same rule, different option: a partial filter accepted and ignored - // would enforce `unique` over documents the filter excludes. + // A partial filter holding an operator that narrows nothing another + // predicate could be checked against. .{ .code = 67, .spec = .{ .doc = &.{ .{ .key = "key", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, .{ .key = "partialFilterExpression", .value = .{ .doc = &.{ - .{ .key = "t", .value = .{ .bool = true } }, + .{ .key = "t", .value = .{ .doc = &.{.{ .key = "$ne", .value = .{ .int32 = 1 } }} } }, } } }, } } }, - // And with `unique`, which is the combination that made it a wrong - // answer rather than only a missing one. + // A filter that is not a document at all. + .{ .code = 14, .spec = .{ .doc = &.{ + .{ .key = "key", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, + .{ .key = "partialFilterExpression", .value = .{ .int32 = 1 } }, + } } }, + // Sparse and a partial filter overlap and may not be combined. .{ .code = 67, .spec = .{ .doc = &.{ - .{ .key = "key", .value = .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} } }, - .{ .key = "unique", .value = .{ .bool = true } }, + .{ .key = "key", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, + .{ .key = "sparse", .value = .{ .bool = true } }, .{ .key = "partialFilterExpression", .value = .{ .doc = &.{ .{ .key = "t", .value = .{ .bool = true } }, } } }, @@ -5714,10 +5719,6 @@ test "TTL index round-trips through createIndexes/listIndexes; bad specs give 67 try dispatch(&ctx, &msg, &reply); try testing.expectEqual(case.code, bson.get_pair(reply.pairs.items, "code").?.int32); } - // Mutation check for the two `partialFilterExpression` rows: delete the - // guard in `cmd_create_indexes` and both go green on the code -- and the - // second one's index then refuses `{a: 1, t: false}` twice, which mongod - // accepts because neither document is in the index at all. // Nothing partial was registered by the rejected specs. try testing.expectEqual(@as(usize, 1), tdb.engine.get_collection("test", "sessions").?.indexes.items.len); } diff --git a/src/db.zig b/src/db.zig index d80e1aa..546c166 100644 --- a/src/db.zig +++ b/src/db.zig @@ -1650,7 +1650,16 @@ pub const Engine = struct { if (coll.find_index(ix.name)) |existing| { if (index.Index.spec_equal(existing, ix)) return existing; - return error.IndexOptionsConflict; + // mongod splits the two, measured: a differing option under the + // same name is IndexOptionsConflict (85), and a differing + // *filter* is IndexKeySpecsConflict (86) -- the filter is part of + // which documents the index is over, not of how it behaves. + const filter_differs = (existing.partial == null) != (ix.partial == null) or + (existing.partial != null and bson.compare( + .{ .doc = existing.partial.?.pairs }, + .{ .doc = ix.partial.?.pairs }, + ) != .eq); + return if (filter_differs) error.IndexKeySpecsConflict else error.IndexOptionsConflict; } // Build entries over the existing documents (the index is not @@ -2297,10 +2306,17 @@ pub const Engine = struct { ix.dbg_root(); @panic("unreachable index entries"); } - if (ix.sparse or ix.multikey) continue; + // Three shapes hold fewer entries than there are + // documents, by design: a sparse index skips a missing + // path, a multikey one has no fixed ratio at all, and a + // partial one holds only what its filter selects. Every + // other index covers the collection, and an open that + // produced less than that has lost entries. + if (ix.sparse or ix.multikey or ix.partial != null) continue; assert_msg( ix.count() >= coll.doc_count, - "a non-sparse index must cover every document after an open", + "an index that is not sparse, multikey or partial must cover every " ++ + "document after an open", ); } } @@ -2486,8 +2502,19 @@ pub const Engine = struct { if (ix.sparse) flags |= 2; if (ix.multikey) flags |= 4; if (ix.ttl != null) flags |= 8; + // A fifth bit, and the format stays version 1: a file written before + // partial indexes existed never sets it, so it reads back exactly as + // it did. Same argument the free list used for its own change. + if (ix.partial != null) flags |= 16; try out.append(gpa, flags); try put_u64(gpa, out, @bitCast(ix.ttl orelse 0)); + if (ix.partial) |d| { + const bytes = try bson.serialize_value(gpa, .{ .doc = d.pairs }); + defer gpa.free(bytes); + // `serialize_value` prefixes the element type tag; the document + // body is what `Document.parse` reads back. + try put_bytes(gpa, out, bytes[1..]); + } try put_u32(gpa, out, ix.root); try put_u32(gpa, out, ix.first_leaf); try put_u32(gpa, out, ix.leaf_count); @@ -2605,6 +2632,16 @@ pub const Engine = struct { } const flags = try r.read_byte(); const ttl_raw: i64 = @bitCast(try r.read_u64()); + // Read in the same order it was written, and *before* the identity + // swap below: a corrupt filter has to fail with the index untouched. + var partial_doc: ?bson.Document = null; + errdefer if (partial_doc) |*d| d.deinit(); + var partial_pairs: ?[]const bson.Pair = null; + if (flags & 16 != 0) { + partial_doc = try bson.Document.parse(gpa, try r.read_bytes()); + partial_pairs = partial_doc.?.pairs; + } + defer if (partial_doc) |*d| d.deinit(); const new_name = try gpa.dupe(u8, name); errdefer gpa.free(new_name); @@ -2619,6 +2656,7 @@ pub const Engine = struct { ix.sparse = flags & 2 != 0; ix.multikey = flags & 4 != 0; ix.ttl = if (flags & 8 != 0) ttl_raw else null; + if (flags & 16 != 0) try ix.set_partial(gpa, partial_pairs.?); ix.root = try r.read_u32(); ix.first_leaf = try r.read_u32(); ix.leaf_count = try r.read_u32(); @@ -5933,3 +5971,124 @@ test "the epochs that invalidate a cursor move exactly when they must" { try testing.expect(coll.id_index.epoch != index_before); engine.unlock(); } + +/// An index spec with a `partialFilterExpression`, which `index_spec` has no +/// parameter for -- the filter is the only option that is a document. +fn partial_index_spec( + gpa: std.mem.Allocator, + path: []const u8, + name: []const u8, + unique: bool, + filter_key: []const u8, + filter_value: bson.Value, +) !bson.Document { + var arena = std.heap.ArenaAllocator.init(gpa); + errdefer arena.deinit(); + const a = arena.allocator(); + const key = try a.alloc(bson.Pair, 1); + key[0] = .{ .key = try a.dupe(u8, path), .value = .{ .int32 = 1 } }; + const filter = try a.alloc(bson.Pair, 1); + filter[0] = .{ .key = try a.dupe(u8, filter_key), .value = filter_value }; + const pairs = try a.alloc(bson.Pair, 4); + pairs[0] = .{ .key = try a.dupe(u8, "key"), .value = .{ .doc = key } }; + pairs[1] = .{ .key = try a.dupe(u8, "name"), .value = .{ .string = try a.dupe(u8, name) } }; + pairs[2] = .{ .key = try a.dupe(u8, "unique"), .value = .{ .bool = unique } }; + pairs[3] = .{ .key = try a.dupe(u8, "partialFilterExpression"), .value = .{ .doc = filter } }; + return .{ .arena = arena, .pairs = pairs }; +} + +fn doc_at(gpa: std.mem.Allocator, id: i32, a_val: i32, t: bool) !bson.Document { + var arena = std.heap.ArenaAllocator.init(gpa); + errdefer arena.deinit(); + const al = arena.allocator(); + const pairs = try al.alloc(bson.Pair, 3); + pairs[0] = .{ .key = try al.dupe(u8, "_id"), .value = .{ .int32 = id } }; + pairs[1] = .{ .key = try al.dupe(u8, "a"), .value = .{ .int32 = a_val } }; + pairs[2] = .{ .key = try al.dupe(u8, "t"), .value = .{ .bool = t } }; + return .{ .arena = arena, .pairs = pairs }; +} + +test "a unique partial index constrains only the documents its filter selects" { + // The row the whole feature exists for, and the one this server used to + // get wrong in the direction that refuses legal writes. + // + // Mutation check: delete the `self.partial` block in `build_entries` and + // the first insert pair goes red with E11000 -- which is exactly the + // answer this server gave before the filter was honoured. + 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(); + var spec = try partial_index_spec(gpa, "a", "a_1", true, "t", .{ .bool = true }); + defer spec.deinit(); + try engine.lock(); + defer engine.unlock(); + _ = try engine.create_index("app", "acct", &spec); + + // Two documents with the same `a`, both outside the filter: accepted, + // because neither is in the index at all. + for ([_]i32{ 1, 2 }) |id| { + var doc = try doc_at(gpa, id, 7, false); + defer doc.deinit(); + try engine.insert("app", "acct", &doc, &env.gen); + } + const coll = engine.get_collection("app", "acct").?; + try testing.expectEqual(@as(usize, 2), coll.id_index.count()); + try testing.expectEqual(@as(usize, 0), coll.indexes.items[0].count()); + + // The same value inside the filter: the first is indexed, the second + // collides. + var inside = try doc_at(gpa, 3, 9, true); + defer inside.deinit(); + try engine.insert("app", "acct", &inside, &env.gen); + try testing.expectEqual(@as(usize, 1), coll.indexes.items[0].count()); + var dup = try doc_at(gpa, 4, 9, true); + defer dup.deinit(); + try testing.expectError(error.DuplicateKeyIndex, engine.insert("app", "acct", &dup, &env.gen)); + + // A document leaving the filter frees the value it held. This is the + // half that needs the insert and the remove path to agree about which + // documents the index holds -- they do, because both go through + // `build_entries`. + var left = try doc_at(gpa, 3, 9, false); + defer left.deinit(); + _ = try engine.replace("app", "acct", &left, &env.gen); + try testing.expectEqual(@as(usize, 0), coll.indexes.items[0].count()); + var retake = try doc_at(gpa, 5, 9, true); + defer retake.deinit(); + try engine.insert("app", "acct", &retake, &env.gen); + try testing.expectEqual(@as(usize, 1), coll.indexes.items[0].count()); + + try engine.checkpoint(); + } + + // The filter is part of the index, so it has to come back with it: a + // reopen that forgot it would index every document and start refusing the + // writes above. Mutation check: drop the flag bit in + // `write_index_catalog` and this reopen sees an index over 4 documents. + { + var engine = try Engine.open(gpa, io, tmp.path); + defer engine.deinit(); + const coll = engine.get_collection("app", "acct").?; + try testing.expectEqual(@as(usize, 1), coll.indexes.items.len); + const ix = coll.indexes.items[0]; + try testing.expect(ix.partial != null); + try testing.expectEqual(@as(usize, 1), ix.count()); + + try engine.lock(); + defer engine.unlock(); + var dup = try doc_at(gpa, 6, 9, true); + defer dup.deinit(); + try testing.expectError(error.DuplicateKeyIndex, engine.insert("app", "acct", &dup, &env.gen)); + var outside = try doc_at(gpa, 7, 9, false); + defer outside.deinit(); + try engine.insert("app", "acct", &outside, &env.gen); + } +} diff --git a/src/index.zig b/src/index.zig index 0c3f93f..084ab72 100644 --- a/src/index.zig +++ b/src/index.zig @@ -194,6 +194,14 @@ pub const Index = struct { /// carries the setting. Always within /// [0, max_expire_after_seconds] — parse_spec is the only producer. ttl: ?i64, + /// The `partialFilterExpression` this index was created with, or null. + /// Owns its arena. + /// + /// Consulted in exactly one place -- `build_entries` -- which is what + /// keeps the insert and the remove path from ever disagreeing about which + /// documents this index holds. A filter applied on one side and not the + /// other would leave entries pointing at documents that are gone. + partial: ?bson.Document, multikey: bool, // -- the tree ---------------------------------------------------------- @@ -270,6 +278,7 @@ pub const Index = struct { .unique = unique, .sparse = sparse, .ttl = ttl, + .partial = null, .multikey = false, .pager = pager, .node_pages = .empty, @@ -321,6 +330,21 @@ pub const Index = struct { for (self.keys) |k| gpa.free(k.path); gpa.free(self.keys); gpa.free(self.name); + if (self.partial) |*d| d.deinit(); + } + + /// Attach a partial filter after construction. + /// + /// A separate call rather than an `init` parameter because `init` has nine + /// call sites and eight of them are harnesses that will never want one -- + /// and because the two that do (`parse_spec` and the catalog reader) both + /// already build the index first and settle its identity afterwards. + pub fn set_partial(self: *Index, gpa: std.mem.Allocator, pairs: []const bson.Pair) !void { + var arena = std.heap.ArenaAllocator.init(gpa); + errdefer arena.deinit(); + const owned = try bson.copy_pairs(arena.allocator(), pairs); + if (self.partial) |*old| old.deinit(); + self.partial = .{ .arena = arena, .pairs = owned }; } pub fn count(self: *const Index) usize { @@ -348,6 +372,17 @@ pub const Index = struct { defer arena.deinit(); const a = arena.allocator(); + // A partial index holds only the documents its filter selects. This is + // the *only* place that is decided, so an insert and the matching + // remove always agree -- the alternative, filtering at each call site, + // is how an index ends up with entries pointing at documents that no + // longer exist. + if (self.partial) |filter| { + if (!try query.matches_bytes(a, filter.pairs, doc)) { + return .{ .entries = .empty, .multikey = false }; + } + } + var per_path: std.ArrayListUnmanaged(std.ArrayListUnmanaged(bson.Value)) = .empty; var multikey = false; var multi_paths: usize = 0; @@ -1155,6 +1190,10 @@ pub const Index = struct { try out.append(arena, .{ .key = "name", .value = .{ .string = self.name } }); if (self.unique) try out.append(arena, .{ .key = "unique", .value = .{ .bool = true } }); if (self.sparse) try out.append(arena, .{ .key = "sparse", .value = .{ .bool = true } }); + // Before the expiry, which is the order mongod reports them in. + if (self.partial) |d| { + try out.append(arena, .{ .key = "partialFilterExpression", .value = .{ .doc = d.pairs } }); + } // int32 like MongoDB: parse_spec caps the value at // max_expire_after_seconds, so the cast always fits. if (self.ttl) |secs| try out.append(arena, .{ .key = "expireAfterSeconds", .value = .{ .int32 = @intCast(secs) } }); @@ -1164,6 +1203,11 @@ pub const Index = struct { if (!std.mem.eql(u8, a.name, b.name)) return false; if (a.unique != b.unique or a.sparse != b.sparse) return false; if (!std.meta.eql(a.ttl, b.ttl)) return false; + if ((a.partial == null) != (b.partial == null)) return false; + if (a.partial) |pa| { + const pb = b.partial.?; + if (bson.compare(.{ .doc = pa.pairs }, .{ .doc = pb.pairs }) != .eq) return false; + } if (a.keys.len != b.keys.len) return false; for (a.keys, b.keys) |ka, kb| { if (!std.mem.eql(u8, ka.path, kb.path)) return false; @@ -2061,16 +2105,77 @@ pub fn parse_spec(gpa: std.mem.Allocator, pager: *pgr.Pager, spec: *const bson.D // would leave it ambiguous which component dates the document. if (key_pairs.len != 1) return error.TtlOnCompoundIndex; } - const name_value = bson.get_pair(spec.pairs, "name") orelse { + var partial: ?[]const bson.Pair = null; + if (bson.get_pair(spec.pairs, "partialFilterExpression")) |v| { + partial = switch (v) { + .doc => |d| d, + else => return error.PartialFilterNotDocument, + }; + try check_partial_filter(partial.?); + // Measured: mongod refuses the combination outright rather than + // picking one. They overlap -- a sparse index is a partial one whose + // filter is `{path: {$exists: true}}` -- and a document satisfying one + // and not the other has no defined answer. + if (sparse) return error.PartialAndSparse; + } + + const name_value = bson.get_pair(spec.pairs, "name"); + var ix = if (name_value) |nv| blk: { + const name = switch (nv) { + .string => |s| s, + else => return error.InvalidIndexSpec, + }; + break :blk try Index.init(gpa, pager, name, keys[0..key_pairs.len], unique, sparse, ttl); + } else blk: { const nm = try default_name(gpa, key_pairs); defer gpa.free(nm); - return Index.init(gpa, pager, nm, keys[0..key_pairs.len], unique, sparse, ttl); + break :blk try Index.init(gpa, pager, nm, keys[0..key_pairs.len], unique, sparse, ttl); }; - const name = switch (name_value) { - .string => |s| s, - else => return error.InvalidIndexSpec, - }; - return Index.init(gpa, pager, name, keys[0..key_pairs.len], unique, sparse, ttl); + errdefer ix.deinit(gpa); + if (partial) |pf| try ix.set_partial(gpa, pf); + return ix; +} + +/// Which predicates a `partialFilterExpression` may hold. +/// +/// mongod restricts it, and the restriction is what makes a future +/// implication test possible: every operator here narrows a set in a way that +/// another predicate can be checked against. `$ne` and `$regex` do not, and +/// are refused -- both measured on 8.3.7, along with `$in`, which is +/// *allowed* and which this server would otherwise have grouped with them. +fn check_partial_filter(filter: []const bson.Pair) !void { + for (filter) |p| { + if (std.mem.eql(u8, p.key, "$and") or std.mem.eql(u8, p.key, "$or")) { + const branches = switch (p.value) { + .array => |a| a, + else => return error.PartialFilterUnsupported, + }; + for (branches) |b| switch (b) { + .doc => |sub| try check_partial_filter(sub), + else => return error.PartialFilterUnsupported, + }; + continue; + } + if (p.key.len > 0 and p.key[0] == '$') return error.PartialFilterUnsupported; + // A plain value is an equality, which is always allowed -- except a + // regex, which a driver sends as a BSON value rather than as + // `{$regex: ...}` and which mongod refuses either way. + const ops = switch (p.value) { + .doc => |d| d, + .regex => return error.PartialFilterUnsupported, + else => continue, + }; + if (!query.all_operator_keys(ops)) continue; + for (ops) |op| { + if (!partial_filter_operator(op.key)) return error.PartialFilterUnsupported; + } + } +} + +fn partial_filter_operator(name: []const u8) bool { + const allowed = [_][]const u8{ "$eq", "$gt", "$gte", "$lt", "$lte", "$in", "$exists", "$type" }; + for (allowed) |a| if (std.mem.eql(u8, name, a)) return true; + return false; } /// MongoDB's bound on expireAfterSeconds. Keeping it means a TTL always @@ -2434,6 +2539,13 @@ pub fn plan( } } for (indexes) |ix| { + // A partial index holds a *subset* of the collection, so answering a + // query from it is only correct when the query's predicates imply its + // filter. That implication test does not exist yet, and reading from + // the index without it returns too few documents -- the one failure + // worse than having no index at all. So it is maintained, it enforces + // `unique`, and reads scan. PLAN §6. + if (ix.partial != null) continue; var cand = (try evaluate_index(gpa, ix, clauses.items, sort)) orelse continue; if (best) |b| { if (plan_better(&cand, &b)) {