executor: replace HashBuild estimates with allocation-owned admission - #26531
Conversation
mergify
Bot
added
kind/bug
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The earlier expression-tree and join-result blockers are fixed. The remote late-fragment blocker is resolved by the execution/CN-keyed tombstone and is withdrawn. The original short sparse-bitmap case is fixed, but its replacement sizing rule introduces a distinct blocking panic for valid stale-long bitmaps. The separate workload-validation blocker remains open per the author and is omitted because another reviewer already raised it. Focused tests could not run because this worktree lacks cgo/libmo.dylib.
P1 - Preserve stale bitmap extent when duplicating accounted vectors (pkg/container/vector/vector.go:1677)
Author response: The author states that Dup now admits every non-empty external bitmap for the complete vector row domain, with coverage for bitmap.Len() < v.length. That reported sparse case is fixed. Why this remains blocking: the inverse state is also valid—SetLength changes only v.length, and the existing TestUnionBatchFastPathStaleBitmapBits explicitly documents bits beyond the shortened length as normal reuse state. This change removes the previous bitmap-length sizing and admits only v.length. For an accounted vector preextended to N rows, with a null/grouping bit near N-1 and then SetLength(1), Dup allocates one-row external bitmap capacity before Bitmap.InitWith copies the N-row source; InitWith panics when the source data exceeds that capacity. This is a distinct stale-extent root cause, not a repeat of the short sparse case. Size for the checked maximum of vector length and bitmap length, or deliberately trim the copied bitmap to the row domain, and cover stale null/grouping extents—including the constant-grouping branch.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes.
The earlier expression-tree, retained join-result, and remote late-fragment ownership blockers have responses, but this head is still not ready to approve.
-
Vector.Dup must preserve valid stale bitmap extent. A vector may be preextended with a null/grouping bit near the old high watermark and then shortened with SetLength(1). If Dup allocates external bitmap capacity from only the current vector length, Bitmap.InitWith can copy the longer valid source bitmap into too-small storage and panic. Admit the checked maximum of the vector row domain and each source bitmap extent, or explicitly trim semantics before copying, for both null and grouping, including the constant-grouping path. Add the stale-long null/grouping regression; the sparse-short case alone is insufficient.
-
Workload acceptance is incomplete. The PR itself says the controlled base/head TPCH resident+spill A/B evidence and issue acceptance matrix are not yet complete while a TPCH regression is being resolved. This change replaces a production memory-admission path across 205 files; package/race tests do not establish distributed no-OOM, spill correctness, or the non-spill performance contract. Please add durable regressions/workload evidence and update the stated acceptance matrix before approval.
|
Addressed the stale-long bitmap blocker in d13b910. |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The d13b910 delta fixes the prior stale-long Vector.Dup panic by admitting max(vector length, source bitmap extent) for null/grouping storage, including constant and empty-stale cases; that blocker is withdrawn. The sparse-short bitmap issue remains fixed. Earlier expression-tree and join-result blockers are fixed, and the remote late-fragment blocker remains withdrawn. The workload-validation blocker remains unresolved in another reviewer’s thread and is not duplicated here. Focused tests/build/vet could not run because local CGo artifacts and headers are absent; formatting and diff checks pass.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the complete PR at d13b9103c8 and read all existing discussions and author responses. The retained expression-tree and join-result ownership blockers are fixed. The sparse-short and stale-long Vector.Dup panics are also fixed at this head; their focused tests pass locally.
Three blockers remain:
- Accounted buffer growth applies
GrowCapacitytwice, producing false hard-cap rejections (inline). The same shape exists in the accounted string-hash iterator. - The remote late-fragment fix only rejects the old generation for five minutes (inline). A pipeline RPC can remain alive for the 24-hour
MaxRpcTime, so the same execution can still reopen after tombstone expiry. - Workload acceptance remains incomplete. The latest TPCH100G+1T run is still in progress, with the 1T step and result collection unfinished. The committed evidence predates the final semantic fixes, and the PR still has no durable workload-level acceptance matrix for #26174/#26192/#26413/#26454 plus the original #25782 high-cardinality case. Package tests cannot establish distributed no-OOM, terminal-zero, spill correctness, or the base/head resident+spill performance contract.
Local validation: git diff --check, build, vet, and the complete tests for mpool, hashmap, vector, hashbuild, spillutil, and compile pass both normally and under -race; the focused bitmap/account/remote tests also pass under -race -count=100. Two counterexample regressions fail on this head: the exact single-replacement admission boundary is rejected as used=10240 requested=24576 limit=26624, and deleting the expired tombstone allows the same execution key to register successfully. Please fix both invariants and close the workload acceptance evidence before approval.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking issue on the current head: optools/images/Dockerfile changes the native stage from make -C thirdparties to init usearch xxhash croaring, so it no longer produces the platform ONNX Runtime library. This stage is not non-ONNX-only: .github/workflows/native-dependency-check.yml builds target native from this Dockerfile and explicitly verifies thirdparties/install/lib/onnxruntime.so on amd64 and onnxruntime_arm64.so on arm64. Both Native build jobs on run 30795881950 fail at that exact test. Please preserve the native-stage output contract, or update every consumer and its validation coherently; skipping the required artifact in this shared stage leaves the branch image incomplete.
This reverts commit 95baebc.
|
The temporary ONNX-runtime skip used only for remote acceptance validation has been moved off this PR to It is reverted from this PR by |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 57811ed7fa7c. The new commit exactly reverts the Dockerfile change that omitted ONNX Runtime; its tree is byte-identical to reviewed head cd36bbbe776e, where the HashBuild allocation-owned admission, remote tombstone lifetime, spill-recovery reservation, ownership/lifecycle paths, and focused package tests were already checked. The shared native stage now again satisfies native-dependency-check.yml. No remaining code blocker found. CI for this head is still running.
iamlinjunhong
left a comment
iamlinjunhong
left a comment
There was a problem hiding this comment.
[P1] 24 小时 tombstone 注册表缺少硬空间上限
不完整的远端执行结束时，installRemoteAllocationStatementTombstoneLocked 会把唯一的 executionID@CN key 和一个 timer 放入进程级全局 map：
remoteAllocationStatementGroups.tombstones[key] = tombstone
tombstone.timer = time.AfterFunc(
remoteAllocationStatementTombstoneTimeout,
...
)
该 tombstone 固定保留 MaxRpcTime = 24h，但注册表没有数量、字节或 admission 上限。
触发路径是：
远端计划预计多个 fragment；
dispatch 只成功一部分，或某个 fragment 提前失败；
group 被终止并安装 tombstone；
每次重试使用新的 execution ID，旧条目无法复用；
在网络异常或持续 partial-dispatch 下，CN 保留约 失败速率 × 24h 个 map entry、字符串和 runtime timer。
|
Addressed the tombstone-registry P1 in bf244d6. The fix reserves bounded generation capacity at first remote-group admission: len(byKey) + len(tombstones) < 4096. Existing fragments of an admitted generation reuse that reservation. A partial dispatch can therefore transfer its already-reserved generation into a 24-hour tombstone, but retries cannot create more retained keys or timers than the hard limit. At capacity, only new generations fail closed; no tombstone is evicted, so a late fragment cannot reopen an aborted execution. The regression fills the registry to the exact boundary, proves an existing sibling is still admitted, a new execution is rejected, and the active partial execution transitions to the 4096th tombstone. Focused tests passed under race with count 100; the full pkg/sql/compile package passed under race, and vet passed. |
Merge Queue Status
This pull request spent 27 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Requeued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 46 minutes 21 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks Failing checks:
HintYou may have to fix your CI before adding the pull request to the queue again. Requeued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 32 minutes in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #26531 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
Failing checks:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
6 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #26459
What this PR does / why we need it:
Replaces estimator-driven HashBuild hard admission with one allocation-owned production path:
Reset -> ClearAllocationAccount).Validation was completed locally after rebasing onto main
49f9e33cae48; candidate head is258010da171c:-race -p=2: pass;-race -count=10: pass;go vet -mod=readonlyandgo build -mod=readonlyfor every modified production package: pass;git diff --checkandgofmt: clean;Reset -> ClearAllocationAccountlifecycle.Local benchmark evidence shows zero Go allocations for physical admission/release, bounded vector/pSpool reuse, fixed iterator count for mixed GROUPING batches, and spill serialization improving from 67,751 ns/op / 2,904 B/op / 516 allocs/op to 62,709 ns/op / 344 B/op / 4 allocs/op.
Design, implementation plan, local validation, and raw benchmark evidence:
docs/rfcs/00000000_allocation_accounted_memory_admission.mddocs/design/allocation_accounted_memory_admission_impl.mddocs/design/evidence/26459_local_validation.mddocs/design/evidence/26459_allocation_accounting_bench.txtA distributed TPCH 100G validation run (
30690122244) exposed one lifecycle boundary bug rather than OOM: Q7 remote fragments sharing a CN also shared a message board, while the first fragment completion closed that board before its siblings finished. The fix gives each physical remote execution a UUID and expected-fragment count per CN; registration and completion are counted, and only the final fragment closes/drains the board and finalizes every allocation account exactly once. A four-fragment lost-final-response test covers the failure edge. Fullpkg/sql/compilenormal/race tests, vector/batch normal/race tests, affected vet, repeated alias decode benchmarks, and an independent correctness re-review all pass after the fix.Latest-main spill-recovery integration
Integrated the spill-recovery changes from
d29b4758f8(PR #26585); the branch is current through820ee12144and preserved its recovery-liveness invariant without restoring a second estimator ledger. Each shuffle HashBuild worker now pre-admits an isolated recovery-capacity class; physical MPool allocations borrow that headroom and remain the only exact allocation owner, so recovery capacity is not double-charged. The floor covers expression replacement, partition/materialization, marshal, and serial Packer headroom, and is released at the build terminal boundary. Vector varlena-area proofs and the serial encoded-size/Packer contracts from #26585 are retained.Post-merge local validation at
0552adba24: affected and dependent package tests pass;mpool,process,hashbuild, andhashjoinpass under-race; focused recovery transfer/isolation tests pass under-race -count=100; and the forced-shuffle SQL blackbox (TestHashBuildSharedBudgetRecoverySQL, 28 MiB cap, DOP 8,serial_full, one million rows) spills successfully, returns the exact result, and admits a fresh statement afterward. The retained varlena projection benchmark remains allocation-free (~0.24 µs per 32-column 8K-row batch).