7de3e6b666ad099fd4a65409b543b341832c7b12
155 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
A.Shakhmatov
|
7de3e6b666 |
index: a partial index may answer a query that implies its filter
Step 5, the last of `docs/M3_INDEX_TYPES_DESIGN_REVIEW.md`'s order and the
last item in M3's row. A partial index was maintained and enforced
`unique`, and every read scanned -- correct, but the speedup the option
exists for was never earned.
The test is one-sided by construction: a `false` costs a scan, a `true`
has to be right, because returning too few documents is the one failure
worse than having no index at all. Two routes, and a filter conjunct is
implied if either answers yes:
**Route one, the query pins a value.** Run the *real matcher* against a
stand-in document holding that value at the path, rather than
reimplementing eight operators against a comparison that would then have
two definitions. Sound because every operator `check_partial_filter`
admits is existential -- "some value at this path satisfies it" -- so a
document with more values at the path satisfies it at least as easily,
and every document the query matches has the pinned value among its
values there. Covers `$eq`, `$in`, `$type`, `$exists` and the bounds in
one stroke.
Two shapes break that argument and are refused rather than approximated,
and both have a row in the test table:
- an **array** value. `{a: [1, 2]}` matches `{a: [[1, 2], 3]}`, whose
values at `a` do not include 1 or 2 -- only one level is expanded, so
the real document's value set is not a superset of the stand-in's.
- a **null** value. `{a: null}` also matches a document with no `a`,
which has no values at the path rather than more of them. The
stand-in alone would report `{a: {$exists: true}}` as implied, so an
empty document is tested too and both have to agree. This is the rule
the previous commit's null fix made necessary and possible in the
same breath.
**Route two, bounds.** The only route needing neither side to name a
document: `{a: {$gt: 5}}` implies `{a: {$gt: 0}}`. Inclusivity is where
it is decided -- `$gte: 0` admits the endpoint that `$gt: 0` excludes.
Soundness is judged against *this server's* matcher, not mongod's. Both
halves of the question run the same code: `query.matches_bytes` decides
the index's contents in `build_entries` and re-filters every candidate
the plan yields. Where this server's comparison differs from mongod's
(PLAN §6: the comparison operators are not type-bracketed) both halves
are wrong together, which is a matching bug and not a lost document.
`$or` on the filter's side is implied by one implied branch: sufficient,
not necessary, since a query can imply a disjunction without implying a
disjunct.
**What each gate can and cannot see, measured with two mutations.** With
`query_implies_filter` forced to `true`, `partial.json` goes 23/7 and
every failure reads "expected N, got N-1" -- the exact shape of the bug.
With it forced to `false` -- the behaviour this commit replaces -- the
corpus is 30/30, because no client can observe *that* an index was used,
only that an answer went missing. So the corpus guards soundness and the
unit test on `plan()` is the only thing that sees the feature work at
all; both are needed and the commit says which does which.
Six corpus cases added, each pairing a query that implies the filter with
one that does not and touches the same field: a query leaving the
filter's field out, an `$in` straddling the filter, a query for null
against an `$exists` filter, equalities inside and outside a range
filter, a sort a partial index could serve, and a unique partial index
read. `partial.json` 24 -> 30 cases, `tests/spec/indexes/` 42 -> 48.
Verified: 257/257 unit tests in ReleaseFast and ReleaseSafe, 88/88 fuzz,
all four corpora 0 fail, pinned scorecard unchanged at 228/63/196, the
full e2e matrix and crash-fuzz green.
|
||
|
A.Shakhmatov
|
bf685aa6de |
plan/spec: hashed indexes are done, the implication test is next
`tests/spec/indexes/` is 42/42, so all four recorded corpora are green:
positional 51, operators 125, indexes 42, aggregate 70.
Records what implementing the row taught, including the three things the
design review got wrong. Two were already noted when the corpus was
recorded ($in is allowed; a differing filter is 86); the third is new and
cheaper than the review's version: hashed needs no flags bit, because it
belongs to a key *component* and the per-component direction byte was
already there holding 0 or 1.
Also fixes a corpus case that did not measure what it said. "equality
across numeric types" sent an int32, because a source is plain JSON and
`5.0` is `5` after JSON.parse -- it was a second copy of the case above
it. Reading sources as EJSON was tried and reverted, and the recorder now
says why: EJSON's wrapper namespace collides with the query operators
these sources are made of, so `{"$regex": "x"}` became a BSONRegExp,
`structuredClone` flattened it to `{pattern, options}`, and "a filter
using $regex is refused" silently became a filter mongod accepts. The
cross-type property is a unit test instead, which is where it belongs --
it is about how this server hashes, and mongod's hash is a different
function, so a corpus could only ever check the answer.
The case is renamed to what it does measure rather than deleted: two
documents sharing a value is still the read a hashed index exists for.
Verified: 256/256 unit tests in ReleaseFast and ReleaseSafe, 87/87 fuzz,
all four corpora 0 fail, pinned scorecard unchanged at 228/63/196, the
full e2e matrix and crash-fuzz green.
|
||
|
A.Shakhmatov
|
235ae19e30 |
query: a missing field is null to equality, and to nothing else
`find({a: null})` has to match a document with no `a` at all, as well as
one holding an explicit null. This server matched only the explicit one --
with or without an index -- so `{a: null}` returned one row where mongod
returns two, and `{"a.b": null}` returned none where mongod returns three.
Found by tests/spec/indexes/hashed.json, which is the last of the four
recorded corpora and the first test in this repository to ask the
question. The pinned crud+aggregate suite does not: the scorecard is
unchanged at 228/63/196 across this commit.
Two layers already believed this and only the matcher did not.
`index.build_entries` stores a missing field as null under a non-sparse
index, and `evaluate_index`'s sparse guard exists specifically to stop
this query reading an index that skipped those documents -- a guard that
was defending a behaviour that did not exist. So the fix makes three
layers agree rather than introducing a rule.
The substitution is deliberately narrow, and applies to `$eq`/`$in` and
their negations `$ne`/`$nin` only. A missing field is *not* null to
anything else: `{a: {$lt: 5}}` does not match it even though null sorts
below 5, `{a: {$exists: false}}` still has to see that there is nothing
there, and `{a: {$type: "null"}}` stays false. Collecting a null candidate
instead of substituting one would have flipped all three.
Measured against mongod 8.3.7 over a document set covering missing,
explicit null, a value, an empty array and a subdocument, with and without
an index on the path. The equality family now agrees in all four
combinations. Five neighbouring answers still differ and are none of them
touched by this commit -- four trace to one root cause, comparison
operators not being type-bracketed, and one to `{a: []}` traversed by a
dotted path. Both are recorded in PLAN §6.
|
||
|
A.Shakhmatov
|
21494469a5 |
db/index: a hashed key holds a hash of its value
M3's last row, step 4 of the index review's order. `{a: "hashed"}` was
refused with "invalid index spec" -- the right answer with the wrong code,
and the half of the row the review called honestly missing.
A hashed component is stored as a tag byte plus a 64-bit hash of the
value's *ordinary* encoded bytes. Hashing the encoding rather than the
value is what makes `{a: 5}` and `{a: 5.0}` land on the same entry for
free: `bson.encode_key` already normalizes every numeric type through
f128, because two values that compare `.eq` have to encode identically for
the tree to be a memcmp. One `encode_component` does it for entry
generation and both lookup paths, so the two sides cannot disagree -- a
component hashed on the way in and not on the way out would simply never
find anything.
Collisions are harmless, because this file's governing invariant is that
an index only generates candidates and the full filter is re-applied to
every one. The single place that would not survive one is uniqueness,
which is why `unique` is refused (16764) rather than approximated.
The planner is the mirror of the partial rule and just as conservative:
equality only. A range or a sort over a hashed component would read a band
of leaves ordered by hash, which is an arbitrary set of values, so both
are declined and the query scans. That is what leaves `find({a: {$gte:
5}})` and `find({}, {sort: {a: 1}})` correct.
The catalog needs no new field. `write_index_catalog` has always written
one byte per component and that byte has only ever held 0 or 1, so a third
value costs no format change and `catalog_version` stays 1. That is a
departure from the review, which guessed at a sixth flags bit: hashed
belongs to a *component*, and a compound index may hold one beside range
ones.
Measured on mongod 8.3.7 rather than recalled, and three of the five
answers were not what the corpus source assumed:
two hashed components 31303, codeName Location31303
unique on a hashed index 16764, codeName Location16764
an unknown plugin string 67, codeName CannotCreateIndex
an array at the path 16766 -- a *writeError* beside `ok: 1` on an
insert or update, and a command error from
createIndexes over data that already holds one
an array through a path refused for a *one-element* array too, which
is why `array_on_path` walks the path instead
of counting the values at it
tests/spec/indexes/hashed.json goes 0/18 -> 17/18. The one that remains
is not about hashed indexes: `find({a: null})` has to match a document
with no `a`, and this server matches only an explicit null -- with or
without an index. Next commit.
|
||
|
A.Shakhmatov
|
55009a429d |
plan/spec: partial indexes are done, hashed is next
partial.json 3/24 -> 24/24, hashed.json still 0/18. PLAN §6 records the planner rule that is deliberately left conservative -- a partial index is maintained and enforces `unique`, and reads scan until the implication test exists. Full matrix at this commit: 250/250 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, operators 125/0, positional 51/0, aggregation 70/0, pinned crud 228/63/196, e2e and crash-fuzz green. |
||
|
A.Shakhmatov
|
f151f13bfc |
db/index: partial indexes hold only what their filter selects
`partialFilterExpression` is honoured rather than refused. The filter is
consulted in exactly one place -- `build_entries` -- which is what keeps the
insert and the remove path from ever disagreeing about which documents the
index holds. Filtering at each call site instead is how an index ends up with
entries pointing at documents that are gone.
That single choke point is also why `unique` comes out right for free, and
`unique` is the whole reason this was a wrong answer rather than a missing
feature: two documents sharing a value *outside* the filter are now accepted,
where before they were E11000. Unique-within-a-subset is what the option is
for.
The filter is part of the index, so it is persisted with it: a fifth bit in
`write_index_catalog`'s flags byte and the serialized document after the TTL.
A file written before this never sets the bit and reads back exactly as it
did, so `catalog_version` stays 1 -- the same argument the free list used.
**The planner declines to read from a partial index.** It holds a subset, so
answering a query from it is only correct when the query's predicates imply
its filter, and that implication test does not exist yet. Returning too few
documents is the one failure worse than having no index at all. So it is
maintained, it enforces `unique`, and reads scan. PLAN §6.
Which predicates a filter may hold is measured: `$eq`, `$gt`, `$gte`, `$lt`,
`$lte`, `$in`, `$exists`, `$type`, `$and`, `$or`. `$ne` and `$regex` are
refused -- including a regex sent as a BSON *value*, which is how a driver
spells `{$regex: "x"}` and which the corpus caught. `sparse` and
`partialFilterExpression` may not be combined: 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.
Same name, different filter is IndexKeySpecsConflict (86) where a differing
*option* is IndexOptionsConflict (85) next door -- measured, and the split is
that a filter decides which documents the index is over rather than how it
behaves.
ReleaseSafe earned its keep: an assertion held that a non-sparse index covers
every document after an open, and a partial one is a third shape that does
not. Extended rather than relaxed -- multikey was already the second.
partial.json 3/24 -> **24/24**. 250/250 unit tests in ReleaseFast and
ReleaseSafe, 83/83 fuzz, everything else unmoved.
|
||
|
A.Shakhmatov
|
dc26c66f40 |
tests/spec: name is an option of createIndex, not a positional
`POSITIONAL` holds `name` because `dropIndex` takes it as one. `createIndex` does not -- it is an option there, and `options()` was stripping it, so a corpus case asking for a named index silently got a derived one and then disagreed with an expectation recorded from a driver that had been passed the name. Two cases in `tests/spec/indexes/partial.json` failed on exactly that. Put back after the strip rather than removed from the set, because the set is right for every other operation that reads `args.name`. |
||
|
A.Shakhmatov
|
1c098dc48c |
plan: the index corpus is M3's last gate
42 cases at 3/39, and the two rows where recording it corrected the review: `$in` in a partial filter is allowed, and a same-key different-filter clash with no explicit name is IndexKeySpecsConflict (86) rather than 67. Everything else at this commit is unmoved: 249/249 unit tests, operators 125/0, positional 51/0, aggregation 70/0, pinned crud 228/63/196. |
||
|
A.Shakhmatov
|
879a6bb07b |
tests/spec: a recorded corpus for partial and hashed indexes
M3's last row, and the first of the four corpora here whose subject nothing in
the repository tested at all: the pinned suite is crud and aggregate, and
`e2e5.js`/`e2e6.js` write neither a partial nor a hashed spec.
42 cases in two files, recorded red at 3/39. The three that pass are the reads
a partial index does not change -- this server indexes every document, so a
query still finds everything, which is exactly why the review called this a
smaller fire than `arrayFilters`.
A case here is a *sequence* rather than one operation: create an index, insert
against it, read back, list it. So the recorder walks a case's operations in
order and stops at the first that throws, which is what a client would see,
and drops the collection between cases because an index outlives a
`deleteMany`.
Two of the design review's own guesses were wrong, which is the argument for
recording rather than reasoning:
- **`$in` in a partial filter is allowed.** The review grouped it with `$ne`
and `$regex`, which are 67.
- **Same key, different filter, no explicit name is IndexKeySpecsConflict
(86)**, not the 67 the review assumed.
What it confirmed, and what the implementation now has to satisfy: a unique
partial index constrains only the documents its filter selects; a document
*leaving* the filter frees the value it held; a document *entering* it must
take a value nothing inside holds, or the update is E11000. `sparse` and
`partialFilterExpression` may not be combined (67). `expireAfterSeconds` and a
filter may, and `listIndexes` reports the filter before the expiry. Two hashed
components is 31303, `unique` on a hashed index is 16764, and an array at a
hashed path is 16766 *at insert time* rather than at creation.
|
||
|
A.Shakhmatov
|
21002528be |
commands: refuse partialFilterExpression instead of ignoring it
`createIndex({a: 1}, {partialFilterExpression: ...})` answered success, built
the index over every document, and left the option out of `listIndexes`.
An over-inclusive index still answers reads correctly -- it holds a superset,
never a subset -- so this was not the array-destroying class of bug. `unique`
is where it stopped being harmless. Measured on mongod 8.3.7:
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: E11000 duplicate key error, dup key: {a: 1}
A legal insert refused. Unique-within-a-subset -- unique email among active
accounts, unique external id among synced rows -- is the whole point of the
option, so every use of it hit this.
Refused at creation, which is the same judgement `cmd_update` already makes
about an update spec's `sort`: "ignoring the field would be the worst of the
three possible answers." The two alternatives here were to keep enforcing
`unique` over the wrong set, or to echo the option back from `listIndexes`
while not honouring it, which is a larger lie than saying no.
CannotCreateIndex (67), joining the five existing rows of the same test with
the mutation that reddens them written beside it. The implementation is the
rest of M3's last row and comes next, against a corpus that does not exist
yet -- nothing in this repository covered the row above, in any suite.
249/249 unit tests, pinned crud corpus unmoved at 228/63/196.
|
||
|
A.Shakhmatov
|
ce4ff1fd63 |
docs: M3 design review -- partial and hashed indexes
M3's last row names them together. They are not the same kind of gap, and the
measurement is what says so.
**Hashed is honestly missing**: `createIndex({a: "hashed"})` is refused. Wrong
code -- 2 where mongod says 67, and it has four more specific ones besides --
but the right answer, and queries keep working because there is simply no
index.
**Partial is accepted and ignored.** `createIndex` reports success,
`listIndexes` does not mention `partialFilterExpression`, and the index is
built over every document rather than the ones the filter selects.
An over-inclusive index still answers reads correctly, which is worth saying
plainly: it holds a superset, never a subset. `unique` is where that stops
being true --
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
-- a legal insert refused. Unique-within-a-subset is the whole point of the
option, and every use of it is an insert this server rejects.
Nothing in the repository covers that row: the pinned suite is crud and
aggregate, and neither `e2e5.js` nor `e2e6.js` writes a partial or hashed
spec. So this needs its own recorded corpus, like the positional operators and
the update operators before it.
The review measures both feature's rules -- which predicates a partial filter
may hold, why `sparse` and `partialFilterExpression` may not be combined, the
four hashed refusals and the one that fires at insert time rather than at
creation -- and argues the planner rule that matters: 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.
Recommended order: refuse `partialFilterExpression`, record the corpus, then
implement partial, then hashed, then the implication test. Both need one
catalog field each, and `write_index_catalog`'s flags byte has spare bits, so
`catalog_version` stays 1 -- the same argument the free list used.
|
||
|
A.Shakhmatov
|
98fde82946 |
plan/spec: pipeline-style updates are done
M3's row and §6 updated: the operator corpus is 125 cases across seven files, all green, and the pipeline work also gave `aggregate` the `$replaceWith` stage it never had -- the compiler is shared between the two. Full matrix at this commit: 249/249 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, operators 125/0, positional 51/0, aggregation 70/0, pinned crud 228/63/196, e2e and crash-fuzz green. |
||
|
A.Shakhmatov
|
003c418a0e |
commands: an update may be written as a pipeline
`u` (and `findAndModify`'s `update`) may be an array of aggregation stages
instead of a document of operators. The stages themselves already existed --
M2 built `compile_rewrite` and `apply_rewrite` for `aggregate` -- so this is
mostly about which of them are allowed here and what happens around them.
Six are: `$addFields`/`$set`, `$project`, `$unset`, `$replaceRoot`/
`$replaceWith`. Each rewrites one document into one document, which is the
property that makes them usable: `$match` could drop it, `$group` and
`$unwind` could change how many there are, and `$sort` means nothing to one.
mongod refuses those four by name with InvalidOptions (72) and a name that is
no stage at all with 40324, and the difference is worth keeping -- "not here"
and "not at all" are different things to be told.
`$replaceWith` did not exist here at all and now does, in `compile_rewrite`,
so `aggregate` gets it too: it is `$replaceRoot` with the expression in place
of the `{newRoot: ...}` wrapper, one stage under two spellings.
**The `_id` survives every stage.** `$replaceRoot: {newRoot: "$t"}` drops it
and `$project: {_id: 0}` asks to, and it comes back either way, because a
pipeline update rewrites a document rather than replacing one document with
another. A stage that sets it to a *different* value is ImmutableField (66);
restating the same one is fine. Measured on all six stages, and the mutation
that reddens the test is deleting the restore -- the document becomes
unfindable by the id it is stored under.
`EvalCtx.coll` becomes optional, which is what lets the upsert path run a
pipeline over a document held in memory before the collection it will be
inserted into exists. Only an `.offsets` stream reads the slab; a `.docs` one
never touched the field.
`arrayFilters` beside a pipeline is FailedToParse (9), not ignored: an
identifier a pipeline cannot spell would make the update silently different
from the one the client wrote.
pipeline.json 0/23 -> 23/23, operator corpus 125/125.
Pinned crud scorecard **218 -> 228 pass, 73 -> 63 fail**: the five pipeline
files, the four `-rawdata` files that use pipelines, and
`findOneAndUpdate-comment`, which is two more pipeline cases.
249/249 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz.
|
||
|
A.Shakhmatov
|
e344a073a1 |
tests/spec: record the pipeline-style updates
An update document may be an *array* of aggregation stages instead of a
document of operators. The pinned crud corpus has five cases, one per command,
and not one of them is a refusal -- so which stages are allowed, what happens
to `_id`, and what an upsert does are all unmeasured there.
23 cases into the existing operator corpus rather than an eighth directory:
this is another shape of update document, and it shares the recorder, the
masking and the README.
Recorded red, 0/23. What it settled:
- **`_id` always survives**, through `$replaceRoot: {newRoot: "$t"}` and
through `$project: {_id: 0}` alike. A stage that sets it to a *different*
value is ImmutableField (66); restating the same one is fine.
- `$match`, `$group`, `$sort` and `$unwind` are real stages refused
specifically here: InvalidOptions (72), "$X is not allowed to be used
within an update". A name that is no stage at all is 40324, and two
stages packed into one array element is 40323.
- `arrayFilters` beside a pipeline is FailedToParse (9).
- an upsert runs the pipeline over the document the filter implies.
Three shapes were authored and removed: `{b: 1, $set: {...}}`, an empty
pipeline, and a pipeline holding a non-document. The driver rejects all three
before they reach a server, so there is nothing to record.
|
||
|
A.Shakhmatov
|
44788ae681 |
plan/spec: the operator corpus goes green
102/102. PLAN's M3 row now names both corpora as the gate, because neither the pinned crud suite nor e2e3/e2e4 can see this work: the eight operators are barely in the pinned corpus and `$push`'s modifiers are not in it at all. §6 records what the corpus found that was nobody's operator -- an upsert never reporting the `_id` it generated -- and the three things left open on purpose: `$bit`, a multi-field `$sort` key inside `$push`, and the 64-path bound on the conflict check. Full matrix at this commit: 247/247 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, operators 102/0, positional 51/0, aggregation 70/0, pinned crud 218/73/196 unmoved, e2e and crash-fuzz green. |
||
|
A.Shakhmatov
|
71e3879ae0 |
update: an unknown modifier, and two paths that decide one field
Two refusals this server did not have, both measured.
**ConflictingUpdateOperators (40).** Two paths in one update where either is a
prefix of the other at a segment boundary leave the result depending on which
operator ran first, so mongod refuses rather than picking an order:
`{$set: {a: 2}, $inc: {a: 1}}`, `{$min: {a: 2}, $max: {a: 9}}`,
`{$set: {a: 2, "a.b": 3}}`, and `$rename` counts both its ends. Siblings are
fine, and the segment boundary is what makes them fine -- `a.b` and `a.bb`
share a string prefix and decide nothing about each other, which is the
mutation the test names.
The check is quadratic in the number of paths and bounded at 64, so it stays
on the stack. Past the bound it stops checking rather than refusing: missing
a conflict in a 65-field update is a better answer than refusing a legal one,
and no driver writes updates that wide.
**Unknown modifier (9).** A name not in the operator table was
`InvalidUpdate` -> BadValue; mongod answers FailedToParse and names it. What
matters is the option nobody took: skipping an unknown operator would make a
typo an update the client believes ran. A known operator handed something
that is not a document of fields is the same code.
One existing test changed answer and was wrong: it packed
`$set: {new: 5}` and `$rename: {new: "renamed"}` into one update and passed
only because there was no conflict check. mongod refuses that with 40 --
measured, then the test split into two updates.
operators corpus 95/102 -> **102/102**. The pinned crud corpus, the positional
corpus and the aggregation corpus are unmoved at 218/73, 51/0 and 70/0.
247/247 unit tests.
|
||
|
A.Shakhmatov
|
c9575eec0a |
commands: an upsert reports the _id it generated
`Engine.insert` generates an `_id` into the bytes it writes and leaves the caller's tree without one, so every handler that looks afterwards found nothing: `update`'s `upserted` array carried `_id: null`, which the driver surfaces as `upsertedId: null`, and `findAndModify` with `returnDocument: after` returned an upserted document with no `_id` at all. The client has never seen this document. The `_id` in the reply is the only way it can name it again, so both answers were wrong in the way that matters. Settled in `build_upsert_doc` rather than in the storage engine: the upsert path is what decides the identity of the document it builds, and it has to be after the operators run because `$setOnInsert` may supply the `_id` itself. At the front of the document, where MongoDB stores it and where the `_id_` index descends on it. Found by `tests/spec/operators/`, which is the first corpus here to upsert into an empty collection and then look at what came back. The pinned crud corpus does not move -- its upsert cases match `upsertedId` loosely. set-on-insert.json 7/9 -> 8/9. 245/245 unit tests. |
||
|
A.Shakhmatov
|
1b6753fcec |
update: $setOnInsert and $currentDate
`$setOnInsert` is `$set` on the branch that inserts and nothing at all on the
branch that updates, so `Options` grows the one bit that says which -- and
`build_upsert_doc` is the only caller that sets it. It does not go through
`op_set` because of the measured exception: writing `_id` is allowed on a
document being built and refused on one being rewritten. A document nobody
has yet has no identity to change.
`$currentDate` reads a clock, and the clock is a parameter. There is no
fallback to a global one: this file has no `io` to reach the real clock
through, and the handlers pass the same `std.Io.Timestamp` that `ttl_sweep`
and the cursor sweep already read. A caller that forgets gets the epoch --
deterministic and obviously wrong -- rather than something that changes
between runs.
Measured: `$currentDate: {d: false}` writes a date. The boolean says "a date",
not "whether", and either value means the same thing. `{$type: "timestamp"}`
writes a BSON timestamp, seconds in the high 32 bits and an ordinal in the
low ones; mongod fills the ordinal from the oplog, a standalone has none, so
it is 1.
current-date.json 3/11 -> 11/11, set-on-insert.json 0/9 -> 7/9. The two left
are not `$setOnInsert`'s: one is ConflictingUpdateOperators, and the other is
a pre-existing bug the corpus found -- an upsert never reports the `_id` it
generated, because `Engine.insert` writes it into the bytes and not back into
the caller's tree. `updateOne(..., {upsert: true}).upsertedId` is null here
and an ObjectId on mongod. Next commit.
244/244 unit tests.
|
||
|
A.Shakhmatov
|
482ddb3d1a |
update: $push's modifiers are modifiers
`$slice`, `$position` and `$sort` were parsed, accepted and dropped.
`{$each: [3, 4], $slice: -3}` appended both values, sliced nothing and
answered ok: 1 with modifiedCount: 1 -- the same class of wrong answer the
positional operators were, and the reason `tests/spec/operators/` exists
rather than a list of TODOs.
Measured, and the order is the whole of it: insert at `$position`, then
`$sort` the array *including* the new elements, then `$slice` the result.
- `$slice: n` keeps the first n, `$slice: -n` the **last** n. That half is
what a capped log depends on and is the one easy to write backwards; the
test mutates exactly it.
- `$position` counts back from the end when negative, and clamps at the
front rather than wrapping.
- `$sort: 1` orders whole elements in BSON order; `$sort: {a: 1}` orders on
a field of them, and an element without it sorts as null -- the rank a
missing field has everywhere else here.
- **without `$each` there are no modifiers at all**: `{$push: {t: {$slice:
1}}}` pushes the document `{$slice: 1}` as a value. That is what makes
`$each` the flag rather than a member of the set, and it is measured, not
reasoned.
An unknown `$`-prefixed key beside `$each`, or a `$slice`/`$position` that is
not a number, is BadValue -- refused rather than ignored, which is the point.
push-modifiers.json 6/21 -> 21/21. 240/240 unit tests.
|
||
|
A.Shakhmatov
|
066b32617e |
update: $addToSet, $pop and $pullAll
`$addToSet`'s identity is `bson.compare` equality, which is already exactly
mongod's: an int32 `2` and a double `2.0` are one value, and `{a: 1, b: 2}`
and `{b: 2, a: 1}` are two, because `compare_docs` walks the pairs
positionally and tie-breaks on the key. Candidates are checked against the
array as it grows, so a `$each` holding the same value twice adds it once.
`$pullAll` is `$pull`'s neighbour and its opposite: `$pull` takes a predicate,
`$pullAll` takes values compared whole. `{$pull: {t: {a: 1}}}` removes
elements *having* `a: 1`; `{$pullAll: {t: [{a: 1}]}}` removes elements that
*are* `{a: 1}`. Sharing the comparison would have been the natural mistake and
is what the test mutates to check.
`$pop` is the small one, and its refusals are the measured part: an empty
array and an absent field are no-ops, an argument that is not 1 or -1 is
FailedToParse (9), and a non-array field is TypeMismatch (14) -- where the
identical mistake under `$addToSet` and `$pullAll` is BadValue (2). Three
codes for one shape, none of them derivable from the others. `$each` that is
not an array is 14 under `$addToSet` and 2 under `$push`, measured on both.
array-ops.json 2/26 -> 26/26. 234/234 unit tests.
|
||
|
A.Shakhmatov
|
c2f682717b |
update: $mul, $min and $max
Three operators, two shapes. `$mul` joins `$inc` in `op_arith` because they
differ only in the operation; `$min` and `$max` are not numeric operators at
all and get their own.
Measured, and each row is a rule that would have been guessed wrong:
- `{$mul: {gone: 5}}` writes **0**, not 5. An absent field starts from zero
under both operators, which is the identity for one and the annihilator
for the other, and mongod picks zero for both.
- `$min`/`$max` compare in BSON canonical order, so `{$min: {s: 5}}` on
`s: "b"` writes 5 -- a number ranks below a string -- and `{$max: {a: 1}}`
on `a: null` writes 1. An absent field is always written: there is nothing
to be smaller or larger than.
- an int32 product that does not fit widens to int64, the same ladder
`numeric_add` already climbed.
`$inc` changes answer with them: a non-numeric field or operand was
`InvalidUpdate` -> BadValue (2), and mongod answers TypeMismatch (14) with a
different sentence for each side. So two errors rather than one, and `$inc`
gets the codes it should always have had.
numeric.json 0/21 -> 20/21. The one left is `$min` and `$max` on the same
field, which is ConflictingUpdateOperators (40) -- a whole error class this
server does not have yet, and its own commit.
228/228 unit tests.
|
||
|
A.Shakhmatov
|
301a4538fd |
update: one function per update operator
`apply_operator` was a chain of six inline bodies and about a hundred lines, and M3 adds five more operators to it. One function each, and the shape every operator's argument has -- a document of path/operand pairs -- checked once at the top instead of six times. No behaviour change: 222/222, the same tests, unmoved. |
||
|
A.Shakhmatov
|
eaf8d7c677 |
tests/spec: a recorded corpus for the update operators
PLAN §3 lists eight operators for M3 -- `$setOnInsert`, `$addToSet`, `$mul`,
`$min`, `$max`, `$pop`, `$pullAll`, `$currentDate` -- and the pinned crud
corpus says almost nothing about any of them. A probe running the identical
update against mongod 8.3.7 and this server found all eight answering
`bad update`, code 2, one message for every question.
It also found the thing this directory exists for: `$push`'s `$slice`,
`$position` and `$sort` are **silently ignored**. `{$each: [3, 4], $slice: -3}`
appends both values, slices nothing, and answers ok: 1 with modifiedCount: 1.
A missing operator is an error the client can see; a modifier that is parsed,
accepted and then dropped is the same class of wrong answer the positional
operators were.
103 cases in six files, 18 pass / 84 fail -- red by construction, like
`tests/spec/aggregate/expressions.json` at 1/26 and the positional corpus at
15/36. Inputs authored in `sources/`, every expectation measured.
Two things here cannot be recorded as values, and both become a `$$type`
assertion rather than being left out: a `$currentDate` field is whatever the
clock said (named per case in `volatile`, so a real stored date can still be
pinned one day), and an upsert that inserts gets a generated ObjectId (that
one automatic -- no source authors an ObjectId). Everything else is compared
exactly. Files are canonical extended JSON, which the runner already parses
that way: `$mul` overflowing an int32 produces an int64, and writing
`4000000000` as a bare number would not have said so.
What recording it settled, none of it guessable:
- `$mul` of a missing field writes **0**, not the operand; of a non-numeric
field, or by one, TypeMismatch (14).
- `$min`/`$max` are not numeric operators. They compare in BSON canonical
order, so `$min: {s: 5}` on `s: "b"` writes 5, and a missing field is
always written.
- two operators writing one field is **ConflictingUpdateOperators (40)** --
`$min`+`$max`, `$set`+`$inc`, `$setOnInsert`+`$set`. A whole error class
this server does not have.
- `$addToSet` compares documents whole, **field order included**:
`{a:1,b:2}` and `{b:2,a:1}` are two values. But `2` and `2.0` are one.
- `$push` modifiers are only modifiers when `$each` is there: `{$slice: 1}`
alone is a value to push. With it, the order is position, then sort the
whole array, then slice.
- `$currentDate` with `false` still writes a date.
- `$setOnInsert` may write `_id` on an insert, where `$set` may not.
- an unknown modifier is FailedToParse (9), not BadValue.
One case was authored and then removed: `{b: 1, $set: {c: 1}}` never reaches a
server -- the driver rejects it -- so there was no answer to record and the
case would have asserted nothing.
|
||
|
A.Shakhmatov
|
1482df891b |
commands/tests: name the mutation that actually reddens the arrayFilters test
The comment claimed moving `update.validate` below `scan_matching` would redden it. Ran the mutation: it does not -- the call still sits above the zero-match branch, so it still runs. What reddens it is deleting the standalone call and leaving the check to `apply`, which runs once per matched document and so never at all when nothing matched. A mutation note that has not been run is worth less than no note. |
||
|
A.Shakhmatov
|
1892cb7969 |
plan/spec: the positional gate goes green
Scorecard 204 -> 218 pass, 87 -> 73 fail: the fourteen `arrayFilters` cases across five files, which is the whole of what the two implementation commits were expected to move and nothing else. Positional corpus 51/51. The three divergences measured on the way are written into PLAN §6 rather than left in commit messages: `$` with two predicates on one array that no element satisfies together, an array filter with a top-level `$and`/`$or`, and a literal index into a scalar element -- the last being the one place the positional walk and the plain indexed path now answer differently, which is worth a commit of its own and needs its own measurements first. The design review gets an outcome note, since two of its guesses were wrong and the corpus is where that was settled. |
||
|
A.Shakhmatov
|
200228b0cb |
commands: arrayFilters and the query reach the update
The engine landed last commit with nothing feeding it: `$[<identifier>]` had no filters to bind and `$` had no query to resolve against, so both refused correctly and uselessly. This is the plumbing. `arrayFilters` is read per update statement on `update` and once on `findAndModify`, which is where each command carries it. Only the *shape* is checked here -- an array, of documents, TypeMismatch (14) with mongod's own field names for either -- because which identifier a filter names, whether it is spelled legally and whether the update ever uses it all need the update's paths, and those belong to `update.validate`. `validate` is called before `scan_matching`, not inside the per-document loop, and that placement is load-bearing in both directions: an array filter the update never uses is refused (9) even when the query matches nothing at all, and an identifier nothing binds is refused (2) before a single document is read. Both measured; a test pins each, with the mutation that reddens it named in the comment. Positional corpus 23 -> 51 of 51, the gate green. Pinned crud scorecard 204 -> 218 pass, 87 -> 73 fail: the whole arrayFilters cluster, which until two commits ago was answering ok: 1 having replaced the array with a document keyed by the path segment's literal text. 222/222 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz. |
||
|
A.Shakhmatov
|
a05874d597 |
update: a positional path resolves to the elements it names
`$[]`, `$[<identifier>]` and `$` stop being refused and start being walked.
A path with a positional segment names a *set* of concrete paths rather than
one -- `y.$[].b` on a two-element array names `y.0.b` and `y.1.b` -- so
`resolve` expands it against the document at hand and every operator then
walks paths it already knew how to walk. Nothing below `resolve` knows a
positional segment exists, which is why `$set`, `$inc`, `$unset`, `$push` and
`$pull` all get it at once.
The static half -- which identifier binds to which array filter, whether a
segment may sit in first position, whether a filter went unused -- depends
only on the update and the filters, so it runs once in `validate` before any
document is touched. The document-dependent half is resolution itself: an
absent or non-array path is a refusal there, because array updates address
what is there rather than creating it, unlike `$set` on a plain path.
Codes and messages measured on mongod 8.3.7, not recalled. Sixteen shapes
were run; the ones that changed what this commit does:
- `$[]` in first position answers the *array filter identifier* message,
not the `$` one -- mongod treats the two spellings as one check.
- an array filter may have several top-level fields as long as they all
name the same identifier: `{i.b: 3, i.c: 1}` is legal, `{i.b: 3, j.b: 1}`
is not. So the check is on the name, not on the count.
- an identifier is `[a-z][a-zA-Z0-9]*`: `aB2` yes, `Ab`, `a_b`, `1x` no.
- `$rename` refuses a positional path on *either* end, with its own message
for each, so it keeps the plain split rather than resolving.
- `$unset` of an element leaves a null in its place rather than shortening
the array -- the same answer `$unset: {"y.0": ""}` already gave.
- a scalar element with more path below it is PathNotViable (28), not a
field to create. Without that check the walk would hand `y.1.b` to
`set_path` and it would replace the `7` at `y.1` with `{b: 9}` -- a
smaller copy of the destruction this whole walk replaced.
One divergence, measured and deliberate: `{"y.b": 3, "y.c": 2}` matches
`[{b: 3, c: 1}, {b: 1, c: 2}]` without either element satisfying both, and
`$` then picks element 1 on mongod and element 0 here. mongod's answer is an
artefact of which predicate last wrote its match position; guessing at it
would be worse than recording it. PLAN §6.
The command handlers still pass neither the query nor any array filters, so
over the wire this is `$[]` working and the other two refusing -- for the
right reason and with the right code, which they did not before. The plumbing
is the next commit.
221/221 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz.
Positional corpus 15 -> 23 of 51.
|
||
|
A.Shakhmatov
|
e5a84c0598 |
tests/spec: a recorded corpus for the positional operators
M3's gate as named -- "remaining crud coverage; e2e3/e2e4 green" -- cannot
see this work. e2e3 and e2e4 contain zero positional paths, and the pinned
crud corpus covers `$[<identifier>]` only: no `$[]` case, no bare `$` case
anywhere in it. Both stayed green through a bug that replaced an array with
`{"$[i]": {...}}` and answered ok: 1. So M3 brings its own corpus, built the
way M2.5's was: inputs authored in `sources/`, every expectation recorded
from mongod 8.3.7, run through the shared runner with `--suite-dir`.
51 cases across the three spellings. It stands at 15 pass / 36 fail against
the refusal, which is the intended shape -- `expressions.json` was recorded
at 1 pass / 26 fail before the evaluator and is green now. The 15 that pass
are refusals where this server's code already matches; of the 36, 31 are the
constructs answering "not implemented" and 5 are refusals whose code
differs, four of them arrayFilters validation this server cannot do because
it never parses the option.
Every case records its `outcome`, refusals included. That is deliberate and
it is the whole point of the file: a refusal that left the document mangled
is indistinguishable from a clean one in `expectError` alone, and a mangled
document is what this corpus exists to catch.
What recording it settled, none of it guessable, the first contradicting
what the design review assumed:
- `y.$[i].c.$[i].d`, one identifier reused at two levels, is **accepted**
-- not a duplicate-identifier error
- `$[]` over an empty array is a no-op with modifiedCount 0
- `$[]` over an array with a non-document element is error 28, where every
other path failure here is 2
- a positional segment never creates: a missing or non-array path is an
error, where `$set: {'a.b': 1}` would construct one
- an upsert gets no special case -- it fails for the same reason
- `$` writes only the first matching element, and is refused when the query
never touched the array
- any of the three in first position is refused, as is `$` twice in a path
- `arrayFilters` alongside a replacement is ignored rather than refused
A case needing its own documents reseeds through operations rather than
`initialData`, because the format's `initialData` is per file and the runner
seeds it once per test. That keeps one file per operator instead of one file
per document shape.
crud scorecard unchanged at 204/87, aggregation corpus still 70/0.
|
||
|
A.Shakhmatov
|
3c2ac38fd9 |
commands: drop does not unlock the collection it just freed
A use-after-free. Dispatch held `drop`'s collection lock across the handler,
`drop_collection` freed the Collection the lock lives in, and dispatch then
ran `unlock_collection` on freed memory -- an atomic read-modify-write
inside `Io.RwLock.unlock`. One insert and one drop was enough.
Two things kept it hidden for this long. `drop` had no unit test at all:
before this commit every `parse_fake_msg("drop", ...)` in the tree was in a
test written to hunt it. And over the wire it does not fault -- 25
insert/drop cycles against a live server pass -- because the general
allocator leaves the freed page mapped and the atomic write lands somewhere
harmless. That was luck, not safety: the same undefined behaviour either
way, and testing.allocator is what makes it visible, which is why the
regression test is a unit test rather than an e2e script.
Fixed by giving `drop` no collection lock at all. The catalog lock is what
actually excludes here: every collection lock in this engine -- dispatch,
the TTL sweep, `compact`'s rebuild, `write_catalog`, `slab_stats`,
reclamation -- is taken while holding the catalog at least shared, so
holding it exclusively already keeps every one of them out. The collection
lock was buying exclusion that was already there and paying for it by
locking an object about to cease existing.
That exposed a second bug rather than creating one. The dispatch epilogue --
commit, then maybe checkpoint, then maybe compact -- fired on
`locks.coll == .exclusive` as a stand-in for "this was a write". It is now
keyed on `kind == .write`, because the two agreed only by accident:
`dropDatabase` is the one write that never held a collection lock, so it has
never reached that epilogue, and a dropped database waited for some later
write to trigger the checkpoint that records it.
Mutation-checked: restoring `.coll = .exclusive` on the drop row reproduces
the original SIGSEGV in the new test.
208/208 unit (2 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, crud
scorecard unchanged at 204/87, aggregation corpus 70/0, full e2e matrix,
crash-fuzz and the 25-cycle wire drop probe green.
Left open and recorded in PLAN §6: `drop_collection` still writes no log
record, so a dropped collection resurrects on reopen unless a checkpoint ran
(pre-existing, with its own test at db.zig:5542); and `apply_pending_write`
drops under `engine.rwlock` rather than the catalog lock, so the two drop
paths disagree about which lock protects a namespace.
|
||
|
A.Shakhmatov
|
8136ffe8d4 |
plan: drop segfaults when dispatched in-process
Found while writing the positional-refusal tests and not caused by them --
it reproduces at
|
||
|
A.Shakhmatov
|
f04e7125c9 |
update: refuse a positional path instead of destroying the array
`{$set: {"y.$[i].b": 2}}` did not fail to update the array. It replaced
`y: [{b: 3}, {b: 1}]` with `y: {"$[i]": {"b": 2}}` -- every element
discarded -- and answered ok: 1, modifiedCount: 1. Remotely reachable by any
client issuing an ordinary MongoDB update.
`arrayFilters` was not implicated: the string appears nowhere in src/, the
option is accepted off the wire and dropped. The destruction was in the
path, at `set_path`'s "treat as non-array: replace with a doc" branch, so it
fired for all three spellings of "descend into this array" -- `$`, `$[]` and
`$[<ident>]` -- under every operator. `$inc` through `$[i]` stored its
operand rather than incrementing.
Two refusals, because the branch held two different mistakes:
- a positional segment is refused up front, before anything is applied, so
an update naming a good path and a positional one lands neither. Its
code is BadValue (2), which is what mongod answers for every positional
path failure.
- a plain non-numeric segment under an array -- `y.nope.b`, `y.$x.b` -- is
PathNotViable (28), measured. It is never a field to create, which is
the opposite of what `set_path` does for a missing *document* field and
the reason this branch existed at all.
Numeric segments are untouched, including the null padding past the end,
which a test now pins.
Messages are this server's own words. mongod's PathNotViable text embeds a
shell-syntax rendering of the offending element (`Cannot create field 'nope'
in element {y: [ { b: 3 }, { b: 1 } ]}`) and no BSON formatter here produces
it. The code is what the corpus asserts and the code is exact; a half-copy
of the text would be worse than a clear sentence that does not pretend.
Re-running the 17-case probe against both servers: 16 of 17 diverged before,
0 are destructive now, and 5 agree with mongod's code exactly -- every case
where mongod also refuses. The rest refuse where mongod succeeds, which is
the honest not-implemented state and is what the design review chose.
Scorecard unchanged at 204 pass / 87 fail: the 14 arrayFilters cases still
fail, now reporting the refusal rather than a corrupted document. That was
the gate this review picked -- `docs/M3_ARRAYFILTERS_DESIGN_REVIEW.md` §5,
option D -- because the corpus has no `$[]` case and no bare `$` case at
all, so passing it would have certified two live ways to destroy an array.
206/206 unit (8 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, aggregation
corpus 70/0, full e2e matrix and crash-fuzz green. Both refusals are
mutation-checked: dropping the up-front scan reddens four tests on the
array's contents rather than on the error, and restoring the destructive
branch reddens the PathNotViable test.
|
||
|
A.Shakhmatov
|
3c5eee2171 |
docs: M3 design review -- arrayFilters, and the positional operators
The plan names a missing feature sized at 14 corpus cases. The measurement
found a data-loss bug.
`{$set: {'y.$[i].b': 2}}` does not fail to update the array -- it replaces
the array with `{"$[i]": {"b": 2}}` and answers ok: 1, modifiedCount: 1.
Every element is discarded. `src/update.zig:283` reaches a non-numeric
segment under an array and takes the "treat as non-array: replace with a
doc" branch, so `$`, `$[]` and `$[<ident>]` all destroy what they were meant
to descend into, under every operator -- `$inc` through `$[i]` stores the
operand rather than incrementing. 16 of 17 probe cases diverge from mongod.
`arrayFilters` is not implicated: the string appears nowhere in `src/`. The
option is accepted off the wire and dropped.
The gate finding, which is the reason to write this before any code: M3's
gate is "remaining crud coverage; e2e3/e2e4 green". e2e3 and e2e4 contain
zero positional paths and would stay green through every version of this
bug. The pinned corpus covers `$[<ident>]` only -- it has no `$[]` case and
no bare `$` case anywhere. A fix scoped to what the gate measures would go
green on 14 new passes with two of the three ways to destroy an array still
live. The gate would certify the bug as fixed.
Also measured, none of it guessable and two of them inverting how set_path
behaves today: one path can name many targets (`y.$[].c.$[].d` is a genuine
cross-product); a positional segment never creates anything, so a missing
or non-array path is an error where `$set: {'a.b': 1}` would construct, and
upsert gets no special case; ten distinct refusals across codes 2, 9 and 14,
recorded with their messages, including one row that breaks the otherwise
tidy 2/9 split and is left untidy on purpose.
Recommends refusing first as its own commit -- the M2 doctrine, and it stops
the data loss without waiting on the redesign -- then a recorded positional
corpus built like tests/spec/aggregate/, with `$[]` and `$[<ident>]`
together and `$` after, since only `$` needs the matched index carried out
of query evaluation.
|
||
|
A.Shakhmatov
|
5942f5e65f |
plan: what measuring distinct turned up
M3 opens with `distinct` recorded as done and `arrayFilters` named in its scope, and §6 gains an M3 entry for the three findings the measurement produced that are not `distinct`'s to fix. The one worth reading twice: an unknown query operator answers `ok: 1` with an empty result on find, count and aggregate alike, where mongod answers BadValue on all three. Measured on both servers side by side. That is the same shape as M2's six silent wrong answers -- a typo'd operator reads as "no matches" -- and it sits in the shared query path, so it is one fix for every command rather than one per command. Also recorded: the InvalidNamespace divergence is dispatch's answer for the whole command table, not distinct's; and distinct joins $group and $sort on the list of things unbounded in memory. |
||
|
A.Shakhmatov
|
1f141ef619 |
commands: distinct
A whole command that did not exist: five corpus cases answered "no such
command". Two things about it were measured against mongod 8.3.7 rather
than recalled, and the first is not what anyone would guess.
- The answer is **sorted in canonical BSON order**, not in the order the
values were met. `{s: "b"}, {s: "a"}, {s: null}` answers
`[null, "a", "b"]`. Insertion order is the obvious implementation, it
passes every test anybody would think to write by hand against
`[11, 22, 33]`, and it is wrong.
- Deduping is the same comparator, so an int32 `1` and a double `1.0`
collapse while `null` and `"1"` survive.
Both fall out of `bson.compare`, which `$sort` and `$min` already use --
and that is not luck: mongod accumulates into a `BSONElementSet` ordered by
the same `woCompare`. The rest reuses the shared read path: byte-walked
`collect_values_bytes` for the key, so the traversal, the multikey descent
and the numeric path segments are the ones the matcher and the index
already agree on.
Also measured: a terminal array contributes its elements exactly one level
deep (`[[7, 8], 9]` gives `[7, 8]` and `9`, never 7 and 8); a missing field
contributes nothing where an explicit null contributes null; an absent
collection, an absent database and an empty key are each `ok: 1` with an
empty array rather than an error; `query` absent and `query: null` are both
an empty filter; a missing `key` is IDLFailedToParse (40414) while a
wrong-typed one is TypeMismatch (14).
Running the identical probe against both servers now agrees on every
semantic row. Three divergences remain, all outside this command and
recorded in PLAN §6: an unknown query operator matches nothing instead of
erroring (shared with find/count/aggregate, and the same class as M2's six
silent wrong answers), a non-string collection name is refused by dispatch
as BadValue where mongod says InvalidNamespace, and an unknown top-level
field is tolerated -- deliberately, since `comment` and `rawData` arrive
through that door and the corpus requires both be ignored.
crud scorecard: 201 pass / 90 fail -> 204 / 87. distinct.json 0/2 -> 2/0,
distinct-rawdata 0/1 -> 1/0. distinct-comment nets zero: its "no such
command" is replaced by the pre-4.4.14 document-comment case, which
`estimatedDocumentCount` already carries as a standing failure -- and
emulating a bug fixed in 4.4.14 for one command would make the two
disagree. distinct-collation still needs M8, but now fails with the honest
"expected 1 elements, got 2".
198/198 unit (7 new) in ReleaseFast and ReleaseSafe, 83/83 fuzz, aggregation
corpus 70/0, full e2e matrix and crash-fuzz green.
|
||
|
A.Shakhmatov
|
fc611a2c64 |
commands: $project computes, renames and narrows
The last three cases of the corpus, which is now 70 pass / 0 fail -- every
answer byte-identical to mongod 8.3.7 across the accumulators, the expressions
and the document stages.
The fix turned out to need nothing from `query.project`, which `find` shares
and which I had expected to have to rewrite. A nested spec *is* a dotted path:
`{n: {x: 1}}` and `{"n.x": 1}` are the same projection, and dotted paths are
something the existing projection already narrows correctly. So `$project` is
flattened into inclusion/exclusion flags plus a list of computed fields, and
both halves reuse machinery that was already there -- `query.project` for the
flags, `set_path` from the document stages for the computed fields. A bare path
(`{value: "$a"}`) is a rename, which is a computed field like any other.
Both shapes used to read as *falsy*, which flipped the whole projection into
its exclusion branch and returned the entire document minus the field. That was
recorded during M2 as broken rather than unimplemented; this is the fix it was
waiting for.
One case `query.project` genuinely cannot express, so it is built directly: a
projection that only computes keeps `_id` and nothing else, and with no non-`_id`
flag that function reads the spec as an exclusion and returns everything. It
cost two failures and a `id_only` flag to find, which is what a recorded corpus
is for -- the answer is obvious once seen and not before.
`$project` now goes through the same `Rewrite` path as `$addFields`, `$unset`,
`$replaceRoot` and `$unwind`, so its own branch is gone. Its refusal shrank to
the one shape mongod also refuses, mixing inclusion with exclusion, judged on
the flattened flags so a nested spec is treated like a dotted one.
191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2
concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86, crud corpus unchanged at
201/90/196.
|
||
|
A.Shakhmatov
|
1a5386ff00 |
commands: the stages that rewrite a document
`$addFields`, `$set`, `$unset`, `$replaceRoot` and `$unwind`. The corpus goes
0 pass / 24 fail to 21 / 3, and the three left are `$project`'s computed
fields, renames and nested inclusions, which want the `query.project` that
`find` shares and are their own change.
**The design review was wrong about what this needed, and the corpus is what
settled it.** Tier 2 was scoped as a per-stage iterator on the grounds that
`$unwind` is 1->N and "there is no way to express that in a window over the
input". That was true of the window as it stood, and stopped being true the
moment `$project` was made to rebuild the stream instead of moving bounds over
it -- a stage that rebuilds can emit as many documents as it likes, or none. So
all five share one shape: read the window, build a new list, replace the
stream. No iterator, no rewrite.
What the recording settled, and what a hand-written test would have got wrong:
- `$addFields` whose expression resolves to nothing leaves the field out
entirely rather than setting it to null -- so `set_path` is only reached
when there is a value, and `eval_expr`'s absent/null distinction earns its
keep a second time.
- `$addFields: {"n.z": 1}` sets the nested path and keeps its siblings, and
an existing field is replaced *where it stands*, which is what makes the
stage "add or overwrite" rather than "append".
- `$unwind` drops a document whose field is missing or an empty array, keeps
one whose field is not an array *whole*, and numbers `includeArrayIndex`
from zero. Three separate behaviours where one guess would have covered
them all wrongly.
- `$replaceRoot` of a missing path and of a non-document are the same error,
40228.
`ReplaceRootNotDocument` joins `EvalError` rather than being reported at the
stage: it is a failure only a document can produce, which is the line that set
already draws.
191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e3 16,
e2e4 17, e2e6 72, e2e7 86, crud corpus unchanged at 201/90/196.
|
||
|
A.Shakhmatov
|
88ac010c3c |
tests/spec: record Tier 2's spec -- the stages that rewrite a document
24 cases for `$addFields`/`$set`, `$unset`, `$replaceRoot`, `$unwind` and
`$project`'s computed fields, recorded from mongod 8.3.7 before any of them is
written. The file starts at 0 pass / 24 fail, which is the honest number for
five stages this server does not have.
Eight things the recording settled, none of them guessable from the manual:
$addFields whose expression is missing the field is not added at all
$addFields: {"n.z": 1} sets the nested path, keeps siblings
$replaceRoot of missing or non-document error 40228
$unwind of an empty array or missing the document is dropped
$unwind of a non-array the document is kept whole
$unwind path without a $ error 28818
includeArrayIndex 0-based
$project: {n: {x: 1}} narrows it; a document without
`n` keeps only `_id`
That last one is the `query.project` gap recorded during M2 as broken rather
than unimplemented -- a nested inclusion currently reads as falsy and returns
the whole document minus the field. It now has a measured expectation to be
fixed against, in the corpus, rather than a note in a commit message.
Worth stating before the implementation: the design review expected Tier 2 to
need a per-stage iterator, on the grounds that `$unwind` is 1->N and "there is
no way to express that in a window over the input". That was true of the window
as it stood, and stopped being true when `$project` was made to rebuild the
stream -- a stage that rebuilds can emit any number of documents it likes. So
Tier 2 looks like five stages rather than a rewrite. The corpus is what will
say whether that holds.
|
||
|
A.Shakhmatov
|
37cfa863ee |
commands: the aggregation expression evaluator
The whole corpus is green: 46 pass, 0 fail, byte-identical to mongod 8.3.7 on
every case including all 27 expressions and the compound `_id` that was the
last accumulator failure.
Expressions are *compiled once per pipeline and evaluated per document*, and
that split is the point rather than an optimisation: it keeps the property M2's
refusals bought, which is that a pipeline that cannot be answered is refused
before a single document is read instead of half way through with part of the
work already reported. `Expr` is the compiled tree, `compile_expr` reports,
`eval_expr` cannot.
Nineteen operators: `$literal`, the five arithmetic ones, seven comparisons,
`$and`/`$or`/`$not`, `$cond` in both its forms, `$ifNull` and `$switch`. Plus
the two shapes that are not operators at all -- a compound document, which is
what a `$group` `_id` usually is, and an array.
Everything the corpus recorded, and none of it guessable:
- absent and a present null are *different* internally, because `$ifNull`
treats them alike and `$push` does not. Hence `?bson.Value` throughout,
where the obvious shortcut is to fold absent into `.null` at the boundary
and lose the distinction for good.
- arithmetic over absent or null is `null` -- not an error, not zero -- and
over a string is an error, 7157723.
- `$divide` by zero is 4848401, `$switch` with no branch and no default is
40069, and both are failures only a document can produce, so `EvalError`
exists and `report_eval_error` maps it.
- two operators in one expression document is 15983 and *not* `$group`'s
40238: mongod distinguishes an expression from an accumulator there.
- truthiness is MongoDB's, so `-5` is true and `0.0` is false.
- `$mod` follows the dividend's sign, so -5 mod 4 is -1.
- `$not` takes a bare argument as readily as a one-element array.
`compile_expr` and `compile_operator` call each other, so their error set is
written out rather than inferred -- Zig cannot infer a cycle, and the failure
mode is a "dependency loop" message that says nothing about expressions.
Three cases left the Tier 0 refusal test, because a compound `_id`, `$literal`
and a `$multiply` argument all work now. What is refused should be what is
missing, so an unknown operator and a wrong operand count took their place.
191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2
concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86, crud corpus unchanged at
201/90/196.
|
||
|
A.Shakhmatov
|
1c72aa8938 |
tests/spec: record the expression evaluator's spec from mongod
27 cases, recorded before a line of the evaluator is written, which is the
whole point of having built the recorder first: the expectations come from
mongod 8.3.7 rather than from what the implementation is about to do.
The corpus reads 1 pass / 26 fail, and the one pass is the unknown-operator
refusal M2 already answers correctly. Expressions are exercised through
`$group` because `_id` and the accumulator arguments are the only expression
positions that exist until `$addFields` and `$project`'s computed fields land;
testing them anywhere else would be testing a stage that is not there.
Ten things the recording settled that guessing would have got wrong:
$add over a missing field or null null -- not an error, and not 0
$add over a string error 7157723
$divide by zero error 4848401
$mod of -5 by 4 -1, the dividend's sign
$lt of a number and a string true, canonical type order
$and over -5 truthy
$not of a missing field true
$switch, no branch and no default error 40069
two operators in one expression error 15983, *not* $group's 40238
$subtract with one operand error 16020
The six error codes are in `ErrorCode` already, so the evaluator's refusals and
its runtime failures have somewhere measured to land. The evaluator itself is
the next commit and is not started.
191/191 unit tests, crud corpus unchanged at 201/90/196.
|
||
|
A.Shakhmatov
|
76afa75efe |
commands: the $group accumulators
Nine of them -- `$sum`, `$avg`, `$min`, `$max`, `$first`, `$last`, `$push`,
`$addToSet`, `$count` -- and the corpus goes 9 pass / 10 fail to 18 pass /
1 fail. The one left is the compound `_id`, which needs the expression
evaluator.
Landed before that evaluator, against the tier order the design review set
out, and the corpus is why: every one of these takes a single value per
document, a path or a constant, so nine of its ten failures turned out to be
reachable without one. `classify_expr` already produced exactly that value.
What the recording caught, which is the argument for measuring expectations
rather than writing them:
- `$avg` over a group with no numeric value is **null**, not `0`. A divisor
that counted documents rather than numbers would pass every test anybody
would think to write by hand, and be wrong on the one group that matters.
- `$min`/`$max` compare across types in canonical BSON order, so the maximum
of `30`, `7` and `"not a number"` is the string.
- `$push` skips an absent field but would push an explicit null, so "resolved
to nothing" and "resolved to null" cannot be the same value internally --
which is why the accumulators take `?bson.Value` and not `.null`.
- `$first`/`$last` follow input order, including when the value is absent:
`$last` of a missing field is null, not the last present one.
`AccState` is one struct rather than a union: the fields are small and every
site already switches on the kind, so a union would add a tag test where a
switch was going to be anyway. Its arrays are the gpa's, the values inside them
the reply arena's -- they outlive the group and travel with the documents.
`numeric_value` is the int32-or-double narrowing MongoDB reports, shared now
between the accumulators and `cmd_aggregate`'s count fast path. It was written
twice before; a divergence between them would make `countDocuments` disagree
with the pipeline it is a shortcut for.
`$avg` and `$push` came out of the Tier 0 refusal test, replaced by
`$stdDevPop` and `$mergeObjects`. The refusal is a property of what is missing
rather than of a list, and the test should read that way.
191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e3 16,
e2e4 17, e2e6 72, e2e7 86, crud corpus unchanged at 201/90/196.
|
||
|
A.Shakhmatov
|
3044a38d1c |
tests/spec: an aggregation corpus, recorded from mongod
M2.5's gate, built before the milestone it gates -- the same order that put
`expectEvents` before the free list in M1 and Tier 0 before everything in M2.
`mongodb/specifications` has no aggregation suite, which is amendment A6's
central finding, so this milestone has to bring its own. The hazard in a corpus
we author is obvious and fatal: it can encode our own bugs as expectations and
then agree with us forever. So the split is enforced by the tooling.
`sources/*.json` holds documents and pipelines and nothing else; `record.js`
asks a real mongod 8.3.7 what each pipeline answers and writes the unified-
format file from the reply. Inputs authored, expectations measured -- the
discipline that corrected three assumptions in M1's session work and every
error code in M2, where the alternative would have shipped both times.
No second runner. `run.js --suite-dir` points the existing one somewhere else,
so the entity model, the matchers, the skip accounting and `expectEvents` come
for free; a second runner would drift from the first exactly where it mattered.
`--scorecard` is refused with `--suite-dir`, because `scorecard.txt` is the crud
corpus's record and the milestones are compared against it -- writing it from an
unrelated run would replace that record silently.
Errors record the code and not the message: message text is mongod's to change
between releases. Group pipelines end in a `$sort`, because group output order
is unspecified and a case depending on it would fail for the wrong reason on
either server.
The first source covers `$group`: nine accumulators including the edge cases
that decide an implementation -- `$avg` over a group whose values are not
numbers, `$min` of a field no document has, `$push` skipping a missing field,
`$first`/`$last` against input order, grouping on an array, a compound `_id`.
Where it starts, run against the M2 tip:
group-accumulators.json 9 pass 10 fail 0 skip
The nine include the four refusals M2 added, which answer with mongod's own
codes -- so the corpus already confirms that half. The ten are the milestone.
The crud corpus is unchanged at 201/90/196.
|
||
|
A.Shakhmatov
|
8ebeb9d4ec |
commands: $out and $merge, written by the dispatch epilogue
The seven reachable failures of M2, and the first commit of the milestone to move the scorecard: 194/97/196 -> 201/90/196, with `aggregate-*.json` going 9 pass / 13 fail to 16 pass / 6 fail. The seven that moved are exactly the seven priced as reachable, and the six that remain are exactly the six attributed to M2.5 ($addFields, the expression engine), M4 ($listLocalSessions) and M8 (collation). Both stages write to a collection the pipeline is not reading, and three things stood against doing that in the handler: `aggregate` is a `.read` command, dispatch takes locks from a static table keyed on the command name before the handler runs, and `Collection.lock` allows exactly one collection lock at a time. So the handler computes the output under the locks it has and leaves it in `Context.pending_write`; the epilogue applies it with nothing held, beside the commit and the checkpoint already there. The `.read`/`.write` contract is amended in its own comment rather than quietly broken. `pending_write` is cleared at the top of every dispatch, so a handler that errors before setting one cannot leave the previous command's write to fire. A failed write replaces the pipeline's `ok: 1` with the failure, because a client told the aggregation succeeded would believe the collection had been written. What the stages do not implement is refused, not ignored: `$merge`'s `whenMatched`, `whenNotMatched`, `on` and `let` all select behaviour this server does not have, and a `whenMatched: "fail"` that silently merged would be the same lie Tier 0 spent three commits removing. Codes measured against mongod 8.3.7. `$out` and `$merge` answer byte-identically to it on both the replace and the upsert case. NOT ATOMIC, and said out loud in the code rather than left to be discovered. mongod replaces an `$out` target atomically; this engine has no cross-collection atomicity and no rename to build one from, so a crash between the drop and the last insert leaves the target holding part of the new output where MongoDB would leave the whole of the old. The fix is write-to-temp-and-rename and rename is a command that does not exist here. The test's mutation is the argument for the epilogue in one line: apply the write inside the `$out` branch and it deadlocks rather than fails. 191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2 concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86. |
||
|
A.Shakhmatov
|
89eae1cd9c |
plan/docs: the M2 gate is not reachable as written either
Priced the 13 aggregate failures one by one instead of counting them. Six
cannot turn green in M2 whatever is built: three need $listLocalSessions (M4)
*and* $addFields (M2.5), two need the expression engine, one needs a collation
(M8). So the gate reads '0 fail among the seven reachable', with the other six
named and attributed.
Note what the three db.aggregate() cases actually need. Implementing
{aggregate: 1} moves none of them, because each then fails on
$listLocalSessions instead. The review priced that line 'orthogonal, and
cheap'; it was cheap and worth nothing.
The seven that remain are $out and $merge, and they need a decision the review
had no authority to take. They write to a collection the pipeline is not
reading, and three things stand against that: aggregate is declared .read and
the dispatch contract says only .write commands may mutate; dispatch acquires
locks from a static table keyed on the command name, before the handler runs;
and Collection.lock's own comment states the invariant that decides it -- never
more than one collection lock at a time.
That is the same objection that kept $lookup out of M2.5's first cut, and the
review failed to apply it to the write stages in the same breath. Recorded as
the review's own error rather than quietly corrected. Two options set out in
§7, plus the durability question neither of them answers: mongod's $out
replaces the target atomically and this engine has no cross-collection
atomicity.
Nothing past Tier 0 is implemented until one is chosen.
|
||
|
A.Shakhmatov
|
61ce9805c7 |
commands: $project is a stage, not a note about how to print the answer
It set a variable that the emit applied once. Three consequences, all measured
on a live server before this changed rather than read off the code:
- only the *last* `$project` in a pipeline had any effect;
- a `$match` after one still matched on the field it had removed;
- `{y: {$literal: 5}}` read as falsy, which flipped the whole projection into
its exclusion branch and returned every document minus `y`, where mongod
adds a computed `y`.
Now it transforms the stream where it stands, materializing into the same tree
form `$group` already produced. `$match`, `$sort` and `$group` after it read
what it left, which is what "stage" means.
What it cannot do is refused rather than mis-read. A computed field needs the
expression evaluator M2.5 brings, and a nested spec (`{a: {b: 1}}`) needs a
narrowing `query.project` does not do -- both were falsy, and falsy is the
answer that quietly returned the whole document. Codes read off mongod 8.3.7,
and the replies now match it on code, codeName and message text for the empty,
the mixed and the computed forms:
projection must have at least one field 51272 Location51272
cannot mix inclusion and exclusion 31254 Location31254
unknown expression in $project 31325 Location31325
Recorded, not fixed here: `query.project` is shared with `find`'s `projection`,
where a nested inclusion has the same falsy reading and the same wrong answer.
That is a real gap rather than an unimplemented feature -- narrowing is
something this projection should do -- so it is its own fix, not a refusal, and
it belongs with whichever milestone takes `find`'s projection seriously.
Scorecard unchanged again at 194/97/196. The corpus has no `$project` stage
tests either; A6 said so, and this is the second commit to confirm it.
Mutations: drop the refusals and the empty projection answers `ok: 1`; put the
projection back on the emit and the `$match`-after-`$project` case goes red.
190/190 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2
concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86.
|
||
|
A.Shakhmatov
|
d9772c4ed5 |
commands: a pipeline may group what an earlier stage generated
A remote crash, present before this milestone and reachable by any client with
a two-stage pipeline:
aggregate: [{$group: {_id: "$k", n: {$sum: 1}}},
{$group: {_id: null, g: {$sum: 1}}}]
thread panic: index out of bounds: index 2, len 0
`$group` took `[]const u64` and was handed `offs.items[start..end]` whatever
the stream was made of. A pipeline starts as slab offsets -- matched and
reordered in place, never materialized -- and flips to generated documents the
moment a stage produces something the slab does not hold. After that `offs` is
empty while `start`/`end` count trees, so the slice ran off an empty list and
took the server thread down. There is no authentication in front of it.
Found while making `$project` a real stage, which reaches the same branch;
confirmed against the binary at the previous commit rather than assumed, so
this is a pre-existing defect and not a regression of that work. It is
committed on its own for that reason.
`Stream` names the two forms the rest of the pipeline had been carrying
implicitly in `in_trees`, and `$group` now reads whichever is live. The slab
side stays byte-walked -- grouping a million documents does not build a million
trees to read one field -- and `path_in_pairs` is the tree counterpart of
`query_path_value_bytes` for the other half.
The test groups by a key and then counts the groups, and sums `$n`, a path that
only resolves against the generated document. Its mutation -- hand `run_group`
the offsets unconditionally -- aborts the run rather than failing it, which is
why this wanted a test and not a code read.
Answers byte-identically to mongod 8.3.7 on the pipeline above.
189/189 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e3 16,
e2e4 17, e2e6 72, e2e7 86.
|
||
|
A.Shakhmatov
|
ae48e05a19 |
commands: $group refuses what it cannot compute
M2 Tier 0, and the first commit of the milestone: stop answering wrong numbers
with `ok: 1`.
`$group` had one accumulator, `$sum`, and one expression vocabulary, `$field`
paths and constants -- open-coded twice and applied to whatever arrived.
Everything outside that fell through to a zero. Measured on a live server
before this commit: `{$avg: "$x"}` answered `0`, and so did `$max` and `$push`;
a compound `_id` collapsed every document into one group keyed by the
unevaluated expression; `{$literal: 1}` came back echoed; `{$sum: {$multiply:
[...]}}` was `0`. Six of eight probed pipelines succeeded with a wrong result.
That is a worse failure than an unimplemented one. An unrecognised stage is a
bug report; an `$avg` that returns `0` is a corrupted report nobody files. It
is also invisible to the gate: the corpus this project runs has no aggregation
stage tests at all, which is what PLAN amendment A6 is about.
So the vocabulary is now named -- `GroupExpr` is a path or a constant, and
`classify_expr` is the single place the `$`-prefix distinction is made -- and
every accumulator is validated before a document is read. A pipeline that
cannot be answered is refused whole rather than half-answered.
Five error codes added, every one read off mongod 8.3.7 rather than recalled,
and the replies now match it on code and codeName for all six `$group` shapes
probed:
unknown group operator 15952 Location15952
a group specification needs _id 15955 Location15955
accumulator is not an object 40234 Location40234
two operators in one 40238 Location40238
unrecognized expression 168 InvalidPipelineOperator
`$avg` and friends are reported as *unknown* operators, which is the choice
`location_unrecognized_stage` already made for `$addFields`: this server
reports what it does not implement using MongoDB's own code for "no such
thing", because a code MongoDB never emits would break the error-code parity
every milestone is held to. The message names the construct.
`$sum` over a non-number still contributes nothing -- that is MongoDB's rule,
not a stand-in for an unimplemented one, and the comment says so where it
would otherwise read as another silent zero.
The scorecard does not move: 194/97/196 before and after, byte-identical. That
is not a disappointment, it is the design review's central finding arriving on
schedule -- the corpus cannot see any of this, which is why M2.5 has to bring
its own.
Mutations: drop the accumulator-name check and `$avg` answers `ok: 1` again;
drop the `_id` classification and the compound `_id` does.
188/188 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2
concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86.
|
||
|
A.Shakhmatov
|
b481f39ca3 |
docs/plan: all 19 aggregate skips are environmental, not 13
The scorecard collapses a whole-file skip into a single `*` line, so counting the reason lines undercounted the cases they cover. Every one of the 19 is version- or topology-gated, which is why the gate reads *0 fail* rather than *all pass*. |
||
|
A.Shakhmatov
|
7ae81b8179 |
plan: M2 splits in two (amendment A6)
The design review found the milestone's gate names a corpus that is not there: `mongodb/specifications` has no aggregation suite, and the thirteen `aggregate-*.json` files this project runs live inside `crud` and test the aggregate *command*, not stages. One name was covering two milestones. M2 becomes the command surface -- $out, $merge, db.aggregate(), collation, let -- gated on the corpus that already exists, at 0 fail. M2.5 becomes the engine: expression evaluator, per-stage document iterator, accumulators, $unwind; $lookup and $facet explicitly out of the first cut, gated on a corpus this project writes with every expectation measured against mongod. Command surface first, engine second, chosen with the cost stated: the engine is what gives wrong answers now -- six of eight probed pipelines answer ok:1 with a wrong result -- so this order leaves them alive a milestone longer. Which is why M2 carries the refusals: every unimplemented construct stops answering 0 and starts answering an error with a measured code, the same move expectEvents made before the free list in M1. It will push the scorecard down, and that is the point. The review keeps its pre-decision recommendation verbatim, so the argument the decision overrode stays legible. |
||
|
A.Shakhmatov
|
37211a6cda |
docs: M2 design review
PLAN §3 gives M2 one line of scope and one line of gate, and §6 lists the three questions it deferred. This answers them, and reports one finding that has to be settled before the rest is worth discussing: the gate named in the plan does not exist. `mongodb/specifications` has no aggregation suite -- the thirteen `aggregate-*.json` files we run live inside `crud` and test the aggregate *command* surface, not stages. $lookup, $unwind, $facet, $addFields and $replaceRoot appear nowhere in the pinned corpus. Measured, not recalled. Seven stages exist, not the nine an earlier note in this project claimed -- $set and $unset are update operators. There is no expression engine: $field paths are open-coded in two places inside run_group and $sum is the only accumulator. Probed on a live server, six of eight pipelines answer ok:1 with a wrong result -- $avg, $max and $push all return 0, a compound _id groups everything into one bucket keyed by the unevaluated expression, $literal is echoed back, and a $match after a $project still sees the projected-away field. Only $addFields fails honestly. That drives the tiering. M2 is not "add stages to the chain": the stream is a materialized window and each stage moves its bounds, which cannot express 1->N ($unwind), a second collection ($lookup), or sub-pipelines ($facet), and $project is not a stage transform at all. Six tiers proposed, four gate options priced, and a recommendation: Tier 0 (refuse what is not implemented) alone first, for the same reason expectEvents preceded the free list in M1 -- it will move the scorecard down, and that is the point. No decision is recorded in PLAN.md yet; that is what the review is for. |
||
|
A.Shakhmatov
|
a748a3d08c |
db/pager/tests: cleanup pass over the free list
No behaviour is meant to change and the gate confirms it: 1.94x / 679.2 MB
reclaimed / 9 rebuilds and 2.46x / 934.0 MB / 13 on the two 16 KiB lines,
0.0 MB on the 200-byte line, all identical to the numbers recorded for them.
Deduplication. `pages_for` was written in db.zig and again in pager.zig and
twice more inline; there is now one, public, and the two pre-existing copies
call it. `pages_per_map_align` replaces three hand-rolled `map_align /
page_size`. `SlabRun.window_first` was a stored field that could never legally
disagree with `first` and was maintained by hand at two sites -- now a method.
`keep_piece` re-derived `SlabRun.window_count` character for character; it
calls it. `insert_run` scanned linearly for a position `run_of` binary-searches
for, which made loading a fragmented catalog quadratic; both now go through one
`run_lower_bound`. Freeing a run's window map was written four times; one
helper. The 20% rebuild share was stated in `note_compact` and again in
`wants_rebuild`, with a comment arguing at length that they must be the same
number -- `worth_rewriting` makes that structural.
Efficiency. The identity assert in `reclaim_windows` called `dead_located()`,
an O(every window) walk, and `assert_msg` is live in ReleaseFast -- so it
doubled the scan the reclamation was about to make (2.75 MB streamed twice per
reclaiming checkpoint at the 21 GB the gate targets). `dead_located` is now a
maintained counter, the check is O(1) in every build, and the scan cross-checks
it while it is there. `SlabRun.full` lets a run with nothing to give be copied
without its counters being read at all, so the common case is O(runs) rather
than O(windows).
The pager's two allocation policies were hand-copying the claim step, and the
copy had already lost two of the three preconditions -- `alloc_slab_run` never
checked `pages <= reserved_pages`. Both now go through `claim_locked`.
`reclaimed_bytes` moves from Collection to Engine, beside `compactions`, which
is how it is read and the only place it can be honest: a life-of-the-process
total must not lose a dropped collection's share. Both join `Counters`, so
`slab_stats` stops opening `counter_lock` by hand.
The ownership assertion in `write_catalog` was gated on `is_test or Debug`, a
predicate nothing else in the codebase uses, which left the one silent failure
this design can produce unchecked in ReleaseSafe. It is now `!= ReleaseFast`,
the line `protect_stable` already draws. Measured: no change to the suite's
runtime.
Altitude. `note_checkpoint` was called from exactly one place, the tail of
`upsert` -- so a delete armed no checkpoint by any route, which is why
reclamation only ever ran when the *rebuild* trigger fired and the rebuild then
reset the window map it would have used. `remove` and the TTL sweep arm one
now, next to the `note_compact` calls that were added for the same omission a
milestone ago. `compact`'s leading checkpoint stays, demoted in its comment
from the mechanism to the local ordering it actually guarantees.
serverStatus reports `allocTailBytes`/`freeReadyBytes` instead of page counts,
so the harness stops hard-coding 4096 -- the kind of constant this milestone
was blindsided by once already.
tests/e2e/churn.js: `deleteMany({_id: {$in: [5000 ids]}})` exceeded
`index.max_combos`, so the planner refused the index and every delete became a
full collection scan re-filtering each document against 5000 members. That was
the entire runtime of the harness. One delete spec per id instead: the 40k x
16 KiB gate goes 57 s -> 6 s, and the 150k x 200 B line 483 s -> 3 s, with
identical output. Also: the per-round `countDocuments` is gone (the harness
knows the count), and the server log is a bounded ring rather than a rope that
grows with everything the server ever said.
Reverted from the review: reusing one MongoClient across the startup poll. A
client whose first connect fails tears its topology down and every later
command on it fails identically, so it turns "not up yet" into "never comes
up" -- it broke the first run. The reason is now a comment.
187/187 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2
concurrent 2 and the crash pair, e2e3 16, e2e4 17, e2e6 72, e2e7 86,
crash-fuzz 60 cycles.
|