Repository navigation
Make mtree parallelism controllable and predictable - #170
mason-sharp wants to merge 2 commits into
Conversation
A customer running mtree build on 28M-row tables saw their 16-core database servers fully loaded for the whole leaf-hash phase, and lowering --max-cpu-ratio to 0.1 made no real difference. Reproduced on 2-core nodes from a 14-core client with -m 0.1: 3 hash backends per node, 2 Postgres parallel workers under each, and 1 idle pooled connection. Causes: - mtree build and update computed workers as ceil(cores * ratio * 2). The doubling was undocumented and the other mtree paths did not have it, so -m 0.1 on 14 cores gave 3 workers instead of 1. - The count is a share of the cores on the host running ACE, never the database server, and nothing capped it. - Each hash query could also fan out into Postgres parallel workers, which ACE never controlled. - Nothing logged the worker count. Changes: - One worker formula for all mtree paths: round(cores * ratio), never below 1. The doubling is gone, and rounding matches table-diff. The default of 0.5 now runs half as many build and update workers. - --max-connections / -M on mtree build, update, and table-diff, with mtree.max_connections in ace.yaml and max_connections on the HTTP API. Same meaning as on the diff commands: a hard cap on the pool per node, applied to every pool the task opens. One connection holds the tree's transaction, so N connections means N - 1 hash workers, and a value of 1 is rejected. - Every ACE connection now sends max_parallel_workers_per_gather = 0 as a session setting. postgres.max_parallel_workers_per_gather in ace.yaml overrides it; -1 leaves the server's setting alone. - Each mtree phase logs the worker count with the host CPU count and the flags it came from; build also logs the pool size per node. The task store records the ratio and cap for build, update, and diff.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds per-node connection limits and a shared worker-count calculation to Merkle-tree commands. CLI and HTTP inputs expose the limit. PostgreSQL connections also receive a configurable ChangesMerkle resource controls
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The resource controls appear mergeable after normal checks. No concrete connection-limit or incomplete-comparison failure was established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change generally reduces database load and preserves certificate-authenticated access. Connection limits are adjustable per-task settings, not a server-wide budget. No introduced security vulnerability was established, but deployment-wide effects and connection usage have not been fully validated. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 52.63% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 7 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
A rabbit checks the worker count, Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
| Duplication | 6 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
danolivo
left a comment
danolivo
left a comment
There was a problem hiding this comment.
Commitable, if author fixes 'hard cap' issue - '--max-connections' for mtree table-diff
mtree table-diff opened a pool for every compare worker and another pair for every node pair during the tree traversal, all held open until the diff finished. --max-connections bounded each of those pools on its own, so with W workers on a three-node cluster one node could see W + 2 pools, each as large as the cap. The cap was not a cap. The diff now opens one pool per node up front and hands it to the traversal, the compare workers, and the stale-block refresh. Under --max-connections the pool is the cap; without one it is a connection per worker plus one for the refresh transaction, as build already does. Workers that find no pool for a node record the pair as incomplete instead of dialing.
2 participants
A customer running mtree build on 28M-row tables saw their 16-core database servers fully loaded for the whole leaf-hash phase, and lowering --max-cpu-ratio to 0.1 made no real difference. Reproduced on 2-core nodes from a 14-core client with -m 0.1: 3 hash backends per node, 2 Postgres parallel workers under each, and 1 idle pooled connection.
Causes:
Changes: