8136ffe8d4cf500df2dbd06a9d3bf847b43cf311
126 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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.
|
||
|
A.Shakhmatov
|
36311d0faa |
plan: the in-place replace commit, declined on the measurement
The plan made it conditional on the update line still being the worst. It is -- but the gate says the peak is set by the first rebuild's second copy, not by write amplification, so generating less garbage moves the number by nothing. That would be a change to the write path, on the stretch that runs after the log record is durable, for no measured gain. Recorded as declined with the reason rather than done because it was on the list. |
||
|
A.Shakhmatov
|
1491a47479 |
plan/results: the M1 churn numbers
Amendment A5 and the `[M1.1]`/`[M1.2]` blocks. What they record is a result with two halves, and the value is in keeping both: The mechanism works. 934 MB reclaimed over the update run, occupancy at 1.06-1.26x its live data, and the counters that say so are in `serverStatus` rather than inferred. The ratio did not move. 1.94x delete-heavy and 2.46x update-heavy, identical to the end-of-Stage-2 binary measured with the same harness. `file / live` is a high-water mark because the data file never shrinks, and the mark is set in the first round by the one thing reclamation cannot avoid: a rebuild needs a whole second copy of the live data before the first can be freed. So ~2x is the floor of a rebuild-based design and no threshold reaches it -- rebuilding earlier lowers the garbage term and nothing else, rebuilding later raises it. The plan said in advance what to do if this happened, which was to write it down rather than tune, and to name incremental compaction through a doc-id-to-offset indirection layer as the successor. Recorded, with its cost: a second copy-on-write B+tree per collection, a second random read on point lookup, and it undoes A3. A second lever is named that the plan had not: 52% of the steady-state file is space the database owns and is not using, so returning it to the filesystem is worth more here than reclaiming harder. It needs the file never to shrink below what the fallback generation references, which is its own crash-safety pass. D7.4's 1.65x for the delete line is corrected to 1.94x, and the correction is the harness rather than a regression -- the same 1.94x comes out of the binary that predates any of this work. The old ad-hoc version sampled ids to delete blindly, which re-picks dead ones, so it deleted fewer documents than it inserted and measured a collection that was quietly growing. The update line reproduces D7.4 exactly, 2.46 against 2.47. 200-byte documents reclaim nothing, exactly as forecast, and the forecast being written down beforehand is what makes that a result instead of a disappointment. 3.93x on both binaries. Also noted, because the number invites misreading: at that document size the two index trees are comparable to the documents themselves and the file is already 2.18x before any churn -- index structure, not slab garbage. |
||
|
A.Shakhmatov
|
d492726881 |
db/pager: the catalog may not claim a page that is on the free list
Reclamation runs as a checkpoint phase, so it frees pages concurrently with everything else -- and the one failure that arrangement can produce is silent. A catalog that claims a page already handed to the pager gets that page back two generations later, written over by somebody else; the crash that falls back to that generation then reads a document which is no longer there. Nothing fails at the time, and the `seq` retry cannot see it because neither a reclamation nor a rebuild appends a log record. So `write_catalog` now asserts it, per run, in test and Debug builds -- three list scans where every run is being walked anyway. It is proven to fire: have `reclaim_windows` free the pages and keep the old run list, and the suite panics on it. This is the double-ownership detector the plan said an enlarged free list deserves. And `checkpoint` takes a lock of its own. Two can be in flight -- a writer's epilogue claims the pending flag while another is inside `compact`, which checkpoints of its own. The publish was always safe, since it runs under `log_lock`; the phase in front of it is new. One checkpoint's `reclaim_slabs` frees pages under a collection's lock that the other's `write_catalog` may already have serialized, and that is exactly the shape above. Stated plainly: the argument for the lock is by construction, and no test reproduces the interleaving -- removing it leaves the new concurrency test green. What that test does do is run reclamation under two checkpointers and a writer with the ownership assertion armed, which is the harness that would catch the argument being wrong. This is the second instance of the shape PLAN records as still open (a rebuild frees pages under only the collection's lock while a checkpoint may have snapshotted a catalog claiming them). Reclamation is now excluded from it; `compact`'s rebuild walk still is not, and that remains recorded rather than fixed here. 187/187 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, the full e2e matrix, crash-fuzz 60 cycles. |
||
|
A.Shakhmatov
|
8f63c6df70 |
tests/e2e: the churn gate, as a committed harness
D7.4 was the only block in `tests/e2e/results/m0-gates.txt` without a `reproduce:` line. The numbers were real and the harness was not committed, so the one measurement the whole free-list decision rested on could not be re-run against a change. This is that harness. Self-contained like `e2e6.js`: it spawns its own server on a fresh database. Two modes, delete-and-refill and repeated update, over `--docs` documents of `--doc-size`, with `--index` to put index maintenance inside the churn rather than beside it. Three things it does that the ad-hoc version did not: Live bytes are computed here, from the serialized size of one document, rather than read off the server. That is what makes a run against an older binary comparable -- and the first thing this harness was used for was measuring the pre-Stage-3 binary, which has no `multifora` section at all. Deleted ids are sampled from the ids actually live. Sampling blind from the id space re-picks dead ones, so a round deletes fewer documents than it inserts and a supposedly flat-live measurement quietly grows. The first run of this harness ended with 3211 documents where it should have had 2000. And it prints `inUse` beside `ratio`. The data file never shrinks, so `file / live` is a high-water mark and cannot come down however well reclamation works; `inUse` is `(allocTail - freeReady) / live`, which is what the database is actually occupying. On the update line those two read 2.46x and 1.06-1.26x for the same run, and the difference between them is the whole finding. A fixed seed, so two runs churn the same documents in the same order and a difference between them is the code rather than the dice. `--target x` fails the run above a ratio, for use as a gate; without it the harness measures and reports. |
||
|
A.Shakhmatov
|
343b25adc8 |
db: a rebuild reclaims before it copies
Found by the churn harness, and it is the difference between reclamation working and reclamation being unreachable. A checkpoint is what hands back empty slab windows, and a checkpoint is armed by log volume. A delete logs only an `_id`. So deleting half of a 190 MB collection moved the log by a couple of megabytes, no checkpoint ran, and the garbage sailed straight past the rebuild threshold -- the rebuild got there first every time and reset the window map it would have used. Measured before this: six rounds of delete-and-refill, six rebuilds, 1 MB reclaimed. After: the same six rounds, 256 MB reclaimed. The fix is one line of ordering. `compact` now checkpoints before it walks the collections, so the cheap half of the job runs first: a checkpoint hands back whole windows for the cost of one publish, where a rebuild copies every live byte in the database. The per-collection gate then judges what reclamation left rather than what it was about to take, so a collection whose garbage was all in empty windows is not rewritten at all. No new threshold and no new state -- the gate that decides is the one added in "a rebuild copies only the collections that have garbage", now reading a post-reclamation number. 186/186 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, the full e2e matrix, crash-fuzz 60 cycles. |
||
|
A.Shakhmatov
|
8bf35e707c |
commands: serverStatus reports what the slab is doing
A `multifora` subdocument -- named so nobody mistakes it for a MongoDB section -- carrying `liveBytes`, `deadBytes`, `slabBytes`, `reclaimedBytes`, `slabRuns`, `freeReadyPages`, `allocTail` and `compactions`. The milestone's gate cannot be read without them. A steady-state size ratio can look respectable while reclamation does nothing at all: the file grows, a rebuild periodically halves it, and the average comes out fine. What distinguishes the two is `reclaimedBytes` rising while `allocTail` stays put, and no ratio shows that. Same for rebuilds -- "the ratio improved" and "the ratio improved because reclamation worked rather than because a rebuild ran" are different results, so `compactions` counts collections rewritten. The byte figures are summed from the collections rather than read off the engine's running totals, so this reports the same side of the comparison `checkpoint` asserts. A counter that had drifted from the catalog would otherwise make the gate measure the drift. Every field is present at zero. A gate that cannot tell "no pages ready" from "field missing" cannot be read at all. 186/186 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, and the full e2e matrix. |
||
|
A.Shakhmatov
|
0dd9a0c908 |
db: a rebuild copies only the collections that have garbage
`compact` walked every collection unconditionally, so garbage in one paid for a full copy of the other thirty-nine. A copy is not free even where it reclaims nothing: it rewrites every document and every index, and it bumps the layout epoch, which kills every open cursor on a collection that had no reason to be touched. The gate is the share `note_compact` already applies to the engine's totals, and using the same one is what keeps them from disagreeing. If no collection passes it then `dead_i < live_i / 4` for every one, so `sum(dead) < sum(live) / 4` and the engine's trigger could not have fired either -- a compaction that runs always rebuilds at least one collection and cannot spin re-arming itself over garbage no rebuild will take. An absolute floor per collection would break exactly that: forty collections each under the floor can sum to well over it. Two existing tests needed real garbage, which is the change working. "a rebuild kills an offsets cursor and spares a streaming one" and "the epochs that invalidate a cursor move exactly when they must" both called `compact` on a clean collection and relied on it rewriting anyway. The first now rewrites all 60 documents (a replace, not a delete, so the drain still checks that the stream yields exactly 60 once each -- and with a changed field, since an identical replace is deliberately not a write). The new test asserts both halves: the dirty collection's epoch moves, the clean one's does not, and the clean one's `slab_used` is unchanged -- a repack starts the slab over, so that number could not survive one. Mutation: delete the guard, and the clean collection's epoch moves too. 185/185 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. |
||
|
A.Shakhmatov
|
7fe1009243 |
db/pager: a slab extent comes off the free list when one fits
Without this the previous commit is decorative. Windows go back, the free list fills up, and the file grows by the whole write volume anyway -- because nothing asks for the pages in the shape they arrive in. `take_free` cannot serve a slab extent from reclaimed windows, and that is on purpose. It is best fit precisely so the thousands of single-page copy-on-write requests per generation cannot dismantle the large runs; the consequence is that a 2048-page extent request never matches anything smaller, and reclamation hands back runs a few windows at a time. So `alloc_slab_run` is a second policy in the same allocator: at least `min_pages`, at most `max_pages`, longest available so the collection switches extents as rarely as possible, ties to the smallest source run so the big ones stay as whole as they can. It takes a partial run when it cannot have a whole one and it is allowed to trim a larger one -- there is no cannibalisation to fear when the request is itself at least 1 MiB, and what it leaves behind is a run rather than a hole. `take_free` is untouched and its pinned first-fit mutation test still passes. What it hands out is aligned to `map_align`, which is not cosmetic. That is the granularity writeback tears at and the granularity reclamation gives back at, so a run starting mid-system-page both wastes its first window and shares a kernel page with whatever holds the rest of it -- for a page still in the published image, exactly the tearing `mark_appendable` refuses to risk. The trimmed edges stay on the free list. The caller's floor is 1 MiB: a shorter extent is exhausted after a handful of documents and every exhaustion writes off what is left of the one before it. Measured, in the new engine-level test: delete-and-refill of 400 16 KiB documents per round, four rounds. The tail stands at 2068 pages after the first round and 2083 after three more of the same volume -- 15 pages of growth against 4800 pages written. That is the number the whole milestone is about, and it is the one Risk 5 in the plan says to check directly rather than inferring from a ratio. Three tests. The churn one above. The pager's policy: what it hands out starts and ends on a system-page boundary, comes out of the long run rather than the short one and past the long one's unaligned first page, and everything not handed out is still on the list; a run below the floor is left alone. Mutations: raise `slab_run_min_pages` to a whole extent and the churn test goes red (which is also the measurement saying the floor has to stay well under an extent); drop the alignment and the pager test goes red on an odd-page run; drop the `usable < min_pages` test and the short run is handed out. 184/184 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. |
||
|
A.Shakhmatov
|
afdb44fe90 |
db: a slab window with nothing live in it goes back to the pager
The reclamation itself. A checkpoint now begins by handing back every `map_align` window whose dead-byte counter has reached `map_align`, splitting the runs around what is kept. Counting is the entire liveness test, and that is what makes this cheap. `evict_doc` removes a document's index entries before marking its bytes dead, so a window reaches `map_align` only once every document with a byte in it is unreachable -- "no live bytes" and "no reference to these bytes" are the same statement, established without scanning anything. Inside `checkpoint` rather than a hook after it, because reclamation changes two things that must agree: `slab_runs`, which the catalog describes, and the pager's free list. One `publish` makes both durable. A crash before it leaves the old catalog and the old free list -- no reclamation happened -- and a crash after leaves both describing the new ownership. No new record type, no replay path, no ordering in between to get wrong. It also means the write path pays only for a counter update and the cadence of returning pages is the checkpoint threshold. `full_windows` counts windows that have reached `map_align`, so a collection with nothing to give back is not scanned at all. Without it every checkpoint would walk every window of every collection -- O(slab) regardless of workload, and the workload this design is known not to help (small documents on 16 KiB pages) is exactly the one that would pay it for nothing. A failure changes nothing: the replacement run list is built whole before the old one is touched, so out of memory means the garbage stays and the next checkpoint tries again. Past the last fallible step the list is swapped in first and the pages handed over second; a `free_pages` that fails there leaks the run, which costs space. The other order would leave pages owned twice. `slab_used` changes meaning from "bytes ever appended" to "slab consumed and not yet given back". That is what keeps `slab_used - live_bytes` equal to the garbage the collection still has, with no new persistent field -- both halves are already in the catalog. `layout_epoch` is bumped when, and only when, a collection actually gave something back. Reclamation does not move a live document, so a cursor's live offsets stay good; but a saved offset list can name a page now on the free list, and reading it would succeed and return plausible garbage rather than fail. `cursor_still_valid` kills such a cursor with QueryPlanKilled, as a rebuild already does. Not bumping it otherwise matters just as much: every cursor on a busy collection would die on the checkpoint cadence for nothing. Three tests. The survivor: 199 of 200 documents deleted, and the window holding the last one is not given back, still reads, and goes back only once it is empty too. The deferral: a reclaimed page is in `free_hold` after the publish that freed it and in `free_ready` only after the next one -- asserted on the pager's lists, since `free_ready_pages()` also moves for copy-on-write victims. And the epoch, added to the existing three-promise cursor test: unmoved by a checkpoint that reclaims nothing, moved by one that does. Two Stage 0.6 tests changed, in the direction that was the point. "the slab counts what the appender skips" asserted that an abandoned extent tail survives a checkpoint, because only a rebuild could reclaim it; now the checkpoint gives 4 MiB of it straight back and what stays is the edges. "a rebuild leaves behind what its own copying skipped" asserted 2-4 MiB left after a rebuild; the checkpoint inside `compact` reclaims most of that now, so it asserts the same thing about a smaller number -- the counter is not zeroed and equals what the collection has. Mutations: relax the fullness test to accept a window 2 KiB short of empty and the suite aborts on the append-cursor assertion (the appender's own window is the first thing a loosened test reaches); drop that assertion too and the survivor test goes red alone. Remove the `reclaim_slabs` call, 5 tests fail. Remove the epoch bump, 1. Invert the `full_windows` shortcut, 5. 182/182 unit tests in ReleaseFast and ReleaseSafe, 82/82 fuzz, e2e 49, e2e2 concurrent 2 and the crash pair, e2e3 16, e2e4 17, e2e6 72, e2e7 86 (the cursor suite, which the epoch bump was most likely to redden), crash-fuzz 60 cycles. |
||
|
A.Shakhmatov
|
23b283ff44 |
db: the slab knows where its dead bytes are
Inert on its own: nothing is reclaimed yet and no behaviour changes. What
changes is that a collection can now answer *where* its garbage is, which is
the precondition for handing any of it back.
A slab extent becomes a `SlabRun`: the same two u32s plus a dense array of
dead-byte counters, one per `map_align` window. The window is the unit because
it is the smallest thing that can be given back at all -- `mark_appendable`
refuses an unaligned start and `protect_stable` rounds outwards -- so a
counter never exceeds `map_align` and its width follows from that. Two bytes
per window is the entire memory cost: 2.7 MB for a 21 GB slab on 16 KiB pages.
The shapes that track dead *documents* instead (an interval set, a free-run
list) cost gigabytes at the 200-byte document scale of D7.3, and would make
`evict_doc` allocate after the write is already committed, which is a failure
with nowhere to go.
`mark_dead` is therefore infallible, and is called from the two places slab
dies: `evict_doc`, after every index entry naming the bytes is gone, and
`note_skip`, for what the appender writes off at a checkpoint or when it
abandons the tail of an extent. Ordering `mark_dead` last in `evict_doc` is
what will make reclamation by counting alone sound -- a window reaches
`map_align` dead only once every document touching it has been through there.
The run list is now sorted by page number rather than allocation order. That
was free while an extent could only be appended to; a recycled run arrives
*below* one the collection already owns, and `run_of` is a binary search.
Sortedness and non-overlap are asserted at the single point runs enter.
Two counters accompany it. `dead_unlocated` holds garbage that has no window:
the head and tail of a run outside its whole windows, and -- the larger share
-- everything that died before the last restart. It exists so one identity
stays exact:
sum of window counters + dead_unlocated == slab_used - live_bytes
Left side is where, right side is how much; reclamation reads the first and
the compaction trigger reads the second, and a drift between them is either a
rebuild firing on a clean database or a window handed back with a live
document in it. `reclaimed_bytes` is inert here and exists for the churn gate,
which cannot otherwise tell "the ratio improved because reclamation worked"
from "the ratio improved for another reason".
The catalog is byte-identical: still `u32 count, (u32 first, u32 pages)*`, so
`catalog_version` stays 1 and there is no second read path. The window map is
deliberately not persisted -- an open puts the whole amount into
`dead_unlocated` instead. The consequence runs one way: a forgotten dead byte
is a window that is not handed back, never a live window that is. Reading
inserts sorted rather than appending, so a catalog written before this commit
loads into an ordered list.
Five tests. The accounting identity across both kinds of death; run edges
counted but not placed, driven against `mark_dead` directly since the
alignment of a real extent is the allocator's business; documents never
straddling a run, over a collection with an oversized document in a run of its
own; a run recycled to a lower address keeping the list ordered and findable;
and a restart forgetting where the garbage is but not how much.
Mutations, each red on its own: drop `mark_dead` from `note_skip` (30824 vs
0), from `evict_doc` (151304 vs 30824), the run-tail branch (16384 vs 12288),
the run-head branch (26 tests crash on the underflowed window index), the
sorted insert (overlap assert fires), and the `dead_unlocated` line in
`read_catalog` (40240 vs 0).
180/180 unit tests in ReleaseFast and ReleaseSafe, 82/82 fuzz, e2e 49, e2e2
concurrent 2 and the crash pair, e2e3 16, e2e4 17, e2e6 72, e2e7 86,
crash-fuzz 60 cycles.
|
||
|
A.Shakhmatov
|
e416ad179a |
plan: sessions, and the three measurements that corrected it
Records what `lsid` support is and what it deliberately is not, so M4 inherits the decisions rather than the questions. The part worth keeping is not the design but its corrections: three of the assumptions this stage was planned on turned out to be wrong when measured against a real mongod, and one of them -- that unknown fields inside `lsid` are tolerated -- would have shipped a divergence nothing in the test corpus could have caught. Also states that the scorecard did not move, 194/97/196 either side, which was the prediction rather than a surprise: the corpus has no session entities at all. What is new is that the prediction is now checkable -- after the event assertions landed, a command wrongly refused here would show up as a changed event stream instead of silently. |
||
|
A.Shakhmatov
|
3e2d0ab134 |
commands: a count reply is an int32
Two of the tests added alongside endSessions read the count reply's `n` as int64. ReleaseFast does not check the union tag, so both passed there and aborted in ReleaseSafe -- which is the whole argument for running the suite in both, and the reason the plan makes ReleaseSafe non-optional. 175/175 in ReleaseFast and ReleaseSafe. |
||
|
A.Shakhmatov
|
f5c827e581 |
commands: endSessions judges the array it is handed
It stays a no-op -- there is nothing to end -- but stops being a blind `ok`. This is the one session command that arrives in normal operation: a driver sends it on close for every session it handed out. Validating an argument that is then discarded looks like ceremony, and is not: `ok: 1` to a malformed `endSessions` says the server understood something it never read, which is the same class of answer the previous commit removed for `txnNumber`. Codes and messages measured against mongod 8.3.7, including the field path `endSessions.endSessionsFromClient` -- an IDL artefact, since the command's own field is `endSessions` and the parsed argument carries a different name. It is reproduced rather than tidied: a path that differs from the real server's is worse than an odd one. With this the raw probe's replies are identical to mongod's for every lsid and endSessions shape. What is left is five deliberate divergences, all recorded: three because mongod knows per command whether `txnNumber` is even accepted and answers Location50889 or 263 before reaching the standalone refusal, and two because `startSession` and `refreshSessions` are not implemented -- a driver calls neither, generating session ids locally, so CommandNotFound is the honest answer. `commitTransaction` joins them, and it can only be reached by a client whose write this server already refused. 175/175 unit tests. |
||
|
A.Shakhmatov
|
54092f44be |
commands: accept a well-formed lsid, refuse what would be a lie
A driver puts `lsid` on every acknowledged command already, because `add_server_info` advertises `logicalSessionTimeoutMinutes`. So this is not new plumbing, it is a decision about input that has been arriving all along and being ignored. Accepting it and doing nothing is honest: a session here would own nothing -- no transactions to scope, no retryable writes, cursors that outlive their connection for their own reasons. There is deliberately no session registry; it would be a mutex on the dispatch path guarding state nothing reads, and M4's transaction state machine is what should decide its shape. `txnNumber` is a different matter, and ignoring it would be the lie this commit exists to remove. A transactional write would run non-transactionally, answer `ok`, and become durable; the client would find out at `commitTransaction`, by which time the data is on disk. It is refused, with `startTransaction` and `autocommit` alongside it for the same reason. Every code and every message was measured against mongod 8.3.7 through a raw OP_MSG probe -- the driver overwrites `lsid` with its own session, so a malformed one cannot be sent through it and none of this was checkable the usual way. Three things the measurement settled that guessing would have got wrong: an unknown command with a malformed lsid answers CommandNotFound, so the lookup comes first and this check belongs exactly where it sits; the codes for a bad session id are IDL parser codes (40414, 40415) rather than anything resembling the rest of our table; and a bad UUID length is InvalidUUID 207 while a bad subtype is TypeMismatch 14, which no amount of reasoning would have produced. Three divergences from mongod, all one cause: it keeps a per-command table of which commands accept `txnNumber` at all, and answers Location50889 or OperationNotSupportedInTransaction 263 for those that do not, before reaching the standalone refusal. We have no such table and give the standalone answer uniformly. For every CRUD command -- everything a driver would actually send these fields on -- the replies are identical; they differ only on things like `ping`, where mongod is more specific rather than differently right. Checked before any lock is taken, and the test for that is the second half of each refusal: keep using the engine afterwards, with a write that has to take the catalog exclusive to create a collection. Mutation-checked by moving the call below `lock_catalog` -- the test run hangs, which is precisely how the nameless-command lock leak presented, since a leaked *shared* lock is invisible to every reader. 174/174 unit tests. |
||
|
A.Shakhmatov
|
0034da4293 |
wire: parse a logical session id out of a command body
An accessor over the command envelope, next to `db_name` and the same shape: called from dispatch, never from `parse`, because a malformed session id is a command that gets an error reply, not a connection that gets torn down. It returns the 16 bytes or names what is wrong; the codes stay in the command layer, where they were measured. The tolerated fields were measured against mongod 8.3.7 rather than recalled, and the measurement contradicted the assumption this was designed on. The design said unknown fields inside `lsid` would be tolerated, on the reasoning that the server tolerates unknown fields everywhere and pinpoint strictness would be inconsistent. mongod answers IDLUnknownField (40415) -- it is strict here and the reasoning was simply wrong. It also accepts `uid`, the hash of the credentials owning the session, which a driver starts sending the moment authentication is on; rejecting that would have broken every command in M7, and the test says so where a future reader will meet it. `txnNumber` and `txnUUID` inside `lsid` are refused. They are not the retryable-write `txnNumber` that sits outside it: together they name an *internal* session, one that runs a transaction on another session's behalf. mongod refuses them on a standalone too. Also `bson.Value.type_name`, which is mongod's name for a type rather than Zig's -- a TypeMismatch message quotes it, and a driver that matches on the text is matching on these. 170/170 unit tests. |
||
|
A.Shakhmatov
|
e65da2740e |
plan: what the event assertions found
Four defects and one deliberate non-fix, recorded where the milestone can see them rather than only in five commit messages. The one worth carrying forward: the runner dropped `collectionOptions`, so every "unacknowledged write" case in the corpus had been running an acknowledged write and passing, because the two produce results the expectation accepts either way. Only the wire distinguished them and nothing read the wire. Also states plainly that scorecards from before this are not comparable with ones after, and why `bypassDocumentValidation` is left failing: both available refusals are worse than four undeserved entries in the fail column, and one of them is the shape that turns a scorecard into flattery. |
||
|
A.Shakhmatov
|
6d5c860e11 |
tests/spec: an event's command is compared in the shape it was sent
A command-monitoring event hands over the command as the driver holds it in
memory, and that is not always the shape it puts on the wire: a sort is a JS
`Map` (driver lib/sort.js). `Object.keys` on a Map is empty, so the matcher
reported every key of an expected sort as missing from a command that in fact
carried it -- five cases, all of them the runner's fault and none the
engine's.
This one is worth the paragraph because of how well it hides. EJSON serializes
a Map exactly like a document, so `MFDB_DUMP_EVENTS` prints
`"sort":{"_id":1}` next to a failure that says `sort._id` is missing, and the
dump -- the tool built for exactly this triage in the commit that added the
buffers -- reads as evidence that the matcher is wrong about something else.
It took `Object.keys(formatSort({_id: 1}))` returning `[]` to see it.
Converted for the comparison only, and at every depth, since a sort also
appears inside `updates[i]`. `match` stays a plain reading of the spec's
Evaluating Matches with no driver knowledge in it.
Mutation-checked: pass the event's own value through and
findOne.json "FindOne with filter, sort, and skip" goes red again with the
original message.
189/102/196 becomes 194/97/196.
|
||
|
A.Shakhmatov
|
548c882d47 |
commands: the wire version says what the version string says
`buildInfo` has always reported 4.4.0 and the handshake has always reported maxWireVersion 8, which is 4.2. A driver believes the wire version: it refused client-side to send `hint` on an unacknowledged delete or findAndModify (the error is "only supported on MongoDB 4.4+", raised without a round trip), and withheld `comment` from getMore, listCollections and listDatabases (lib/operations/get_more.js:43 and its neighbours). Both are things this engine handles -- the acknowledged hint suites pass, and getMore ignores fields it does not know -- so the effect was purely the number disagreeing with itself. The 8 was not arbitrary. The comment above it tied it to omitting `topologyVersion`, which is what keeps a driver off the streaming hello protocol we do not implement -- a real bug, once visible as Compass reconnecting every heartbeat. Checked before touching it, in the driver rather than from memory: `useStreamingProtocol` (lib/sdam/monitor.js:154) returns false whenever `topologyVersion` is absent and never looks at the wire version at all. The omission is the whole mechanism; the wire version was a second line of defence that never existed. The comment now says so. A test asserts the two agree, so they cannot drift apart again silently, which is the actual defect here -- not the value. Spec suites 173/118/196 -> 189/102/196: 16 cases fixed, none broken. Ten are the unacknowledged-hint cases the previous commit uncovered, six are `comment` forwarding. 166/166 unit tests in ReleaseFast and ReleaseSafe, 82/82 fuzz, e2e 49, e2e2 concurrent 2 + crash pair, e2e3 16, e2e4 17, e2e6 72, e2e7 86, crash-fuzz 60 cycles. |
||
|
A.Shakhmatov
|
7f426cdd33 |
tests/spec: a collection entity gets the options it was declared with
`buildEntities` built every collection as `db.collection(name)` and every
database as `client.db(name)`, dropping `collectionOptions` and
`databaseOptions` on the floor. 15 collection entities declare a
`writeConcern`, 7 a `readConcern`, one a `readPreference` -- and the 15 are all
`{w: 0}`, so every "unacknowledged write" case in this corpus has been running
an acknowledged write against a driver that was never told otherwise.
They passed anyway, because an acknowledged and an unacknowledged write of the
same document produce results a `$$unsetOrMatches` expectation accepts either
way. Only the command on the wire distinguished them, and nothing was reading
the command until the previous commits. This is the first thing the event
assertions found, and it is a fair answer to what they cost.
Option documents are unwrapped from their BSON types on the way to the driver.
The suites are parsed with `relaxed: false`, so `{w: 0}` arrives as an Int32
and the driver gates `writeConcern.w` on `typeof w === 'number'` -- the same
trap NUMERIC_OPTIONS already documents for operation options, and a silent one:
the option would simply not apply. Wholesale unwrapping is safe here in a way
it is not there, since these are settings the driver consumes rather than
values an assertion compares. An option key outside the spec's
`collectionOrDatabaseOptions` set is reported unsupported rather than ignored,
which is the lesson of the bug itself.
159/132/196 becomes 173/118/196. 14 cases fixed, none broken.
The other 10 unacknowledged cases now fail differently, and that is progress
of a sort: with `w: 0` actually applied, the driver refuses client-side to send
`hint` on a delete or findAndModify to a server older than 4.4. This engine
reports itself as 4.4.0 with maxWireVersion 8, and 4.4 is wire 9. That
inconsistency is ours, it is the same one behind the `comment`-on-getMore
failures, and it gets the next commit.
|
||
|
A.Shakhmatov
|
bb8cdd964b |
tests/spec: the scorecard no longer disclaims expectEvents
The disclaimer was accurate for as long as it stood -- events were not read,
so `pass` was an upper bound and saying otherwise would have been a lie about
the number. It is now a lie in the other direction, so it goes, replaced by
what is actually true: events are compared exactly, in number and in order,
which is what makes a pass mean the engine answered correctly *and* was asked
the right question. The header says plainly that scorecards recorded before
this are not comparable, and enumerates what is still skipped inside events
rather than leaving "asserted" to be read as "asserted completely".
Two facts in the docs had gone stale and are corrected here because this is
the commit that rereads them:
- README said `--op-timeout-ms` defaults to 3 s. It has been 10 s since the
commit that explains, at length and directly above the constant, why 3 s
was wrong. A stale number in exactly the place that warns against
tightening it is worse than no number.
- `MAX_SCHEMA` is [1, 24]; the comment above it still claimed 1.0-1.9.
Totals unchanged at 159/132/196 -- this commit only rewrites prose, and the
scorecard is re-recorded so its header matches the runner that produced it.
|
||
|
A.Shakhmatov
|
54ad124c18 |
tests/spec: a CSOT-rewritten maxTimeMS cannot be asserted
Every client entity is built with CSOT `timeoutMS` (OP_TIMEOUT_MS, 10 s), and CSOT overwrites `maxTimeMS` on each command with what is left of that budget. An expectation of `maxTimeMS: 6000` therefore meets the harness's 10000, and no amount of engine correctness would change it. Reported unsupported rather than failed: a FAIL is a claim about the engine, and this is a claim about the runner. Refused unconditionally when an expected command mentions `maxTimeMS`, not only when the two values differ, so it can never become a pass by coincidence. Exactly one case in the corpus asserts it -- estimatedDocumentCount.json, "estimatedDocumentCount with maxTimeMS" -- so the whole cost of the hatch is one case, which is why it is worth taking instead of dropping `timeoutMS`. That option is not open anyway: `timeoutMS` is what replaced the outer race that once turned ~190 good cases into phantom timeout FAILs. This is the only escape hatch in the runner. Everything else is either an honest FAIL or an enumerated unsupported feature. 159/133/195 becomes 159/132/196: one case, fail to skip, and nothing else moves. |
||
|
A.Shakhmatov
|
97e3e3a556 |
tests/spec: assert expectEvents
The headline is not the delta, it is that `pass` changed meaning. 354 of the
487 cases declare `expectEvents` and until now the runner read none of them,
so a case could send the wrong command entirely and still be counted a pass
as long as the *result* came back right. The old column was an upper bound by
construction. 193/99/195 becomes 159/133/195, and the two numbers are not
comparable.
Two rules decide how far the assertion reaches, both taken from the spec
rather than from what would be convenient:
- `command` and `reply` match as *root* documents
(unified-test-format.md:1020-1022, :1037-1039). The driver hangs `lsid`,
`$db` and `maxTimeMS` off nearly everything it sends; as nested documents
essentially the whole corpus would fail on keys no expectation was ever
written to mention, and the number would say nothing.
- the event list is exact in number and order, not a prefix
(unified-test-format.md:3088-3091). 23 cases expect an empty list and a
prefix rule would pass every one of them without looking.
The assertion runs after the operations, so a wrong result is still reported
as a wrong result rather than being masked, and after the listeners are
disabled, so the teardown's own commands cannot reach the buffer.
`cmap` and `sdam` event types, `ignoreExtraEvents`, and any event field beyond
`command`/`reply`/`commandName`/`databaseName` are reported unsupported at the
point of assertion. None occurs in this corpus -- all 354 blocks are
`eventType: command`, carrying 349 `commandStartedEvent` and 6
`commandSucceededEvent` -- so nothing is being quietly waived.
All 34 newly-failing cases, triaged. Not one is a wrong answer from the
engine; every one is a command the driver never sent:
- 22x `command.writeConcern: missing` -- runner gap, and the sharpest thing
this commit found. `buildEntities` drops `collectionOptions` on the floor,
so `writeConcern: {w: 0}` never reached the driver and every "unacknowledged
write" case in the corpus has been running an acknowledged write. They
passed because the results of the two agree. This is precisely the class of
error the instrument was built to find, and it was invisible to the result
column.
- 5x `command.sort.<key>: missing` -- runner gap. The driver holds a sort as
a JS `Map` (lib/sort.js), so `Object.keys` on it is empty and the matcher
reports every expected key as absent. Measured, not guessed: EJSON prints a
`Map` exactly like a document, which is why the dump looks correct.
- 4x `command.bypassDocumentValidation: missing` -- unclassified. The option
is absent from the wire for the `false` cases; the driver only forwards it
when true on some paths (lib/operations/find_and_modify.js:19), and whether
the runner also drops it has not been established.
- 2x `command.comment: missing` on getMore -- server gap, most likely. The
driver gates it on `maxWireVersion >= 9` (lib/operations/get_more.js:43)
and this engine advertises 8 while reporting itself as 4.4.0, which is
wire 9. The inconsistency is ours.
- 1x `command.maxTimeMS: expected 6000, got 10000` -- the CSOT rewrite, dealt
with in the next commit.
Each of those gets its own commit, and none of them is fixed here: a check and
the fix for what the check caught do not belong in one change.
|
||
|
A.Shakhmatov
|
6560aec915 |
tests/spec: buffer command-monitoring events per client entity
Plumbing only: a client entity that declares `observeEvents` now gets `monitorCommands` and a buffer, and nothing reads the buffer. That is the point of splitting it out -- the totals not moving *is* this commit's test. Command monitoring changes how the driver builds every command it sends, and if that alone shifted a result there would be no way to tell it apart from the assertions landing in the next commit. 193/99/195 before, 193/99/195 after, 175/175 files, 0 errored. The rules the buffer already enforces, so that the next commit is only about comparing: `ignoreCommandMonitoringEvents` by command name; sensitive commands dropped unless `observeSensitiveCommands` says otherwise, with `hello` and legacy hello inferred sensitive from the driver having redacted them to empty documents (unified-test-format.md:3070-3075). Neither fires on this corpus -- 136 client entities observe `commandStartedEvent`, 6 also `commandSucceededEvent`, and not one sets either field -- but a rule that only exists where it is exercised is a rule that will be missing when M7 brings auth. `cmap` and `sdam` observations are collected by nobody; a test that goes on to assert them is reported unsupported where it asserts, not where it declares. Two things about placement, both load-bearing. Listeners are attached after `connect()`, so a client's own handshake is not in its own buffer -- measured rather than assumed: with the buffers dumped, find.json's five cases show exactly `find`, `getMore`, `getMore` and nothing else. And they are disabled after the operations and before the outcome check (unified-test-format.md:3081), plus again unconditionally in the teardown `finally`, because the outcome check and the teardown both issue commands and a buffer still growing through them would make the assertion a function of the harness rather than of the engine. `MFDB_DUMP_EVENTS=1` prints each case's buffer. That is how the handshake question above was settled and how a failing event assertion will be triaged. |
||
|
A.Shakhmatov
|
afa5c6ef9d |
tests/spec: $$unsetOrMatches does not change root-ness
`special()` passed a hard `false` for `root` into its recursion, so a value
standing behind `$$unsetOrMatches` was matched as a nested document even when
it sat at the top of an `expectResult`. The spec says the opposite in so many
words -- "This operator does not influence whether or not an actual document
value is considered a root-level document" (unified-test-format.md:2873, and
:2821 for `$$matchesEntity`) -- and that distinction is the whole of the
extra-key rule: only a root document may carry keys the expectation does not
mention.
`root` now threads through `match` -> `special` -> the recursion. From under a
key it is always false, which is what it already was; from the top level it is
whatever the caller had.
25 cases go from FAIL to pass and none moves the other way. Every one is the
same shape -- an `expectResult` of `{$$unsetOrMatches: {acknowledged: false}}`
against a driver write result that also carries its counts, or the
`insertedId`/`insertedIds` forms of the same thing -- and every one was the
runner failing a result the engine had got right. 168/124/195 becomes
193/99/195; the scorecard is rewritten here so the delta belongs to this
change alone.
Mutation-checked: put the `false` back in the `$$unsetOrMatches` arm and
bulkWrite-deleteMany-hint-unacknowledged.json returns to 0 pass, 2 fail. The
`$$matchesEntity` arm is the same one-word change on the same sentence of the
spec, but the crud corpus does not use that operator once, so it rests on the
spec text rather than on a red test.
Two consecutive full runs, both 193/99/195, 175/175 files, 0 errored, no
lingering timers.
|
||
|
A.Shakhmatov
|
e66030f43e |
plan: the eight bugs cleared before the free list
Four of them were not in the plan that started this work -- they were found by tests written for the three that were, which is M0's gate lesson arriving a milestone early. The record has to match what happened, or the next session reads a milestone that looks like it went as designed. Also records what nobody is fixing yet: `rebuild_collection` frees pages under only the collection's lock while a concurrent checkpoint may have snapshotted a catalog that claims them, and the `seq` retry cannot see it because a rebuild appends no log record. Written down so the free list does not add a second instance of the same shape. |
||
|
A.Shakhmatov
|
332206e5dd |
db: the slab counts what the appender skips
`slab_used` only ever grew by a document's length, so the two places the appender writes slab off went uncounted: the gap left when a checkpoint freezes the page the cursor points into and the cursor resumes at the next system page, and the tail of an extent abandoned for a document that no longer fits. Both are real garbage -- only a rebuild gets them back -- and both were invisible to the trigger that decides whether a rebuild is worth doing. An abandoned tail can be most of 8 MiB. `note_skip` counts them into `slab_used` where they happen and hands the number back for the caller to charge to the engine, which keeps the identity the last commit established: `dead_bytes` is the sum of `slab_used - live_bytes` over the collections that exist. That identity is also why `compact` no longer zeroes `dead_bytes`. A repack appends through the same slab, so it abandons a tail of its own whenever the next document does not fit; zeroing was true only if a rebuild leaves nothing behind, and it does not. `sum_dead_bytes` recomputes it from the collections, each under its own lock. Carried with it, because this commit is what exposed it: the four engine counters get a lock of their own. They are the only engine-wide mutable state a writer touches while holding nothing but its own collection's lock, so two writers on different collections reach them with no lock in common -- and the checkpoint's consistency check read them ordered against nothing, while the per-collection figures it compares them to were read under each collection's lock. Before this commit `dead_bytes` moved on nearly every write, so that check was skipped almost every time; with skips counted it stands still between checkpoints, the check runs, and it aborted three of eight ReleaseSafe runs of "a checkpoint runs alongside writers on several collections". Not mutation-checked, and worth saying so: reverting the lock did not re-trigger the abort in 24 further runs, and neither did the exact pre-fix revision in 10. The rate depends on machine load, and a mutation check that cannot be relied on to go red is not a check. The lock stands on inspection instead -- an unsynchronized read-modify-write on a counter shared by threads holding no common lock is a defect whatever its rate -- and the concurrency test now asserts the identity once everything is quiet, which is the half of it that does not depend on a race being caught in the act. Mutation-checked, each red on its own: drop either `note_skip` call in `slab_reserve`; put `self.dead_bytes = 0;` back in `compact`. The `note_skip` in `slab_append` is covered by the concurrency test, the only place a publish lands between a reservation and its append. 165/165 unit tests in ReleaseFast and ReleaseSafe, 82/82 fuzz, e2e 49, e2e2 concurrent 2 + crash pair, e2e3 16, e2e4 17, e2e6 72, e2e7 86, crash-fuzz 60 cycles. |
||
| e15d7f2ed0 |
db/pager: the concurrency test writes the way the server does
"a checkpoint runs alongside writers on several collections" drove its writers through `Engine.lock()` -- the legacy whole-engine lock, which the server has not used since the locks were decomposed. That serialized the writers against each other, so the overlap the test is named for never happened: `write_catalog` takes each collection's lock shared, and nothing it was racing against took that lock at all. Drive them the way `commands.zig` dispatch does instead: catalog shared, then the target collection exclusive. The test then does what it says, and immediately found something -- two appenders on different collections calling `bytes_mut` at the same time corrupt the pager's `dirty` set, which is an unsynchronized hash map. ReleaseSafe aborts in `getOrPutContextAdapted`; three runs in five. `dirty` is test-only instrumentation (`track_dirty = builtin.is_test`), so this is a harness bug rather than a server one -- but it is the one shared structure on a write path whose writers are otherwise kept apart by owning different pages, and it needs a lock of its own. Six ReleaseSafe runs clean afterwards. 163/163 unit tests in ReleaseFast and ReleaseSafe, 82/82 fuzz. |
|||
| 992cc2a5ab |
db: a dropped collection is reclaimed, not deadened
`free_collection` charged the engine's `dead_bytes` with the dropped collection's live bytes, and then, three lines down, handed every page that collection owned back to the pager. A drop therefore asked for a rebuild -- a full copy of every collection that was left -- to reclaim space that had already been reclaimed. Its own garbage was wrong the other way: bytes that died before the drop stayed on the engine's books after the pages holding them were freed. Both halves of that are the same statement: `dead_bytes` is the sum of `slab_used - live_bytes` over the collections that still exist. Make it so on the drop path, then stop storing it separately at all -- `read_catalog` recomputes it from the collections the catalog lists, so the watermark's copy is now a hint for anything inspecting the header rather than a second source of truth. It would be wrong in one specific way if it stayed one: a collection dropped after the last checkpoint is gone from the catalog but still charged for in the hint. `write_catalog` now returns the dead sum beside the live one and the checkpoint asserts it the same way, under the same quiescence condition. That is what makes the accounting checkable rather than merely intended. Mutation-checked three ways, each red on its own: charge the drop again; delete the subtraction of the collection's own garbage; delete the accumulation in `read_catalog`. 163/163 unit tests in ReleaseFast and ReleaseSafe, 82/82 fuzz, e2e 49, e2e2 concurrent 2 + crash pair, e2e3 16, e2e4 17, e2e6 72, e2e7 86, crash-fuzz 60 cycles. |
|||
| f8a39a0965 |
db: the catalog snapshot is read under each collection's lock
`write_catalog` walks every collection's slab extents, byte counters and index metadata while holding only the *shared catalog* lock -- which is the same lock a writer holds, taking the collection's lock exclusively. So the snapshot read structures their owners were free to mutate underneath it. `slab_extents` makes it more than a torn read: it is an ArrayList that `slab_reserve` appends to, and an append that reallocates leaves the serializer walking freed memory. What it writes from that walk is the catalog the next open trusts to find every extent the collection owns. Now under each collection's lock, shared, taken inside the catalog lock -- the same order `compact` uses, so no new ordering to reason about. Not the commit that found the concurrency bugs above; those needed a checkpoint racing writers, which this lock is orthogonal to. It is the one that makes the snapshot legal rather than merely lucky. Verified: `zig build test` in ReleaseFast and ReleaseSafe. |
