refactor(inkless:produce) Cut commit_file_v2's staging cost from O(requests x partitions) to O(partitions) - #751
Draft
jeqo wants to merge 2 commits into
Draft
refactor(inkless:produce) Cut commit_file_v2's staging cost from O(requests x partitions) to O(partitions)#751jeqo wants to merge 2 commits into
commit_file_v2's staging cost from O(requests x partitions) to O(partitions)#751jeqo wants to merge 2 commits into
Conversation
Production metrics show one cluster committing ~632 distinct partitions and ~1580 batches per 4 MiB file (p999), and another ~130 partitions at the same file size. Fan-out is therefore a workload property, but nothing told us what it costs the control plane, and the existing coalescing benchmark tops out at 64 partitions. This harness answers the two questions field metrics cannot separate: - fanOutSweep holds batches-per-commit constant and varies only the partition spread. The commit_file_v1 leg is the controlled one: v1 writes one batches row per request regardless of shape, so insert volume and request-array size stay fixed and partition count is the only variable. It also samples WAL via a pg_current_wal_lsn() delta. - tempTableChurnFloor prices the per-call CREATE TEMPORARY TABLE ... AS / DROP pair on its own, empty and at production width, so the DDL can be compared against whole-commit numbers instead of assumed expensive. - walFromLoopedUpdates contrasts the per-request staging updates against the same updates on a logged table, which is what the staging table exists to avoid. Baseline on this commit (local container, 1600 batches per commit, v2 leg): 83.1 ms at 16 partitions, 106.5 at 320, 112.5 at 640, 213.4 at 1600; the temp-table pair costs 867 us empty and 968 us at 630-row width; WAL fits 18 KiB + 0.63 KiB per batches row. Tagged benchmark, so excluded from check/test; run with ./gradlew :storage:inkless:benchmarkTest.
…ted batch commit_file_v2 ran one `UPDATE logs_tmp ... RETURNING high_watermark` per accepted request, purely to keep the l_log variable in sync with the staging table. logs_tmp is created by CTAS and so has no index, which made that update a seqscan over one row per distinct partition in the file: staging cost was O(requests x partitions), quadratic in fan-out. V25 accumulates in l_log instead and writes back once per partition run - at each partition change, before the SELECT overwrites the variable, and once after the loop. Staging cost becomes O(partitions). The transformation is semantics-preserving: l_log was already the value the loop reads (l_assigned_offset and the run bookkeeping both consume it), and the RETURNING existed only to refresh it. The write-back sets absolute values rather than deltas, so re-reading a partition after a flush yields the accumulated state; correctness therefore no longer depends on arg_requests being grouped by partition, which the previous code relied on for efficiency only. Rejecting branches still CONTINUE above the accumulation, so a rejected request leaves the watermark untouched, and a partition missing from logs leaves l_log all-NULL, which both flush sites guard against. Measured with CommitFileFanOutCostBenchmarkTest (1600 batches per commit, v2 leg, before -> after): 106.5 -> 77.2 ms at 320 partitions, 112.5 -> 85.4 at 640, 145.9 -> 109.6 at 800. The 1600-partitions-by-1-batch shape is the control - there flush-per-run is update-per-request, so no saving is possible and none appears (213.4 -> 208.3, within noise). WAL is unchanged: logs_tmp is a temporary relation and is not WAL-logged (the same 1600 updates against a logged table cost 345 KiB), and the transfer into real logs is still one UPDATE per partition. commit_file_v1 keeps the old accumulation: it is deprecated, retained only for mid-rolling-upgrade brokers and as the coalescing benchmark's baseline.
Contributor
There was a problem hiding this comment.
Pull request overview
Optimizes commit_file_v2 by accumulating log state locally and reducing repeated temporary-table scans.
Changes:
- Adds fan-out, temporary-table, and WAL benchmarks.
- Introduces migration V25 with per-partition-run write-backs.
- Regenerates jOOQ sources for schema version 25.
Reviewed changes
Copilot reviewed 2 out of 120 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
CommitFileFanOutCostBenchmarkTest.java |
Adds performance benchmarks. |
V25__Commit_file_local_log_accumulation.sql |
Implements local accumulation. |
storage/inkless/src/main/jooq/org/jooq/generated/**/*.java |
Regenerates jOOQ metadata for schema 25. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+17
to
+18
| -- Here the accumulation happens in `l_log` and is written back to `logs_tmp` once per partition run | ||
| -- (at each partition change, and once after the loop), so the staging cost becomes O(partitions). |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
commit_file_v2ran oneUPDATE logs_tmp ... RETURNING high_watermarkper accepted request, only to keep a plpgsql variable in sync with the staging table.logs_tmpis created by CTAS and therefore has no index, so each of those updates seqscanned one row per distinct partition in the file — making the staging cost quadratic in fan-out. That matters because fan-out is set by producers, not by us: one production cluster commits ~632 distinct partitions and ~1580 batches per 4 MiB file at p999, another ~130 partitions at the same file size.This PR adds a benchmark that isolates the cost, then removes it. Measured with
CommitFileFanOutCostBenchmarkTest(1600 batches per commit,commit_file_v2leg, before -> after):The last row is the control: with one batch per partition, flush-per-run is update-per-request, so no saving is possible — and none appears. WAL is unchanged.
Commits
test(inkless:control_plane): add commit_file fan-out cost benchmark— a harness that varies fan-out at constant batch volume (using thecommit_file_v1leg as the controlled one, since v1's inserted-row count is shape-independent), prices the per-call temp-tableCREATE/DROPpair on its own, and samples WAL per commit.fix(inkless:control_plane): stop re-scanning logs_tmp once per committed batch—V25__Commit_file_local_log_accumulation.sql: accumulate in the local, write back once per partition run.See the commit messages for the reasoning behind each step, including the argument for why the transformation is semantics-preserving.
What this is not
Two things the benchmark ruled out, recorded here so they are not re-proposed:
CREATE TEMPORARY TABLE ... AS/DROPpair costs 867 us empty and 968 us at 630-row width — under 1% of a commit. It does own ~18 KiB/commit of catalog WAL, which is a separate follow-up.WAL/commit ~= 18 KiB + 0.63 KiB x batches-rows. Since coalescing's floor is onebatchesrow per partition per file, slicing a 630-partition file in two roughly doubles the rows, and so the WAL, for the same data.Migration notes
V25is aCREATE OR REPLACEofcommit_file_v2with an unchanged signature and unchanged semantics, so it needs no coordination with broker rollout.commit_file_v1is deliberately untouched (deprecated; retained for mid-rolling-upgrade brokers and as the coalescing benchmark's baseline). The regenerated jOOQ sources carry only theschema version:24 -> 25bump.No new or changed configs, no new or renamed metrics, no operator-visible behaviour change beyond lower
commit_filelatency.Testing
Container measurements are indicative: no lock contention, no network, and a different fsync profile from production, where the
commit_filemean is ~562 ms. The cost shape should transfer; treat the ~25% as an upper-bound estimate until the productioncommit_filemean moves.Follow-ups (not in this PR)
commit_batch_request_v1[]argument rather than the plpgsql loop, testable by timingSELECT count(*) FROM unnest(arg_requests)alone. If that is where the time goes, it dwarfs this change.logs_tmpentirely for in-memory accumulation would remove the residualO(partitions^2)staging term (which dominates above ~1000 partitions per commit) and the ~18 KiB/commit catalog WAL. The CPU payoff is uncertain — plpgsql composite-array mutation can re-import the same cost as memcpy — so it needs its own prototype and benchmark rather than being bundled here.