Repository navigation
[pull] main from MaterializeInc:main - #1331
Merged
Merged
Conversation
### Motivation Replica resource usage is only visible as periodic orchestrator samples, roughly one per minute, landing in `mz_cluster_replica_metrics_history`. A spike between two samples is invisible, so the usage of a hydration episode that starts and finishes inside one sampling gap cannot be recovered at all. This is the replica-local, higher-cadence half of the Improved Hydration Visibility work. ### Description `mz_metrics::usage` reads cgroup v2 interface files, `getrusage`, `/proc/self/status` and `statvfs` on the existing periodic metrics task. A new `ComputeLog::ResourceUsage` logging dataflow reports them as `mz_introspection.mz_cluster_replica_resource_usage(process_id, source, metric, value)`, following the `ComputeLog::PrometheusMetrics` shape: usage is per-process, so one worker per process reports and the rest drop their capability. Three decisions the diff cannot show. **The relation surfaces uninterpreted observations.** It does not decide which number is "the" memory usage of a replica, and it never combines two sources into a third figure. Sources measure overlapping but distinct quantities: `cgroup memory.current` is the accounting that limit enforcement and the OOM killer act on, while `VmRSS` is the process's resident set. They disagree by a measured ~96 MiB on staging, in the direction of `VmRSS` being the larger, because a page is charged to whichever cgroup first faulted it in and this binary's resident text was faulted in by the runtime that unpacked the image. That disagreement is the diagnostic signal, and it is not recoverable once two sources have been folded into one number. Combining sources belongs in views, where it can change without a migration and where a reader can see what was combined. For what fusing costs, note that `mz_cluster_replica_metrics.disk_bytes` currently reports disk *plus swap* because a consumer wanted it displayed that way, and the fix is blocked on porting that consumer. **Long rows, not one column per metric.** A wide row retracts and re-inserts on any field's change, so its churn is set by its noisiest field: a filesystem-usage number that moves every sample would drag a memory peak that has not moved in an hour through a retraction every time. Long rows churn only what changed. They also make a missing observation representable as an absent row rather than an in-band sentinel, and let a new metric be added as kernels gain interface files without a catalog migration. **Peaks are reported, not derived.** A peak cannot be recovered downstream from this relation: each sample retracts the previous one, so a `reduce` computing a maximum sees one value per key and returns it. Retaining the samples instead is not an option, because a compute log collection is an arrangement with no retention, window or TTL mechanism available to it, so a retained history would grow unbounded for the process's lifetime. Most peaks cost nothing to report because the kernel maintains them: `cgroup memory.peak` and `memory.swap.peak` are exact high-water marks of the cgroup accounting, kept per cgroup for the container's lifetime and therefore unaffected by how rarely we read them, by the sampler being disabled and restarted, or by `environmentd` restarting; `ru_maxrss` is already a high-water mark. Only a source with no kernel-side peak gets one folded in the sampler, currently the scratch filesystem and swap below the kernel version providing `memory.swap.peak`, and those are reported under a distinct `_peak` metric name so a reader can tell an exact peak from a sampled one. The logging operator downgrades its capability on its own timer rather than on the sampler's. Were the sampler to drive progress, disabling it (which `mz_metrics_usage_refresh_interval = 0` is meant to allow) would freeze the collection's frontier and wedge every query over the relation along with the `mz_catalog_server` indexes on it. Cadence is a new dyncfg, `mz_metrics_usage_refresh_interval` (5s), kept separate from `memory_limiter_interval`, which governs OOM-kill behavior and must not be retuned for introspection's sake. #### Cost No source read here is proportional to heap size. `VmRSS` and `VmSwap` come from per-mm counters and the cgroup figures are `page_counter` reads. Measured on a 32-core Linux host, p50 over 2000 iterations: | read | empty heap | 32 GiB resident | |---|---|---| | `getrusage(RUSAGE_SELF)` | 0.22 us | 0.22 us | | `statvfs` | 0.41 us | 0.41 us | | `/proc/self/status` | 4.67 us | 4.74 us | | `/proc/self/smaps_rollup` | 9.6 us | 137 ms | | `/proc/self/numa_maps` | 20.8 us | 142 ms | The last two rows are not read by this path and must not be added to it. They walk page tables, so their cost scales with resident pages and rises superlinearly. Held at 16 GiB resident, `numa_maps` costs 28.9 ms over one fully-touched VMA, 11.0 ms over 8192 VMAs, and 1.5 ms when only every 64th page is written, confirming the cost tracks page-table entries rather than VMA count. At the 5s default the whole sampler is roughly 0.0002% of one core. #### Incidental refactors `ProcStatus` and the `statvfs` helper move from `mz_compute::memory_limiter` to `mz_metrics::usage`, so the limiter, the clusterd usage collector and the sampler share one reader. The cgroup helpers move from `mz_ore::memory` to `mz_ore::cgroup` and lose their `target_os = "linux"` gate, since a second crate now uses them and the file is pure `std`. The module gains a `CgroupV2` handle that resolves the cgroup directory once and reads its interface files, rather than this becoming a fourth copy of the `/proc/self/cgroup` parse. It exposes the resolved path rather than logging it, so the module keeps its property of depending on nothing outside `std`: it is compiled unconditionally now, including into the wasm32 build where `mz_ore`'s optional dependencies are absent. The three existing cgroup *limit* readers (`usage_metrics.rs`, `ore::cgroup`, `service::boot`) are deliberately left alone; consolidating them is worth doing separately. The Prometheus metrics operator reports a sampled source in the same shape, so the interval-boundary capability downgrade and the snapshot diff become `downgrade_to_interval_boundary` and `emit_snapshot_diff` in `mz_compute::logging`, and both operators call them. Docs: the relation is described in `mz_introspection.md`, and the sources, the metrics they report, how to interpret them and an example query live on a new page, `manage/monitor/replica-resource-usage.md`. Adding a builtin log changes the inlined-VALUES fingerprints of `mz_catalog.mz_indexes` and `mz_catalog.mz_sources`, so both get a `MigrationStep::replacement` at the current dev version. ### Verification - Unit tests in `mz_metrics::usage`: a sample is non-empty, a derived peak rises with a higher observation and survives a lower one, an unreadable source is absent rather than zero, and a derived peak is only published for a source that was actually read. - `test-resource-usage` in `test/cluster/mzcompose.py`: asserts both processes of a 2-process replica report, that they report the same metric set, that `rusage` and `proc_status` are present, and that no peak goes backwards across a workload. cgroup metrics are deliberately not asserted, since `memory.peak` and `memory.swap.peak` depend on the kernel version. - Catalog goldens regenerated. Note that `autogenerated/mz_introspection.slt` needed `ci/test/lint-docs-catalog.sh --rewrite`, not just `--rewrite-results`: the relation name is baked into the query text, and a plain rewrite silently produced an empty result. - Verified on staging (eu-west-1). `memory_peak` and `swap_peak` are both present on the node kernel, and `cgroup memory_current` matches cAdvisor's `container_memory_working_set_bytes` for the same container to within 1.5% across every pod, so the cgroup directory resolution is correct. - A replica hydrating a 60M-row index against a 3.79 GiB RAM limit reported `memory_current` 1.20 GiB / `swap_current` 695 MiB while `memory_peak` was 3.79 GiB and `swap_peak` 3.98 GiB: the instantaneous readings understate the episode by 3.2x and 5.9x. Sampled again minutes later, `memory_current` had risen 2.46 GiB and `swap_current` fallen 616 MiB as pages swapped back in, with both peaks byte-identical. That is the monotonicity property under a bidirectional swing. - `VmRSS` runs a roughly constant 96 MiB above the replica's own cgroup charge (7% spread across replicas spanning 36 MiB to 400 MiB of `memory_current`), because this binary's resident text is charged to the runtime that unpacked the image. Taking `ru_maxrss` as the replica's footprint, as the previous design did, overstates the enforced figure by 2.75x on an idle replica and 1.20x on a loaded one. - `memory.events`' `max` stayed 0 even with the cgroup pinned at its ceiling, because reclaim succeeded by swapping rather than failing. It is therefore not a reliable limit-hit signal where swap is configured; `memory_peak` reaching `memory_max` is. Both are reported without interpretation. ### Release notes This release adds `mz_introspection.mz_cluster_replica_resource_usage`, reporting the resource usage of each cluster replica process as measured by each available source, including cgroup memory and swap usage and their kernel-maintained peaks. Co-authored-by: Claude <noreply@anthropic.com>
The developer endpoint's `initialize` instructions tell agents never to query `mz_introspection.mz_dataflow_arrangement_sizes`. That rule dates from #35555, before the `query` tool existed (#36949): the only tool then was `query_system_catalog`, which cannot target a cluster, so the relation could only answer about the session's default cluster. Through `query` with its cluster argument the read is fine, and it is the standard way to attribute memory per dataflow; agents that follow the current instruction skip it. This restates the rule as what actually holds: - `mz_introspection` relations are cluster-scoped and `query_system_catalog` answers about the session's default cluster, so read them only through the `query` tool with its cluster argument (and `cluster_replica` on a multi-replica cluster), never through `query_system_catalog`. - `mz_dataflow_arrangement_sizes.id` and `mz_dataflows.id` are dataflow ids (`uint8`) that do not join to `mz_catalog.mz_objects.id` (`text`); reach the catalog through `mz_introspection.mz_compute_exports` (`dataflow_id` to `export_id`, which is `mz_objects.id`), or match the `name` column, which reads `Dataflow: <database>.<schema>.<object>`. Other `mz_introspection` relations use other id spaces (peek UUIDs, operator and channel ids), so the rule names these two relations. Both verified live on v26.38.1, on a Cloud environment (v26.39), and against the relation's definition on `main`. The introspection rule is built from the same flag as the `query` tool's own description line: when `enable_mcp_developer_query_tool` hides the tool, the instructions say that cluster-targeted introspection is unavailable on this server instead of naming a hidden tool. Instruction text only, no behavior change. The developer-endpoint `tools/list` tests check both variants of the instructions, and the two datadriven expectations that embed them are updated. Companions in MaterializeInc/agent-skills: [agent-skills#48](MaterializeInc/agent-skills#48) corrects the `mcp-developer-analysis` skill, which repeated the old rule, and [agent-skills#50](MaterializeInc/agent-skills#50) (built on it) drops the material the skill duplicated from these instructions and tells agents that servers before v26.40 still carry the old rule. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…38491) Document the read privileges CREATE TABLE ... FROM SOURCE requires ### Motivation `CREATE TABLE ... FROM SOURCE` populates the new table from an existing ingestion, so creating it reads that source's data. #38480 makes the privilege requirements match that, fixing a case where `CREATE` on the destination schema alone was enough to read a source the role had been denied ([SQL-655](https://linear.app/materializeinc/issue/SQL-655), 2026 penetration test finding H02). The privileges include for `CREATE TABLE` still lists only the schema and type requirements, so it understates what the statement needs. ### Description Add the read requirement to `create-table.md`, the single include used by all five `CREATE TABLE` pages and by the generated privileges appendix: `SELECT` on the source plus `USAGE` on its schema. The note about scope is the part worth reading. The source is the authorization boundary, so `SELECT` on it permits attaching any reference that source ingests, including references with no existing table and columns some existing table omitted. An admin deciding whether to grant it needs that sentence. Split out from #38480 so that fix, which is an urgent security finding, is not gated on a second CODEOWNERS scope. It is accurate to merge this either before or after #38480: before, it documents a requirement that is about to exist; after, it closes a gap where the docs understate what is enforced. ### Verification Prose only. Rendering is unchanged in shape, four bullets where there were three.
Adds `PeekResultIterator::step(&mut fuel) -> Step`, which charges one unit of fuel per cursor position and returns `Step::OutOfFuel` when the budget runs out. The commit is @aljoscha's, extracted verbatim from #38040 and authored by him. The file is byte-identical to its state in that PR, so the extraction carries no rebase risk. Taking the commit directly rather than merging #38040 avoids landing the parts of it that later layers in this stack subsume. Inert on its own: the only caller is `next()`, which passes `usize::MAX`. The layer that supplies a real budget is stacked above, and that is also where `OutOfFuel` first gets test coverage. 🤖 Opened by [Claude Code](https://claude.com/claude-code) on behalf of @antiguru Replaces #38450, which GitHub closed when the design document moved from the bottom of the stack to the top and its branch was force-pushed past these commits. Same content, new base. Co-authored-by: Aljoscha Krettek <aljoscha.krettek@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Introduces `PeekError` as a structured peek failure and adds a configurable limit on how many rows a peek may iterate. The first commit is @aljoscha's, taken from #38158 and authored by him. It carries the uncommitted fixes that accompanied that patch, because the mailbox commit alone leaves a state where the golden in `src/environmentd/tests/testdata/http/ws` contradicts its own code: the double-wrap in `src/environmentd/src/http/sql.rs` re-flattens the structured error to `XX000`. Splitting the fixes out would have attributed his work to someone else and left a red intermediate commit. The version gate was moved to this branch's release. `PeekError::Dataflow` is produced unconditionally, so structured error frames flow on the wire regardless of whether the row-iteration limit is enabled. The second commit resets `enable_compute_peek_row_iteration_limit` after use in the two slt files that set it. 🤖 Opened by [Claude Code](https://claude.com/claude-code) on behalf of @antiguru Replaces #38451, which GitHub closed when the design document moved from the bottom of the stack to the top and its branch was force-pushed past these commits. Same content, new base. --------- Co-authored-by: Aljoscha Krettek <aljoscha.krettek@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extends the existing "Code comments" section of AGENTS.md with three rules distilled from a comment audit of `mz_ore::pool` and `mz_timely_util::columnar` (cleanup in #38434): * **One owning comment per fact.** Reasoning lives at the decision point, mechanism docs at the mechanism, config semantics on the public setter or constant; other sites point at the owner or say nothing. Covers the common failure of the same clause appearing near-verbatim in a module doc, a struct doc, and an inline comment, plus its corollaries: don't restate a callee's contract at call sites, and don't duplicate an assert message in a comment. * **Budget doc-comment paragraphs by decisions.** No re-arguing a decision documented elsewhere, no enumerating callers or fields, no speculative future-implementation notes, no absence-narration, no ASCII section-divider banners, no unverifiable performance color. * **Test-doc economy.** A test whose name and assert messages state the property needs no doc comment; test docs are for non-obvious setup, scenario-encoding fixtures, and multi-phase protocols. The existing rules in the section (fluff, body narration, chronology, NOTE/TODO) are untouched; the additions only cover what the section didn't already say. Generated with [Claude Code](https://claude.com/claude-code)
`misc/mcp-materialize` and `misc/mcp-materialize-agents` both ran on a
floating `python:3.13-alpine` tag with no digest, in directories
dependabot did not cover — so the base was neither pinned nor refreshed.
```
-FROM python:3.13-alpine
+FROM python:3.13-alpine@sha256:540c7d91f98ff6880174c40e99067bf5941eb54d818a7a5e094d188b196a934d
```
plus `/misc/mcp-materialize` and `/misc/mcp-materialize-agents` added to
the docker ecosystem in `.github/dependabot.yml`.
Both halves are needed: a digest alone freezes the base, coverage alone
leaves it mutable. The tag stays *rolling* (`3.13-alpine`) on purpose —
dependabot only refreshes a digest when the tag rolls, so a
version-stamped tag plus a digest would never update.
Alpine is fine here, sitting alongside debian-slim as an acceptable
minimal base, so this is a pinning fix rather than a base migration.
### Digest verification
The digest is the multi-arch **index**, not a per-arch manifest — a
per-arch pin builds on an arm64 laptop and dies in amd64 CI with `exec
format error`:
```
$ docker manifest inspect python@sha256:540c7d91...
mediaType : application/vnd.oci.image.index.v1+json
is_index : True
platforms : linux/386, linux/amd64, linux/arm/v6, linux/arm/v7,
linux/arm64/v8, linux/ppc64le, linux/riscv64, linux/s390x
```
### Worth considering separately
`dhi.io` publishes a hardened `python`, including an alpine variant — I
confirmed `dhi.io/python:3.13-alpine3.22` carries openssl 3.5.7 where
the current upstream alpine base is behind. That would be a rung up from
plain alpine, but it is a base migration rather than a pin, so it does
not belong in this PR.
One of three PRs closing the unpinned production bases in this repo; the
others cover the console images and `materialized-base`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Jason Hernandez <7144515+jasonhernandez@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Require read privileges for CREATE TABLE ... FROM SOURCE Fixes [SQL-655](https://linear.app/materializeinc/issue/SQL-655) (2026 Refactor penetration test, finding H02, High). ### Motivation `CREATE TABLE ... FROM SOURCE` bypassed source authorization: it required only `CREATE` on the destination schema, and nothing about the source it reads. Because the new table is owned by whoever creates it, `CREATE` on any schema a role controls was enough to read a source that role had been explicitly denied. The assessment read 4,076 rows from an Auction load generator through an attached table while direct reads of the same export stayed denied. The behaviour has been reachable by default since v26.25.0 and was still present in v26.35.0. ### Description `CREATE TABLE ... FROM SOURCE` now requires `SELECT` on the source and `USAGE` on its schema, the same read privileges `CREATE SINK` requires on the relation it reads. **The source is the authorization boundary.** `SELECT` on a source permits attaching any reference that source ingests, including references that have no table yet, and including columns some existing table omitted. If a reference should not be reachable, it should not be in the source's publication or topic set. Pentest remediation item 2 (additionally requiring `SELECT` on an existing table for the same reference) is intentionally not implemented: making the outcome depend on which other tables happen to exist would let one team's migration revoke another team's ability to run an unchanged statement. Per-column granularity belongs in column-level grants. ### User-visible effect This tightens an existing privilege requirement, and `enable_create_table_from_source` is on by default, so it applies to everyone on upgrade. A role that previously needed only `CREATE` on its own schema plus `USAGE` on the source's schema now also needs `SELECT` on the source. Deployments where a platform team owns sources and application teams attach tables into their own schemas will need those `SELECT` grants added. The user-facing privileges include for `CREATE TABLE` is updated in a follow-up PR, kept separate so this fix is not gated on a second CODEOWNERS scope. ### Verification New `test/sqllogictest/rbac_create_table_from_source.slt`: a direct read is denied; the attach is denied without `SELECT` on the source; the attach is denied for a role that has `SELECT` on the source but no `CREATE` on the destination schema; it succeeds with both; and a second role with the same grants can attach the same reference after another team has already attached it. Registered in `tests_without_views` alongside the other RBAC suites, since `--auto-index-selects` view-wrapping would change privilege semantics. ### Residual, not fixed here Purification runs before authorization, and its gate covers secrets, connections and types but not sources. The upstream connection also comes from the source's own definition rather than the statement, so an unauthorized role can still cause an outbound connection to the source's upstream using the source owner's credentials, and use purification errors as an upstream existence and column-name oracle. Shared with `CREATE SOURCE` and `CREATE SINK` rather than introduced here; closing it needs a pre-purification privilege gate. Filed separately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )