M2: the aggregate command surface, and the refusals that had to come first #2
75
PLAN.md
75
PLAN.md
@@ -394,6 +394,69 @@ per-collection gate judges what reclamation left. Same six rounds, 256 MB.
|
||||
Numbers and reproduction in `tests/e2e/results/m0-gates.txt` under `[M1.1]` and
|
||||
`[M1.2]`.
|
||||
|
||||
### Amendment A6 — M2 splits in two, and its gate did not exist (amends the M2 row)
|
||||
|
||||
The M2 design review (`docs/M2_DESIGN_REVIEW.md`) found that the milestone's
|
||||
gate names a corpus that is not there. `mongodb/specifications` has no
|
||||
aggregation suite: the thirteen `aggregate-*.json` files this project runs live
|
||||
inside `crud` and test the aggregate *command* — cursor and `batchSize`,
|
||||
`readConcern` routing, the write semantics of `$out`/`$merge`, `collation`,
|
||||
`let`. They touch stages barely and expressions not at all; `$lookup`,
|
||||
`$unwind`, `$facet`, `$addFields` and `$replaceRoot` appear nowhere in the
|
||||
pinned corpus. A literal "green" is unreachable besides: all 19 skips are
|
||||
version- or topology-gated and none can pass on a standalone, so the gate is
|
||||
stated as *0 fail* rather than *all pass*.
|
||||
|
||||
So one milestone name was covering two milestones, and they are split.
|
||||
|
||||
**M2 is the command surface**, gated on the corpus that already exists: `$out`,
|
||||
`$merge`, `db.aggregate()`, collation, `let`. That is 8 of the 13 current
|
||||
failures in the write stages alone. It is small and fully measurable today.
|
||||
|
||||
**M2.5 is the engine**, gated on a corpus this project has to write, in the
|
||||
unified format the runner already reads, with every expectation measured
|
||||
against mongod 8.3.7 rather than recalled — the discipline that corrected three
|
||||
assumptions in M1's session work. The tiers, in dependency order: the
|
||||
expression evaluator, then a per-stage document iterator (the stream is a
|
||||
materialized window today, which cannot express 1→N), then the accumulators.
|
||||
`$unwind` is in the first cut because it falls out of the iterator for free.
|
||||
`$lookup` is out: a stage that takes a second collection's lock while the
|
||||
pipeline holds one is a lock-order question that should not ride along.
|
||||
`$facet` is out: sub-pipeline execution should be allowed to settle after the
|
||||
iterator exists.
|
||||
|
||||
**Sequencing: command surface first, engine second** — chosen deliberately, and
|
||||
with the cost stated rather than glossed. The engine is what gives wrong
|
||||
answers *now*: measured against a live server, `$avg`, `$max` and `$push` all
|
||||
return `0`, a compound `_id` collapses every document into one group keyed by
|
||||
the unevaluated expression, `$literal` is echoed back, and a `$match` after a
|
||||
`$project` still sees the projected-away field. Six of eight probed pipelines
|
||||
answer `ok: 1` with a wrong result; only `$addFields` fails honestly. Doing the
|
||||
command surface first leaves those alive for a further milestone.
|
||||
|
||||
*Measured after Tier 0 landed, and it corrects this amendment:* pricing the 13
|
||||
failures one by one shows **6 of them cannot turn green in M2 at all** — three
|
||||
need `$listLocalSessions` (M4) *and* `$addFields` (M2.5), two need the
|
||||
expression engine, one needs a collation. The gate reads "0 fail among the
|
||||
seven reachable", not "0 fail". And the seven that remain, `$out` and `$merge`,
|
||||
need a lock-model decision this amendment did not anticipate: they write to a
|
||||
collection the pipeline is not reading, which the dispatch contract, the static
|
||||
lock table and `Collection.lock`'s one-at-a-time invariant all stand against.
|
||||
It 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. Both options are set out in
|
||||
`docs/M2_DESIGN_REVIEW.md` §7; nothing past Tier 0 is implemented until one is
|
||||
chosen.
|
||||
|
||||
**Which is why M2 carries the refusals.** Every construct the engine does not
|
||||
implement stops answering `0` and starts answering an error, with the code
|
||||
measured against mongod: unknown accumulators, non-path expressions where a
|
||||
path is expected, `$project` anywhere but last. This is the same move
|
||||
`expectEvents` made before the free list in M1 — it will push the scorecard
|
||||
*down*, and that is the point. An unrecognised stage is a bug report; an `$avg`
|
||||
that returns `0` is a corrupted report nobody files. Without this the sequencing
|
||||
above would be trading a measurable milestone for a year of silent wrong
|
||||
answers, and the trade is only acceptable because the lie is removed first.
|
||||
|
||||
---
|
||||
|
||||
## 3. Milestones and gates
|
||||
@@ -402,7 +465,8 @@ Numbers and reproduction in `tests/e2e/results/m0-gates.txt` under `[M1.1]` and
|
||||
|---|---|---|---|
|
||||
| M0 | **mmap + WAL foundation** | data file format, page/extent allocator, mmap slab + B+tree arena, copy-on-write + page free list (A1/A2), watermark/replay, checkpoint (= compaction repurposed), leaf payload → slab offset (A3), drop docs hashmap, churn measurement | D7 (6 items) |
|
||||
| M1 | **Cursors + wire polish** | getMore / killCursors / batchSize; server-side cursor state with idle timeout; sessions plumbing (lsid accepted) as drivers send it; hello advertisement updates; **`moreToCome` on requests** (see the bug below); command-monitoring assertions in the spec runner | crud spec suite green; e2e green |
|
||||
| M2 | **Aggregation expansion** | pipeline stages and expression engine (tiered scope defined at M2 design review) | aggregate spec suite green |
|
||||
| M2 | **The `aggregate` command surface** | `$out` and `$merge` (7 of the 13 failures), and refusing every pipeline construct the engine does not implement instead of answering `0` (amendment A6). The other 6 failures are blocked on M2.5, M4 and M8 — see `docs/M2_DESIGN_REVIEW.md` §7 | `aggregate-*.json`: 0 fail among the 7 reachable cases |
|
||||
| M2.5 | **The aggregation engine** | expression evaluator, per-stage document iterator, the accumulators, `$unwind`; `$lookup`/`$facet` explicitly out of the first cut (amendment A6) | a purpose-built stage corpus, every expectation measured against mongod |
|
||||
| M3 | **Update operators + index types** | $setOnInsert, $addToSet, $mul, $min/$max, $pop, $pullAll, $currentDate, pipeline updates; partial + hashed indexes | remaining crud coverage; e2e3/e2e4 green |
|
||||
| M4 | **Sessions + transactions** | logical sessions, snapshot isolation on the mmap engine, write concern at commit | sessions + transactions spec suites green |
|
||||
| M5 | **Change streams** | change feed + resume tokens (likely log-seq based), getMore integration | change-streams spec suite green |
|
||||
@@ -969,8 +1033,13 @@ has to be its own commit with its own re-recorded scorecard.
|
||||
taking a `session` argument, and no `lsid` assertion. The value here is
|
||||
protocol hygiene, not a number — and after Stage 1 a wrongly-refused command
|
||||
would have shown up as a changed event stream rather than silently.
|
||||
- **M2 aggregation**: stage/expression tiers, which spec-test files are
|
||||
the gate, whether $lookup/$unwind/facet make the first cut.
|
||||
- **M2 aggregation** — *settled, see amendment A6 and `docs/M2_DESIGN_REVIEW.md`.*
|
||||
The three questions this entry held are answered: the tiers, the gate (the
|
||||
named one does not exist), and the first cut ($unwind in, $lookup and $facet
|
||||
out). What the review did *not* settle and the engine milestone must:
|
||||
`$out`/`$merge` durability semantics, whether the expression evaluator is
|
||||
shared with M3's pipeline updates, and whether `allowDiskUse` has to stop
|
||||
being a lie.
|
||||
- **M4 transactions**: snapshot isolation over mmap (COW vs undo), read
|
||||
concern snapshot, conflict → TransientTransactionError semantics,
|
||||
retryable-writes interplay.
|
||||
|
||||
359
docs/M2_DESIGN_REVIEW.md
Normal file
359
docs/M2_DESIGN_REVIEW.md
Normal file
@@ -0,0 +1,359 @@
|
||||
# M2 design review — aggregation expansion
|
||||
|
||||
PLAN §3 gives M2 one line of scope ("pipeline stages and expression engine,
|
||||
tiered scope defined at M2 design review") and one line of gate ("aggregate
|
||||
spec suite green"), and §6 lists the three questions it deferred: the
|
||||
stage/expression tiers, which spec files are the gate, and whether
|
||||
`$lookup`/`$unwind`/`$facet` make the first cut.
|
||||
|
||||
This document answers them, and reports one finding that has to be settled
|
||||
before any of the rest is worth discussing.
|
||||
|
||||
Everything below is measured against the M1 tip (`a748a3d`) unless it says
|
||||
otherwise. Nothing here is recalled.
|
||||
|
||||
---
|
||||
|
||||
## 1. The gate named in the plan does not exist
|
||||
|
||||
`mongodb/specifications` has no aggregation suite. The full list of `source/`
|
||||
directories at the pinned commit is: `auth`, `bson-*`, `causal-consistency`,
|
||||
`change-streams`, `client-side-encryption`, `collation`,
|
||||
`collection-management`, `command-logging-and-monitoring`, `compression`,
|
||||
`connection-*`, `crud`, `gridfs`, `index-management`, `load-balancers`,
|
||||
`logging`, `read-write-concern`, `retryable-*`, `run-command`, `sessions`,
|
||||
`server-discovery-and-monitoring`, `server-selection`, `transactions`,
|
||||
`unified-test-format`, `versioned-api`, and a handful of others. There is no
|
||||
`aggregation`.
|
||||
|
||||
The thirteen `aggregate-*.json` files we do run live inside `crud`, and they
|
||||
test the aggregate **command surface** — cursor and `batchSize`, `readConcern`
|
||||
and `readPreference` routing, the write semantics of `$out`/`$merge`,
|
||||
`collation`, `let`, `allowDiskUse`, `rawData`. They barely touch stages, and
|
||||
they touch expressions not at all: `$lookup`, `$unwind`, `$facet`,
|
||||
`$addFields`, `$replaceRoot` do not appear anywhere in the pinned corpus.
|
||||
|
||||
So "aggregate spec suite green" is not a gate for the milestone the scope line
|
||||
describes. It is a gate for a much smaller milestone that happens to share the
|
||||
command name. **Whichever option §4 picks, PLAN's M2 gate line has to be
|
||||
rewritten**, and that is the first decision this review asks for.
|
||||
|
||||
Where the current corpus stands, for reference:
|
||||
|
||||
```
|
||||
aggregate-*.json overall 9 pass 13 fail 19 skip
|
||||
```
|
||||
|
||||
| failure | cases |
|
||||
|---|---|
|
||||
| `$merge` unimplemented | 5 |
|
||||
| `$out` unimplemented | 3 |
|
||||
| `db.aggregate()` — `{aggregate: 1}`, no collection name | 3 |
|
||||
| `collation` in aggregate | 1 |
|
||||
| `let` | 1 |
|
||||
|
||||
All 19 skips are environmental -- 10 distinct requirements, some covering a
|
||||
whole file at once (`needs topology replicaset`, `needs server >= 5.0`, `needs
|
||||
server >= 8.2.0`, `needs server <= 4.2.99`). Not one can turn green on a
|
||||
standalone, so a literal reading of "green" is unreachable for that reason
|
||||
alone, and the gate has to be stated as *0 fail* rather than *all pass*.
|
||||
|
||||
---
|
||||
|
||||
## 2. Where the implementation actually is
|
||||
|
||||
**Seven stages**, all in one `if/else if` chain in `cmd_aggregate`:
|
||||
`$match`, `$sort`, `$skip`, `$limit`, `$project`, `$group`, `$count`.
|
||||
Anything else answers `Location40324`, with the code verified against a real
|
||||
mongod.
|
||||
|
||||
(`$set` and `$unset` exist in the tree as **update operators** in
|
||||
`src/update.zig`. They are not pipeline stages. An earlier count of nine
|
||||
stages in this project's notes was wrong.)
|
||||
|
||||
**There is no expression engine.** `$field` path resolution is open-coded in
|
||||
two places inside `run_group`, and nowhere else. `$sum` is the only
|
||||
accumulator.
|
||||
|
||||
That last sentence is the important one, because of *how* the missing pieces
|
||||
fail. Probed against a live server on three documents
|
||||
(`{g:'a',x:10}, {g:'a',x:20}, {g:'b',x:30}`):
|
||||
|
||||
| pipeline | answer | correct answer |
|
||||
|---|---|---|
|
||||
| `$group: {_id:'$g', s:{$sum:'$x'}}` | `[{a,30},{b,30}]` | ✅ |
|
||||
| `$group: {_id:'$g', a:{$avg:'$x'}}` | `[{a,0},{b,0}]` | `[{a,15},{b,30}]` |
|
||||
| `$group: {_id:'$g', m:{$max:'$x'}}` | `[{a,0},{b,0}]` | `[{a,20},{b,30}]` |
|
||||
| `$group: {_id:'$g', p:{$push:'$x'}}` | `[{a,0},{b,0}]` | arrays |
|
||||
| `$group: {_id:{g:'$g'}, n:{$sum:1}}` | one group, `_id` = `{"g":"$g"}` | two groups |
|
||||
| `$project:{g:1}` then `$match:{x:10}` | matches | matches nothing |
|
||||
| `$group: {_id:{$literal:1}, ...}` | `_id` = `{"$literal":1}` | `_id` = `1` |
|
||||
| `$addFields` | `Location40324` | (unimplemented) |
|
||||
|
||||
**Six of those eight are silent wrong answers with `ok: 1`.** Only
|
||||
`$addFields` fails honestly. That is the shape of the risk in this area and it
|
||||
should drive the milestone: an unrecognised stage is a bug report, an `$avg`
|
||||
that returns `0` is a corrupted report that nobody files. It is also precisely
|
||||
the class the M1 spec-runner work was about — and the corpus cannot see any of
|
||||
it, because the corpus has no stage tests.
|
||||
|
||||
**The pipeline model will not carry the milestone as it stands.** The stream is
|
||||
a materialized window `[start, end)` over either slab offsets or generated
|
||||
documents, and each stage moves the bounds. That works for the filtering and
|
||||
reordering stages and nothing else:
|
||||
|
||||
- `$project` is not a stage transform at all. It sets a variable that is
|
||||
applied once, at emit — so only the last `$project` in a pipeline has any
|
||||
effect, and a `$match` after a `$project` sees the unprojected document.
|
||||
- `$unwind` is 1→N. There is no way to express that in a window over the input.
|
||||
- `$lookup` needs a second collection under a second lock, inside a stage.
|
||||
- `$facet` needs the same stream fanned into several independent sub-pipelines.
|
||||
- `allowDiskUse` is accepted and ignored. `$group` and `$sort` are unbounded in
|
||||
memory; the three passing `allowdiskuse` cases pass because the option is
|
||||
parsed, not because anything spills. `src/spill.zig` exists and is used by
|
||||
index builds, not here.
|
||||
|
||||
So M2 is not "add stages to the chain". It is: build an expression evaluator,
|
||||
change the stream model to a per-stage document iterator, and then stages
|
||||
become cheap. Anything that adds stages before those two exist adds more
|
||||
silent wrong answers.
|
||||
|
||||
---
|
||||
|
||||
## 3. What that implies for tiers
|
||||
|
||||
The dependency order is forced, not chosen:
|
||||
|
||||
**Tier 0 — stop lying (no new features).** Refuse what is not implemented
|
||||
instead of answering `0`: unknown accumulators, non-path expressions where a
|
||||
path is expected, `$project` used anywhere but last. Every one of these becomes
|
||||
an error with a measured mongod code. This is small, and it converts six silent
|
||||
wrong answers into six honest ones. It is also the only tier that makes the
|
||||
later ones measurable, for the same reason Stage 1 preceded Stage 3 in M1.
|
||||
|
||||
**Tier 1 — the expression engine.** A real evaluator over `bson.Value`:
|
||||
field paths, `$literal`, comparison and boolean operators, arithmetic,
|
||||
`$cond`/`$ifNull`/`$switch`, string and date operators. Everything above
|
||||
depends on it, including `$project`'s computed fields and `$group`'s `_id`.
|
||||
|
||||
**Tier 2 — the stage iterator.** Replace the window with a per-stage pull
|
||||
model, so a stage can emit more or fewer documents than it consumed. `$project`
|
||||
becomes a real transform. Carries `$addFields`/`$set`, `$unset`,
|
||||
`$replaceRoot`, `$unwind`, `$sortByCount`.
|
||||
|
||||
**Tier 3 — the accumulators.** `$avg`, `$min`, `$max`, `$first`, `$last`,
|
||||
`$push`, `$addToSet`, `$count`, `$mergeObjects`. Cheap once Tier 1 exists.
|
||||
|
||||
**Tier 4 — the expensive stages.** `$lookup` (second collection, lock order),
|
||||
`$facet` (sub-pipelines), `$unionWith`, `$graphLookup`. Each is a design
|
||||
question of its own.
|
||||
|
||||
**Tier 5 — the write stages.** `$out`, `$merge`. These are not aggregation
|
||||
work at all — they are a write path that happens to be spelled as a stage, with
|
||||
their own durability and atomicity questions. They are also 8 of the 13 current
|
||||
`aggregate-*` failures.
|
||||
|
||||
**Orthogonal, and cheap:** `db.aggregate()` (`{aggregate: 1}`), 3 failures.
|
||||
**Orthogonal, and not cheap:** `allowDiskUse` meaning something, which is a
|
||||
memory-bound question for `$group`/`$sort` and belongs with whichever tier
|
||||
first makes those able to hold a lot.
|
||||
|
||||
---
|
||||
|
||||
## 4. Gate options
|
||||
|
||||
Each option is stated with what it would actually prove and what it costs.
|
||||
|
||||
### Option A — "the aggregate command surface"
|
||||
|
||||
Gate: the 13 `aggregate-*.json` files in the crud corpus, minus the 13
|
||||
environmental skips. Scope: Tier 5 + `db.aggregate()` + collation + `let`.
|
||||
|
||||
- **Proves:** the aggregate *command* behaves — cursors, read concern, write
|
||||
stages, database-level form.
|
||||
- **Does not prove:** any stage or expression is correct. All six silent wrong
|
||||
answers survive this gate untouched.
|
||||
- **Cost:** small. Two write stages and three small fixes.
|
||||
- **Honest name:** this is not "aggregation expansion". If chosen, M2 should be
|
||||
renamed and the aggregation engine deferred to its own milestone.
|
||||
|
||||
### Option B — a hand-built stage corpus, modelled on mongod's own
|
||||
|
||||
Gate: a new `tests/spec/aggregate/` corpus in the unified format the runner
|
||||
already reads, with cases ported from `mongod`'s `jstests/aggregation` and
|
||||
every expected result **measured against mongod 8.3.7**, the way the cursor and
|
||||
`lsid` error codes were.
|
||||
|
||||
- **Proves:** exactly what the milestone claims to build, at whatever tier
|
||||
depth the corpus is written to.
|
||||
- **Cost:** the corpus is the milestone's second deliverable and a real one —
|
||||
but the runner, the entity model, the matchers and `expectEvents` all exist
|
||||
already, so the marginal cost is writing cases, not machinery.
|
||||
- **Risk, stated plainly:** a corpus we author is a corpus we can accidentally
|
||||
write to match our own bugs. The mitigation is the one this project already
|
||||
uses: every expectation measured against a real mongod before it is
|
||||
committed, never recalled.
|
||||
|
||||
### Option C — differential against mongod
|
||||
|
||||
Gate: a harness that runs the same pipelines against MultiforaDB and mongod
|
||||
8.3.7 and diffs the results, over generated pipelines and a fixed corpus.
|
||||
|
||||
- **Proves:** the most, by a distance, and it finds the silent-wrong-answer
|
||||
class by construction rather than by someone thinking to test for it.
|
||||
- **Cost:** mongod becomes a test dependency (it already is one for
|
||||
measurement, but not for the gate). Result ordering and floating-point
|
||||
formatting need normalising, and `$group` output order is genuinely
|
||||
unspecified.
|
||||
- **Note:** this is a *harness*, not a corpus, so it composes with B rather
|
||||
than competing with it — B gives regression cases with committed
|
||||
expectations, C finds new ones.
|
||||
|
||||
### Option D — keep the plan's line and reinterpret "green"
|
||||
|
||||
Gate: `aggregate-*.json` at 22 pass / 0 fail / 13 skip, and declare the skips
|
||||
out of scope.
|
||||
|
||||
- **Proves:** the same as A.
|
||||
- **Why it is listed:** only to be rejected explicitly. It reads as if the
|
||||
aggregation milestone were gated on aggregation tests, and it is not. If A is
|
||||
the decision, say A.
|
||||
|
||||
---
|
||||
|
||||
## 5. Decision
|
||||
|
||||
**Taken: the command surface first (Option A as M2), the engine second (M2.5),
|
||||
with Tier 0 folded into M2.** Recorded as PLAN amendment A6; the §3 rows are
|
||||
rewritten and §6's deferred entry now points here.
|
||||
|
||||
The cost of that order is on the record: the engine is what gives wrong answers
|
||||
today, and this leaves them alive for a further milestone. Tier 0 is what makes
|
||||
the trade acceptable rather than merely convenient — every unimplemented
|
||||
construct stops answering `0` and starts answering an error, so the wrong
|
||||
answers become visible bug reports instead of quiet ones while the command
|
||||
surface is built. It is not optional under this sequencing.
|
||||
|
||||
The recommendation the review arrived at independently is kept below, since a
|
||||
decision is easier to revisit when the argument it overrode is still legible.
|
||||
|
||||
## 5.1 Recommendation as written before the decision
|
||||
|
||||
**Tier 0 first, alone, before any scoping decision is final** — the same
|
||||
argument that put `expectEvents` before the free list in M1. Six silent wrong
|
||||
answers are six results nobody can measure past, and turning them into errors
|
||||
is a day's work that changes what every later number means. It will move the
|
||||
scorecard *down*, and that is the point.
|
||||
|
||||
**Then B as the gate, with C as the harness that feeds it.** Option A is a real
|
||||
piece of work and should happen — the `$merge`/`$out` failures are 8 of 13 —
|
||||
but it should be named for what it is and probably belongs in M6 (admin/ops) or
|
||||
its own small milestone, not as the gate for an engine.
|
||||
|
||||
**Tiers 1→3 as M2 proper**, with Tier 4 explicitly out of the first cut. That
|
||||
answers §6's third question: **`$unwind` yes** (it falls out of Tier 2 for
|
||||
free), **`$lookup` and `$facet` no** — `$lookup` because a stage that takes a
|
||||
second collection lock while the pipeline holds one is a lock-order design
|
||||
question this milestone should not be carrying, and `$facet` because it needs
|
||||
sub-pipeline execution that Tier 2's iterator should be allowed to settle
|
||||
first.
|
||||
|
||||
**One thing to decide that this review cannot decide alone:** whether
|
||||
`allowDiskUse` must mean something by the end of M2. It is currently accepted
|
||||
and ignored, which is the same category of lie as `$avg` returning `0` — a
|
||||
client that passes it believes it is protected. If the answer is yes, the
|
||||
memory bound on `$group`/`$sort` joins Tier 2 rather than trailing it.
|
||||
|
||||
---
|
||||
|
||||
## 6. What this review does not cover
|
||||
|
||||
- `$out`/`$merge` durability semantics (atomic swap? logged? crash behaviour?).
|
||||
Needed before Tier 5 is scoped.
|
||||
- Whether the expression engine is shared with `update`'s pipeline-update form,
|
||||
which M3 will need. Probably yes; it changes where the code lives.
|
||||
- Index use inside a pipeline beyond the existing leading-`$match` pushdown.
|
||||
|
||||
---
|
||||
|
||||
## 7. Post-decision measurement: the gate is not reachable either
|
||||
|
||||
Written after Tier 0 landed, when the 13 failures were priced one by one
|
||||
instead of counted. Section 1 established that the *corpus* named in the plan
|
||||
was the wrong instrument; this establishes that the *target* set on the
|
||||
corpus we kept is unreachable too. Both were found by looking rather than by
|
||||
assuming, and this one only by opening each case.
|
||||
|
||||
| cases | what they need | milestone |
|
||||
|---|---|---|
|
||||
| 5 `aggregate-merge.json` | `$merge` with a plain `into` | M2, blocked below |
|
||||
| 2 `aggregate-out.json` | `$out` with a plain target | M2, blocked below |
|
||||
| 3 `db-aggregate*.json` | `$listLocalSessions` **and** `$addFields` | M4 + M2.5 |
|
||||
| 2 `aggregate-let.json` | `let`, `$$var`, `$expr` | M2.5 |
|
||||
| 1 `aggregate-collation.json` | a case-insensitive collation | M8 |
|
||||
|
||||
So **6 of 13 can never turn green in M2**, whatever is built, and the gate has
|
||||
to read *0 fail among the seven this milestone can reach*, with the other six
|
||||
named and attributed. Note what the three `db.aggregate()` cases really need:
|
||||
implementing `{aggregate: 1}` moves none of them, because each then fails on
|
||||
`$listLocalSessions` instead. The design review priced that line "orthogonal,
|
||||
and cheap"; it was cheap and it was worth nothing.
|
||||
|
||||
### And the seven need a decision this review has no authority to take
|
||||
|
||||
`$out` and `$merge` write to a collection the pipeline is not reading. Three
|
||||
things in the current design stand against that, and none of them is a detail:
|
||||
|
||||
- `aggregate` is declared `.kind = .read`, and the dispatch contract is
|
||||
explicit that "only `.write` commands may call engine mutation functions".
|
||||
- dispatch acquires locks from a **static table keyed on the command name**,
|
||||
before the handler runs, for the one collection named by the command.
|
||||
- `Collection.lock`'s own comment states the invariant that decides this:
|
||||
*never more than one collection lock at a time*. Taking the target's
|
||||
exclusive lock while holding the source's shared one breaks it directly.
|
||||
|
||||
This is the same shape as the objection that kept `$lookup` out of M2.5's first
|
||||
cut — "a stage that takes a second collection's lock while the pipeline holds
|
||||
one is a lock-order question that should not ride along" — and the review
|
||||
should have applied it to the write stages in the same breath. It did not,
|
||||
which is the review's own error and is recorded here rather than quietly
|
||||
corrected.
|
||||
|
||||
Two ways out, both real:
|
||||
|
||||
**(a) A pipeline that ends in a write stage is a write command.** Compute the
|
||||
effective lock shape from the pipeline rather than the name: catalog exclusive,
|
||||
no collection lock from dispatch, and the handler takes source-shared →
|
||||
release → target-exclusive in that order. Heavier than mongod, and it
|
||||
serializes such an aggregate against the whole catalog, but it keeps every
|
||||
existing invariant intact and is honest about what the command is.
|
||||
|
||||
**(b) The write stages run in the dispatch epilogue,** after every lock is
|
||||
released, next to the commit and the checkpoint that already live there. The
|
||||
handler stashes what to write. Cheaper and better-behaved under concurrency,
|
||||
but it amends the `.read`/`.write` contract, so the contract's comment has to
|
||||
change with it and say why.
|
||||
|
||||
Either way the durability question is open and belongs with whichever is
|
||||
chosen: mongod's `$out` replaces the target collection *atomically*, and this
|
||||
engine has no cross-collection atomicity. A pipeline that fails after writing
|
||||
half its output must not leave the target half-replaced.
|
||||
|
||||
**Decision: (b), the epilogue.** Taken after the options were set out. The
|
||||
handler computes the output under the locks it already holds and leaves it in
|
||||
`Context.pending_write`; the epilogue applies it once every lock is released,
|
||||
beside the commit and the checkpoint that already live there. The
|
||||
`.read`/`.write` contract's comment was amended to say so and why.
|
||||
|
||||
Measured after: `aggregate-*.json` went 9 pass / 13 fail to 16 pass / 6 fail,
|
||||
and the whole corpus 194/97/196 to 201/90/196. The seven that moved are exactly
|
||||
the seven priced as reachable above, and the six that remain are exactly the
|
||||
six attributed to M2.5, M4 and M8.
|
||||
|
||||
**The durability question is answered honestly rather than solved.** `$out` is
|
||||
*not* atomic here: it drops the target and inserts, so a crash in between
|
||||
leaves part of the new output where mongod would leave the whole of the old.
|
||||
The shape that fixes it is write-to-temp-and-rename, and this server has no
|
||||
rename command. Recorded in `apply_pending_write`'s own comment as well, since
|
||||
that is where somebody will be standing when it matters.
|
||||
|
||||
945
src/commands.zig
945
src/commands.zig
File diff suppressed because it is too large
Load Diff
@@ -18,16 +18,16 @@
|
||||
# hasServerConnectionId, and maxTimeMS in an expected command (CSOT rewrites
|
||||
# it -- the only assertion this runner declines to make).
|
||||
|
||||
total 194 pass 97 fail 196 skip 175 files 0 errored
|
||||
total 201 pass 90 fail 196 skip 175 files 0 errored
|
||||
|
||||
# per-file: name pass fail skip
|
||||
aggregate-allowdiskuse.json 3 0 0
|
||||
aggregate-collation.json 0 1 0
|
||||
aggregate-let.json 0 2 2
|
||||
aggregate-merge-errorResponse.json 0 0 1
|
||||
aggregate-merge.json 0 5 0
|
||||
aggregate-merge.json 5 0 0
|
||||
aggregate-out-readConcern.json 0 0 4
|
||||
aggregate-out.json 0 2 0
|
||||
aggregate-out.json 2 0 0
|
||||
aggregate-rawdata.json 1 0 1
|
||||
aggregate-write-readPreference.json 0 0 4
|
||||
aggregate.json 5 0 2
|
||||
@@ -202,16 +202,9 @@ aggregate-collation.json FAIL Aggregate with collation aggregate: expected 1 ele
|
||||
aggregate-let.json SKIP Aggregate with let option needs server >= 5.0
|
||||
aggregate-let.json FAIL Aggregate with let option unsupported (server-side error) aggregate: expected an error, the operation succeeded
|
||||
aggregate-let.json SKIP Aggregate to collection with let option needs server >= 5.0
|
||||
aggregate-let.json FAIL Aggregate to collection with let option unsupported (server-side error) aggregate: error message "Unrecognized pipeline stage name: '$out'" does not contain "unrecognized field 'let'"
|
||||
aggregate-let.json FAIL Aggregate to collection with let option unsupported (server-side error) aggregate: expected an error, the operation succeeded
|
||||
aggregate-merge-errorResponse.json SKIP aggregate $merge DuplicateKey error is accessible needs server >= 5.1
|
||||
aggregate-merge.json FAIL Aggregate with $merge MongoServerError: Unrecognized pipeline stage name: '$merge'
|
||||
aggregate-merge.json FAIL Aggregate with $merge and batch size of 0 MongoServerError: Unrecognized pipeline stage name: '$merge'
|
||||
aggregate-merge.json FAIL Aggregate with $merge and majority readConcern MongoServerError: Unrecognized pipeline stage name: '$merge'
|
||||
aggregate-merge.json FAIL Aggregate with $merge and local readConcern MongoServerError: Unrecognized pipeline stage name: '$merge'
|
||||
aggregate-merge.json FAIL Aggregate with $merge and available readConcern MongoServerError: Unrecognized pipeline stage name: '$merge'
|
||||
aggregate-out-readConcern.json SKIP * needs topology replicaset/sharded
|
||||
aggregate-out.json FAIL Aggregate with $out MongoServerError: Unrecognized pipeline stage name: '$out'
|
||||
aggregate-out.json FAIL Aggregate with $out and batch size of 0 MongoServerError: Unrecognized pipeline stage name: '$out'
|
||||
aggregate-rawdata.json SKIP Aggregate with rawData option needs server >= 8.2.0
|
||||
aggregate-write-readPreference.json SKIP * needs topology replicaset/sharded/load-balanced
|
||||
aggregate.json SKIP aggregate with a document comment - pre 4.4 needs server <= 4.2.99
|
||||
@@ -261,7 +254,7 @@ bulkWrite-updateOne-pipeline.json FAIL UpdateOne in bulk write using pipelines M
|
||||
bulkWrite-updateOne-rawdata.json SKIP BulkWrite updateOne with rawData option needs server >= 8.2.0
|
||||
bulkWrite-updateOne-rawdata.json FAIL BulkWrite updateOne with rawData option on less than 8.2.0 - ignore argument MongoBulkWriteError: update spec requires u
|
||||
bulkWrite-updateOne-sort.json SKIP BulkWrite updateOne with sort option needs server >= 8.0
|
||||
bypassDocumentValidation.json FAIL Aggregate with $out passes bypassDocumentValidation: false MongoServerError: Unrecognized pipeline stage name: '$out'
|
||||
bypassDocumentValidation.json FAIL Aggregate with $out passes bypassDocumentValidation: false events client0[0].command.bypassDocumentValidation: missing from actual
|
||||
bypassDocumentValidation.json FAIL BulkWrite passes bypassDocumentValidation: false events client0[0].command.bypassDocumentValidation: missing from actual
|
||||
bypassDocumentValidation.json FAIL FindOneAndReplace passes bypassDocumentValidation: false events client0[0].command.bypassDocumentValidation: missing from actual
|
||||
bypassDocumentValidation.json FAIL FindOneAndUpdate passes bypassDocumentValidation: false events client0[0].command.bypassDocumentValidation: missing from actual
|
||||
|
||||
Reference in New Issue
Block a user