Repository navigation
[pull] main from MaterializeInc:main - #1374
Merged
Merged
Conversation
### Motivation On swap-based replica sizes, the catalog only exposes the combined `heap_bytes` / `heap_limit`. clusterd already measures swap separately (`VmSwap` from `/proc/self/status`) and reports it as `swap_bytes` on `/api/usage-metrics`, but the Kubernetes orchestrator folds it into `heap_bytes` (and, for compatibility, into `disk_bytes`) and drops the standalone value, so it never reaches SQL. Deriving it downstream is unreliable: `heap_bytes - memory_bytes` mixes clusterd's VmRSS with the metrics-server working set (different instruments, different sampling instants), and `disk_bytes` only equals swap on swap-enabled sizes, which are not distinguishable from public catalog relations. Recording swap first-class lets the Console show how much of a replica's memory is spilled to disk. Fixes [CNS-154](https://linear.app/materializeinc/issue/CNS-154/add-swap-metrics-to-database-catalog) ### Description * Add `swap_bytes` to `ServiceProcessMetrics`. The Kubernetes orchestrator keeps the clusterd-reported value (the existing swap-into-`disk_bytes` shim is unchanged); the process orchestrator reports `None`. * Append `swap_bytes` as a nullable `uint8` column at the end of `REPLICA_METRICS_HISTORY_DESC`. This is a backward-compatible persist schema change, so existing history is preserved and older rows read `NULL` (same pattern as `heap_bytes` / `heap_limit` in 96fa447). * Record it in `mz_internal.mz_cluster_replica_metrics_history` and expose it on `mz_internal.mz_cluster_replica_metrics`. * Add `swap_percent` to `mz_internal.mz_cluster_replica_utilization` and `mz_cluster_replica_utilization_history`, computed as `swap_bytes / heap_limit * 100`. No clusterd changes are needed: every running clusterd already reports `swap_bytes`, so the metric lights up on existing replicas as soon as environmentd upgrades. Two decisions worth reviewer attention: * **`swap_percent` denominator**: `heap_limit`, matching the sibling columns' "percent of the total allocation" contract, so `heap_percent - swap_percent` reads as the RAM share and the two stack in a gauge. The spill fraction (swap as a share of `heap_bytes`) is derivable as `swap_percent / heap_percent` or from the raw columns. * **Follow-up**: once the Console reads `swap_bytes` directly, the orchestrator shim that folds swap into `disk_bytes` can be retired (see the comment in `get_metrics`). ### Verification Regenerated the explain goldens (`system-cluster.slt`, `catalog_server_explain.slt`) and `autogenerated/mz_internal.slt`, and added the new columns to `mz_catalog_server_index_accounting.slt`; all four files pass against a fresh catalog. The system-catalog docs page is updated to match the column comments. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Rewires `mz_join_core` to take a container builder instead of an output container, then uses that to let a linear join's last stage write the node's output edge directly. Follows up on the review note on #37784. `vec_to_columnar` encoded the last stage's output after the fact whenever no finalization closure followed it. The stage now writes a `ConsolidatingColumnBuilder`, which drops the operator hop and the `Vec` of owned `Row`s that used to sit on the edge in between. The builder choice is per stage rather than global. A `Column` is the wrong intermediate for a consumer that re-encodes what it reads, and both a following stage's arrangement and a finalization closure do exactly that: a `Vec` hands them moved `Row` allocations where a `Column` would copy row bytes first. Every non-terminal stage therefore keeps the `Vec` accumulator, and an error-capable closure keeps it even when terminal, because its output passes through `ok_err` and the demux materializes the ok side as a `Vec` regardless. The delta join's `vec_to_columnar` is not addressed here. Its stages chain through a `Vec`-typed `half_join` input and its output passes through three more operators before the node boundary, so it needs its own change. <sub>Posted by Claude Code</sub>
Makes temporal bucketing columnar throughout, removing the four `Vec` round-trips its call sites paid. The operator's state was already columnar, since its bucket chain wraps `ColumnMergeBatcher`, so the decode and re-encode existed only at its boundary. Three per-record costs go with the round-trips. The reveal path handed the output builder owned tuples reconstituted from the chain's `Column` chunks, and now gives each chunk as a container. `MergeBatcherWrapper::push_container` staged its `Vec` argument into a `Column` one record at a time before the chunker could read it, and now takes a `Column`. `Bucket::split` round-tripped the sealed chunks through a `Vec`, cloning every record, and now re-chunks the containers. On the input side, records are addressed by index through a permutation rather than moved, so neither the pass-through nor the bucket routing materializes an owned record. The partition happens before the sort, because only retained records need ordering and in steady state the pass-through share is the larger one. `TemporalBucketing::bucket` now returns `Self`, which lets a `Vec` stream and a columnar stream both implement it. The reduce key-value path stays on the `Vec` implementation, which stages through the columnar operator so there is one bucketing implementation rather than two. It feeds an arrangement, and a consumer that re-encodes what it reads is cheaper to hand moved allocations than copied bytes. Worth knowing for review: `enable_compute_temporal_bucketing` is compiled in as `false` but served on in production, so this is a live path rather than a dormant one. [CPU-213](https://linear.app/materializeinc/issue/CPU-213) tracks reconciling the default. Verified with the flag defaulted on, since the compiled-in default otherwise leaves the operator untested outside `temporal_bucketing.slt`. <sub>Posted by Claude Code</sub> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
) `decode_and_mfp` wrote both sides of a persist read into one container as `Result`s, so every consumer of `persist_source` ran an `ok_err` demux, and a consumer that wanted columns paid a second pass to re-encode the ok side. The decode operator gets a second output instead, with the ok side generic over a container builder. The compute import asks for columns and gets them straight from the decode; the storage sink and the materialized-view read-back ask for row vectors and get the containers they had before, without the demux. There is one decode implementation. `record_time` picks what a record stores for its time. Compute passes `|time| time.0`, dropping the `Subtime` coordinate: it refines the capabilities so flow control can pace parts within a millisecond, it stays on the capabilities either way, and no reader of those streams distinguishes times that finely. `persist_source_core` passes `|time| time` and keeps it, because `upsert` builds its collection inside the refined scope, then joins the two sides back with `map` and `concat`. That is a move per record on a path that had none, and it is the reason there is one decode operator rather than two. Three commits, separable if you would rather review them apart: * `txn-wal`: `txns_progress_remap` and `txns_progress_frontiers` are exposed separately, so the two streams share one subscription to the txns shard. `txns_progress_frontiers` becomes generic over the container it passes through. * `timely-util`: an `Enter` impl for `Column<(D, T, R)>`, which a columnar collection needs to enter the iterative scope of a `WITH MUTUALLY RECURSIVE` dataflow. * `storage-operators` and `compute`: the split itself. On the table-import dataflow this removes the `OkErr` and `VecToColumnar` operators and four `Vec<(Result<Row, E>, (Timestamp, Subtime), Diff)>` channels, visible in the change to `test/sqllogictest/introspection/relations.slt`. Not measured. The `map`/`concat` on the upsert rehydration read is the one place this adds work rather than removing it, and `delay_sources_past_rehydration` exists because that path is latency-sensitive, so it is worth a look. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
) `set_retry_on_agent_lost` gives every step four automatic retries for failures that belong to the infrastructure rather than the step: a lost agent, an agent stopped by the OS, exit 128 for a registry or GHCR hiccup, and exit 199 for a Rust ICE. It skipped any step that declared `retry.automatic` of its own: ```python retry = step.setdefault("retry", {}) if "automatic" in retry: continue ``` So a step asking for one extra rule lost all four rather than gaining a fifth. Nothing reports that, and the step then hard-fails on precisely the transient classes the shared rules exist for. The loss is worth more than it looks. `ci/plugins/mzcompose/hooks/command` deliberately propagates 128 and 199 out of a job rather than collapsing them to 1, with a comment saying that overwriting them "turns a self-healing infra failure into a job that stays red until someone retries it by hand", and `mzbuild.py` exits 128 when a required image cannot be pulled. A step that overrode the rules went red on the first failed image pull where every other step retried twice — the MinIO Docker Hub outage fixed in #38802 is exactly that shape. Merge instead of skip, adding only the rules a step does not already declare, and leave a step that retries on every exit status alone since it covers them already. ### What regains coverage Applying the old and new function to every checked-in template, thirteen steps were silently missing rules: | pipeline | steps | was missing | |---|---|---| | test | `devel-docker-tags`, `console-sql-test` | all four | | test | `check-merge-with-target` | agent-lost only, having re-listed the other three by hand | | nightly | `devel-docker-tags`, `aws-real`, `aws-glue-schema-registry-real`, 4× `k8s-node-recovery-*`, `terraform-aws-upgrade`, `terraform-azure` | all four | | spec-sheet | `devel-docker-tags` | all four | After the change every step in every template carries all four, with no duplicated rules. The `ci/deploy` steps retry on `exit_status: "*"` and are left untouched. Re-listing the shared rules inside a step stays correct and is now redundant rather than required; I left the existing re-listings in place rather than widen the diff. Found by the post-merge QA review on #38797. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 : )