diff --git a/PLAN.md b/PLAN.md index 5c13dd8..a9a3feb 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1123,6 +1123,32 @@ has to be its own commit with its own re-recorded scorecard. agrees. Worth one commit, and it needs its own measurements first: the padding case (`y.3.b` past the end) is a *legal* creation on mongod, so the fix is not "refuse a non-document element". +- **`partialFilterExpression` was accepted and ignored**, and that made + `unique` mean the wrong thing. An index built over every document instead of + the filtered subset still answers reads correctly -- it holds a superset, + never a subset -- but a *unique* one then rejects inserts mongod accepts: + + ```js + createIndex({a: 1}, {unique: true, partialFilterExpression: {t: true}}) + insertMany([{a: 1, t: false}, {a: 1, t: false}]) + // mongod: accepted, neither document is in the index + // ours: E11000 duplicate key error + ``` + + Unique-within-a-subset is the whole point of the option, so every use of it + was a legal insert refused. Refused at creation now, on the same judgement + `cmd_update` already makes about an update spec's `sort`. Nothing in the + repository covered this row: the pinned suite is crud and aggregate, and + neither `e2e5.js` nor `e2e6.js` writes a partial or hashed spec. Hashed, by + contrast, was honestly missing -- refused, with the wrong code (2 where + mongod says 67) but the right answer. + + `docs/M3_INDEX_TYPES_DESIGN_REVIEW.md` measures both features' rules and + argues the planner rule that decides the design: a partial index may only + answer a query whose predicates *imply* its filter, so until that test + exists the safe rule is to maintain the index and never read from it. Too + few documents is the one failure worse than no index at all. + - **M3's second corpus is `tests/spec/operators/`.** The eight operators PLAN §3 names all answered `bad update` with code 2, one message for every question — and `$push`'s `$slice`, `$position` and `$sort` were parsed, diff --git a/docs/M3_INDEX_TYPES_DESIGN_REVIEW.md b/docs/M3_INDEX_TYPES_DESIGN_REVIEW.md new file mode 100644 index 0000000..6036680 --- /dev/null +++ b/docs/M3_INDEX_TYPES_DESIGN_REVIEW.md @@ -0,0 +1,159 @@ +# M3 design review — partial and hashed indexes + +The last line of M3's row. Written before any code, like the M2 and +`arrayFilters` reviews, and for the same reason: both times the measurement +disagreed with the plan before the implementation did. + +Everything below was measured on 2026-08-10 against mongod 8.3.7 on `:27099` +and this server at `98fde82` on `:27020`, running the identical probe against +both. + +--- + +## 1. The two halves are not the same kind of gap + +PLAN §3 names them together — "partial + hashed indexes" — as though they were +one item. They are not. + +**Hashed is honestly missing.** `createIndex({a: "hashed"})` is refused: + +``` +mongod: a_hashed +ours: ERR [2] invalid index spec +``` + +Wrong code (mongod says 67 for an unknown key direction, and has four more +specific ones besides), but the right answer. A client is told no and knows +where it stands. Queries against the collection keep working, because there is +simply no index. + +**Partial is accepted and ignored.** + +``` +mongod: listIndexes -> {"key":{"a":1},"name":"a_1","partialFilterExpression":{"a":{"$gte":5}}} +ours: listIndexes -> {"key":{"a":1},"name":"a_1"} +``` + +`createIndex` answers `a_1` and reports success. The index is built over +*every* document rather than the ones the filter selects, `listIndexes` does +not mention the option, and nothing tells the client that the index they asked +for is not the index they have. + +## 2. Ignoring it is not harmless, and here is the row that proves it + +An over-inclusive index still answers queries correctly — it contains a +superset of what it should, never a subset, so a scan through it finds +everything. Reads are safe. That is worth saying plainly, because it is the +reason this is a smaller fire than `arrayFilters` was. + +It is not, however, harmless, and `unique` is where it stops being harmless: + +```js +// createIndex({a: 1}, {unique: true, partialFilterExpression: {t: true}}) +// insertMany([{a: 1, t: false}, {a: 1, t: false}]) + +mongod: accepted -- neither document is in the index, so neither collides +ours: ERR [11000] E11000 duplicate key error ... dup key: {a: 1} +``` + +**A legal insert is refused.** The whole point of a partial unique index is +uniqueness *within a subset*: unique email among active accounts, unique +external id among synced rows. Every one of those is an insert this server +rejects and mongod accepts. The client sees a duplicate-key error naming a +constraint it deliberately scoped away. + +Two smaller divergences ride along: `listIndexes` reports an index +specification that is not the one on disk, so a client comparing specs before +creating decides wrongly; and the index carries every document, which is the +storage the option existed to avoid. + +## 3. What the gate can see + +Nothing. There is no index-management corpus here — the pinned suite this +repo fetches is crud + aggregate, and `tests/e2e/e2e5.js` and `e2e6.js` test +indexes but neither writes a partial or hashed spec. The `unique`-plus-partial +row above is not covered by a single test in the repository, in any suite. + +So M3's last line needs its own recorded corpus, exactly like the positional +operators and the update operators did. `tests/spec/indexes/`, same shape: +inputs authored in `sources/`, expectations measured from mongod, run through +`run.js --suite-dir`. + +## 4. What the fix has to be, measured + +### Partial + +The filter is not an arbitrary query. mongod restricts it, and the restriction +is what makes the planner's job possible: + +| | mongod | +|---|---| +| equality, `$gt`/`$gte`/`$lt`/`$lte`, `$exists: true`, `$type` | allowed | +| `$and`, and `$or` from 8.0 | allowed | +| `$regex` | `Error in specification ... ` (67) | +| `partialFilterExpression` that is not a document | TypeMismatch (14) | +| `partialFilterExpression` **with `sparse`** | (67) — the two may not be combined | + +The planner rule is the load-bearing one and it is not symmetric: a partial +index may only answer a query whose predicates *imply* the filter. Using it +for a query it does not cover returns too few documents, which is the one +failure mode worse than not using an index at all. Until that implication test +exists, the safe planner rule is **never use a partial index for a read** — +maintain it, enforce `unique` through it, and let reads fall back to a scan. +That is a correct server that has not yet earned the speedup, and it is where +this should land first. + +### Hashed + +| | mongod | +|---|---| +| `{a: "hashed"}` | allowed, name `a_hashed` | +| `{a: "hashed", b: 1}` | allowed — one hashed component beside range ones | +| `{a: "hashed", b: "hashed"}` | 31303, "A maximum of one index field is allowed to be hashed" | +| `unique` on a hashed index | 16764, "Currently hashed indexes cannot guarantee uniqueness" | +| an array value at the hashed path | 16766, **at insert time**, not at creation | +| `{a: "bogus"}` | 67, "Unknown index plugin" | + +A hashed index answers equality only; mongod still returns correct results for +a range query or a sort over a hashed field by not using the index. So the +planner rule is the mirror of the partial one and just as conservative: +equality predicates only, everything else scans. + +The hash itself need not match mongod's. Nothing a client can observe depends +on the value — `listIndexes` reports `"hashed"`, not a hash — so this is a +private encoding choice, and the ordering of the tree is then meaningless, +which is exactly why sorts may not use it. + +## 5. The order this should land in + +The same shape the `arrayFilters` work took, and for the same reason: the +refusal is small, correct on its own, and stops the wrong answer before the +corpus that measures it exists. + +1. **Refuse `partialFilterExpression`.** This repo already has the precedent + and states it in `cmd_update`: an update spec's `sort` is refused because + "ignoring the field would be the worst of the three possible answers — the + client asked for a specific document and would silently get a different + one." Identical logic. The two alternatives are to keep enforcing `unique` + over the wrong set, or to report the option in `listIndexes` while not + honouring it, which is a larger lie than the current one. +2. **Record `tests/spec/indexes/`** against mongod: partial creation rules, + the `unique` subset semantics, hashed creation rules, and the read paths + for both. Red by construction. +3. **Partial indexes**, maintained and `unique`-enforcing, planner declining + to read from them. +4. **Hashed indexes**, equality-only in the planner. +5. **The implication test**, which is what lets a partial index serve a read, + and is a planner change rather than an index one. + +Steps 3 and 4 both need a catalog field. `write_index_catalog` has a `flags` +byte with four bits used, so a fifth can mean "a partial filter follows" and a +sixth "this key is hashed" — old files never set them and read back +identically, so `catalog_version` stays 1. That is the same argument the free +list used for its own format change and it holds here for the same reason. + +## 6. Not covered + +Neither `$or` in a partial filter beyond accepting it, nor `2dsphere`, `text`, +`wildcard` or `collation` — none of them is in M3's row, and each is a +milestone-sized item that would arrive with its own review. diff --git a/src/commands.zig b/src/commands.zig index da6f72b..a0e304b 100644 --- a/src/commands.zig +++ b/src/commands.zig @@ -934,6 +934,35 @@ 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) { @@ -5655,6 +5684,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. + .{ .code = 67, .spec = .{ .doc = &.{ + .{ .key = "key", .value = .{ .doc = &.{.{ .key = "a", .value = .{ .int32 = 1 } }} } }, + .{ .key = "partialFilterExpression", .value = .{ .doc = &.{ + .{ .key = "t", .value = .{ .bool = true } }, + } } }, + } } }, + // And with `unique`, which is the combination that made it a wrong + // answer rather than only a missing one. + .{ .code = 67, .spec = .{ .doc = &.{ + .{ .key = "key", .value = .{ .doc = &.{.{ .key = "b", .value = .{ .int32 = 1 } }} } }, + .{ .key = "unique", .value = .{ .bool = true } }, + .{ .key = "partialFilterExpression", .value = .{ .doc = &.{ + .{ .key = "t", .value = .{ .bool = true } }, + } } }, + } } }, }; for (bad) |case| { var ctx = tdb.ctx(io); @@ -5668,6 +5714,10 @@ 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); }