From 559b30df1031eeee2de6c9d8f03612eef2385093 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Thu, 9 Jul 2026 15:42:19 -0700 Subject: [PATCH 1/2] docs: capture the key-value storage contract and the index smell test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The extensions section of CLAUDE.md already says store interfaces must be designed for the technology space (per-key reads, no server-side filters), but review of a recent storage change showed the rule is not stated in the form that catches violations mechanically: a ListByBatch(batchID) method plus a new KEY idx_batch slipped through even though a plain key-value backend cannot satisfy it without a secondary index. The missing pieces were the concrete smell test and the prescribed alternative. ### What? CLAUDE.md's over-constraints list gains a query-by-attribute / secondary-index bullet: a schema diff that adds a `KEY idx_*` to make a store method viable means the contract has left get/put-by-key territory. The storage extension README gains a "Key-value contract" section spelling out the rules where store PRs are written: stores expose get/put/conditional-update by primary key only; the derived-key pattern (encode the relationship in a deterministic primary key like `{parentID}/{hash(child identity)}`, giving idempotent creation and at-most-one-row-per-identity by construction) replaces query-by-attribute; and domain state is often already the index — an aggregate that references its parts by ID enumerates their keys for free, so a database index duplicating it is a second source of truth. ## Issue Documentation only; based on main so it can merge independently of the speculation integration stack. --- CLAUDE.md | 1 + submitqueue/extension/storage/README.md | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 38f00189..3f4245ab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -141,6 +141,7 @@ Rule of thumb: if you're about to add a `NewFactory()` or a `map[queue]impl` und Common over-constraints to avoid: - **Batch atomicity** (multi-row inserts as one transaction) — many KV stores can't do this. Prefer single-record primitives + caller loops + idempotency-on-retry. - **Multi-key queries** (`WHERE x IN (...)`) — fine in SQL, awkward elsewhere. Prefer per-key reads. +- **Query-by-attribute / secondary indexes** (`WHERE attr = ?`, `ListByX(attr)`) — a plain KV store cannot look up by anything but the primary key. The mechanical smell test: **if a schema change adds a `KEY idx_*` to make a store method viable, the contract has stopped being get/put-by-key.** Instead, derive the primary key from the composite identity the caller already holds (e.g. `{parentID}/{hash(child identity)}`), and remember that domain state is often already the index — an entity that references its children (a tree listing its paths) enumerates their keys for free. See [submitqueue/extension/storage/README.md](submitqueue/extension/storage/README.md#key-value-contract). - **Server-side filters** (joins, sub-queries, complex predicates) — push filtering and aggregation to the caller; keep the store responsible only for "get/put by key" semantics. - **Transactions across entities** — virtually no distributed store offers this. Use eventual consistency + idempotency. - **Strict ordering / exactly-once** in messaging — most queues are at-least-once with best-effort ordering. Make consumers idempotent. diff --git a/submitqueue/extension/storage/README.md b/submitqueue/extension/storage/README.md index fe53f2c6..38397938 100644 --- a/submitqueue/extension/storage/README.md +++ b/submitqueue/extension/storage/README.md @@ -25,3 +25,13 @@ entity.Version = newVersion // only after the write succeeded ``` The post-success assignment matters whenever the entity is read again later in the same flow. Pre-incrementing in memory before the call is a bug pattern: if the call fails and the caller swallows the error, the in-memory version is now ahead of the database and subsequent updates will fail with `ErrVersionMismatch` for non-obvious reasons. + +## Key-value contract + +Store interfaces are designed for the storage technology *space*, not for SQL (see the Extensions section of the repo `CLAUDE.md`): every method must be satisfiable by a plain key-value backend (DynamoDB, Bigtable, an in-memory map) as cheaply as by MySQL. Concretely, a store exposes only get/put/conditional-update **by primary key**. No lookups by other attributes, no listings filtered server-side, no joins. + +**The smell test is the index.** If implementing a proposed store method in MySQL requires adding a secondary index (`KEY idx_*`) to the schema, the method is a query-by-attribute in disguise and the contract has left the key-value space — a KV backend would need a global secondary index or a hand-maintained index table to fake it. Treat a new `KEY` line in a schema diff as a design review flag, not a tuning detail. + +**Reach for the derived-key pattern instead.** When callers need "all X belonging to Y", encode the relationship in the primary key rather than querying for it: derive the key deterministically from the composite identity the caller already holds — for example `{parentID}/{hash(child identity)}`. Every caller that wants the children can recompute the keys and issue per-key reads; creation under a deterministic key is naturally idempotent (a redelivery finds the existing row); and "at most one row per identity" holds by construction instead of by query discipline. + +**Domain state is often already the index.** Before adding any lookup, check whether an entity the caller already loads enumerates the children — an aggregate that references its parts by ID (e.g. a tree whose paths record their build identities) is the batch→children index, persisted and versioned as domain state. Duplicating that relationship as a database index adds a second source of truth for something the domain already owns. From 10d1eb329e4bc8eb42ad5ce1c615a591b5259ba3 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Thu, 9 Jul 2026 16:01:20 -0700 Subject: [PATCH 2/2] docs(storage): add decision path and sanction reverse-lookup mapping stores MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The key-value contract section prescribed two avoidance patterns (derived keys, domain-state-as-index) but was silent on the case where neither applies — a true reverse lookup where the caller arrives holding only the attribute. It also mentioned the hand-maintained index table only negatively ("to fake it"), leaving no sanctioned path and inviting either sneaked-back `KEY idx_*` lines or contorted keys/aggregates. Adds the third path as first-class: in the KV space the only way to serve a reverse lookup is to make the attribute a primary key somewhere, so promote the relationship to an explicit mapping store (idempotent puts, eventually consistent, rebuildable as a projection — `ChangeStore`/`ChangeRecord` is the in-repo example). Adds an ordered decision path (derive → enumerate → map) with the escalation trigger for contended/unbounded aggregates, and the symmetric guards: one mapping per hot-path access need (never per attribute, never for ops queries), and no contorting keys or aggregates to dodge a legitimate mapping store. CLAUDE.md's query-by-attribute bullet gains the matching clause so the rule and its escape hatch travel together. --- CLAUDE.md | 2 +- submitqueue/extension/storage/README.md | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3f4245ab..de042dd8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -141,7 +141,7 @@ Rule of thumb: if you're about to add a `NewFactory()` or a `map[queue]impl` und Common over-constraints to avoid: - **Batch atomicity** (multi-row inserts as one transaction) — many KV stores can't do this. Prefer single-record primitives + caller loops + idempotency-on-retry. - **Multi-key queries** (`WHERE x IN (...)`) — fine in SQL, awkward elsewhere. Prefer per-key reads. -- **Query-by-attribute / secondary indexes** (`WHERE attr = ?`, `ListByX(attr)`) — a plain KV store cannot look up by anything but the primary key. The mechanical smell test: **if a schema change adds a `KEY idx_*` to make a store method viable, the contract has stopped being get/put-by-key.** Instead, derive the primary key from the composite identity the caller already holds (e.g. `{parentID}/{hash(child identity)}`), and remember that domain state is often already the index — an entity that references its children (a tree listing its paths) enumerates their keys for free. See [submitqueue/extension/storage/README.md](submitqueue/extension/storage/README.md#key-value-contract). +- **Query-by-attribute / secondary indexes** (`WHERE attr = ?`, `ListByX(attr)`) — a plain KV store cannot look up by anything but the primary key. The mechanical smell test: **if a schema change adds a `KEY idx_*` to make a store method viable, the contract has stopped being get/put-by-key.** Instead, derive the primary key from the composite identity the caller already holds (e.g. `{parentID}/{hash(child identity)}`), and remember that domain state is often already the index — an entity that references its children (a tree listing its paths) enumerates their keys for free. When neither applies, a genuinely needed reverse lookup gets its own first-class mapping store keyed by the attribute — in the KV space that is the mechanism, not a workaround. See the decision path in [submitqueue/extension/storage/README.md](submitqueue/extension/storage/README.md#key-value-contract). - **Server-side filters** (joins, sub-queries, complex predicates) — push filtering and aggregation to the caller; keep the store responsible only for "get/put by key" semantics. - **Transactions across entities** — virtually no distributed store offers this. Use eventual consistency + idempotency. - **Strict ordering / exactly-once** in messaging — most queues are at-least-once with best-effort ordering. Make consumers idempotent. diff --git a/submitqueue/extension/storage/README.md b/submitqueue/extension/storage/README.md index 38397938..c82e875b 100644 --- a/submitqueue/extension/storage/README.md +++ b/submitqueue/extension/storage/README.md @@ -35,3 +35,15 @@ Store interfaces are designed for the storage technology *space*, not for SQL (s **Reach for the derived-key pattern instead.** When callers need "all X belonging to Y", encode the relationship in the primary key rather than querying for it: derive the key deterministically from the composite identity the caller already holds — for example `{parentID}/{hash(child identity)}`. Every caller that wants the children can recompute the keys and issue per-key reads; creation under a deterministic key is naturally idempotent (a redelivery finds the existing row); and "at most one row per identity" holds by construction instead of by query discipline. **Domain state is often already the index.** Before adding any lookup, check whether an entity the caller already loads enumerates the children — an aggregate that references its parts by ID (e.g. a tree whose paths record their build identities) is the batch→children index, persisted and versioned as domain state. Duplicating that relationship as a database index adds a second source of truth for something the domain already owns. + +**When neither applies, the reverse lookup is real — give it its own mapping store.** In the KV space there is no third mechanism: the only way to look up by an attribute is to make that attribute a primary key somewhere. So promote the relationship to a first-class mapping entity — keyed by the lookup attribute, written by the same flow that creates the source entity with idempotent puts, and rebuildable as a projection if it drifts. `ChangeRecord` is the in-repo example: it exists so "which requests claimed this change URI" is a by-key read on (queue, URI). Unlike a `KEY idx_*`, the relationship is visible in the contract and portable to any backend. + +### Decision path + +Take the first branch that applies: + +1. **Derive** — the caller already holds the composite identity → encode it in the primary key. No new state. +2. **Enumerate** — an entity already on the caller's path references the children by ID → that aggregate is the index. Escalate to 3 if the list would grow unbounded or take appends from many concurrent writers (a version-contention hotspot under optimistic locking). +3. **Map** — a pipeline controller needs the lookup at runtime → a dedicated mapping store keyed by the attribute. + +The bar for 3 is a hot-path need: one mapping per access path, never per attribute, and never for ops/debug queries — run those against SQL replicas directly. But don't contort 1–2 to dodge a legitimate 3; a primary key that hashes half the entity's fields, or an aggregate bloated into listing everything, is the same duplication hidden in a worse place.