Repository navigation
feat(planning): add finite-byte admission for retained snapshots - #462
realAaronWu wants to merge 6 commits into
Conversation
|
@lucasfang all four upstream checks now pass on unchanged head
Independent checks verified the tested PR merge has the reviewed head's exact tree, actual job/step completion and selected native/GCC8/pre-commit raw logs. The description distinguishes complete logs from successful metadata where transport limited downloads, and retains the sanitizer/support boundaries. Local/fork qualification and source-review findings are closed; no new source change or CI rerun is proposed. Could you review the PR and merge it if the implementation and documented scope look acceptable? My account has no upstream merge permission. This updates the original approval request; CI approval is resolved. The dependent duckdb-paimon #145 also passes its final Linux SQL/distribution and Linux/macOS dynamic bridge CI, while its partial experimental scope remains separately tracked. |
|
@lucasfang gentle follow-up: could you review this PR and approve/merge if the implementation and documented scope look good? Unchanged head |
|
Thank you for your submission! We’ve been preparing for the 0.4 release recently and there’s a fair amount of regression work to work through. I’m afraid we may not be able to start the review this week, but we expect to begin next week. Thanks again for your patience and understanding! |
|
@realAaronWu Sorry for the late response, and thanks for working on this and for the detailed implementation and documentation. I’m concerned about the maintenance cost of introducing a separate planning and metadata decoding path. Future changes to the manifest format, file metadata, or regular planning semantics would need to be reflected in both implementations, creating a risk of divergence. I would prefer reusing the existing planning path and passing a budget-aware Some internal container allocations would remain outside this accounting. If those allocations are relatively small and temporary objects are released promptly, I think that is a reasonable tradeoff, provided the API clearly documents that the budget covers pool-managed allocations rather than all planning memory. Accounting for every allocation—especially through assumptions about STL implementation details—seems to introduce substantial maintenance risk. |
Purpose
Ordinary metadata planning does not admit every variable allocation against a caller-owned byte limit. Add opt-in
TableScan::CreateWithPlanningBudgetso finite-budget callers can reject allocations before requesting storage, including allocations outsideMemoryPool. Exhaustion returnsCapacityError; allocation failure returnsOutOfMemory. No failure returns a partial plan or invokes unrestricted planning.The budget counts cumulative admitted requested bytes. Frees do not replenish it, so temporary input, parser, decompression and publication storage cannot each reuse the whole limit. It covers retained caller schema/path/options, scan/plan objects, metadata buffers, JSON DOM/parser stacks, Avro schema/records, Zstandard workspace/output, native metadata, strings, split vectors and ownership blocks. Fixed diagnostics/control objects, immutable process constants and existing shared services are excluded. This is not an RSS or peak-live measurement. Escaped nonempty splits retain the budget and allocator. Overlapping operations and escaped-owner cleanup require a thread-safe upstream allocator, such as the default pool, or caller synchronization; individual scans are not concurrent.
The initial admitted profile is explicit: Linux/libstdc++9+ C++11 ABI, compatible static Zstandard, retained schema, positive numeric snapshot ID, local unpartitioned append-only bucket-unaware tables, matching file schema, version-2 ADD-only Avro manifests, null/Zstandard compression and Parquet/ORC files. Unknown decoded lengths use admitted streaming workspace/output growth. Unsupported contexts/profiles, DELETE entries, duplicates, wrong writer types and malformed inputs fail explicitly. Other platforms/old ABI/GCC8/shared Zstandard select a compiled
NotImplementedfactory; ordinary scans retain their existing path. System-static Zstandard must pass a current-header/library API link probe, refreshed on reconfiguration, and runtime header/library versions must match.Generic Avro/Arrow decoding does not route all variable storage through admission. This decoder uses budget-backed parser/PMR storage and static Zstandard workspace. Existing native publication constructors receive pre-admission based on qualified GNU string/vector/shared-pointer allocation behavior. The path is synchronous, emits ordinary native splits, and omits shared metadata caching and pruning statistics.
Related: sirius-db/sirius#2211 and polardb/duckdb-paimon#145. This independent native PR does not change the bridge or depend on cancellation PRs.
Tests
Final reviewed head:
f6acf07c3597fe56377d10d275f837d817138e5a.id-projection plan/read parity, independent row IDs, escaped-split reading, exact/insufficient/zero quotas, malformed and unsupported input rejection, and failure injection through every tested arena/publication allocation. ORC TIME decoding is an existing excluded reader limitation.operator newrequests with long-string calibration; static test runtime includes out-of-line libstdc++ constructors. Four earlier assertion-specific mutations catch missing string admission, quota enforcement, JSON failure draining and writer-schema validation; affected logic remains unchanged in the final lint follow-up.-Wno-error=sign-comparefor an unchanged mainline warning inserialization_utils.h:85. The vanilla GCC9-Werrorbuild stops at that pre-existing warning and is not claimed to pass.Reproduction entry points: build
paimon_sharedandpaimon-bounded-table-scan-test, run the registeredctest -R '^paimon-bounded-table-scan-test$' --output-on-failure, and runpre-commit run --all-files. Direct test runs requirePAIMON_TEST_DATApointing attest/test_data/with a trailing slash.CI scope: exact-head fork pre-commit and GCC8 passed. The GCC8 job executed 40 CTest targets, including the explicit unsupported-factory/ordinary-plan target. In Build and Test, completed ARM64 Clang/GNU Release/Debug and ASan+UBSan rows each passed 41 CTest targets; ARM64 TSan passed 39, and x86_64 TSan passed 40. Each included the bounded-planning target. The x86_64 GNU Release/Debug, Clang Release and ASan+UBSan rows have also completed: each passed 42 CTest targets including bounded planning, with exact-head checkout and actual logs independently audited. All twelve native matrix job results now pass. The corrected x86_64 Clang Debug job in attempt 2 passed all 42 CTest targets, including bounded planning, and the actual check-clang-tidy target. Its complete job log and exact f6 checkout were independently audited. Eleven earlier successful job results were carried forward; only Clang Debug was rerun. The first Clang Debug attempt passed 42 CTest targets but failed lint on logging.cpp, which is unchanged by this PR. Fork main was normally fast-forwarded from stale 242dec to actual upstream base a397 before the rerun, correcting origin/main comparison provenance without changing PR source or lint rules. That original failure remains historical evidence. These are CTest target counts, not individual GTest case counts. The existing workflow uses recoverable UBSan settings and retains no per-test artifacts, so instrumented job success is not an unqualified UBSan-clean claim; strict local final-head sanitizer evidence is separately recorded above. All four upstream PR-event workflows now pass on unchanged f6: Build and Test, GCC8, pre-commit and License. Independent review verified the actual PR merge/head identical tree, all twelve native matrix jobs plus script-tests and their successful steps without skips. Complete selected native logs confirm x64 Clang Release 42/42 targets and ARM Clang/GNU Release 41/41, each including bounded planning; complete GCC8 logs confirm 40/40 targets, and all nine pre-commit hooks passed. Nine other native terminal logs were not fully downloaded due network limits; their successful execution is verified by completed job metadata and the unchanged workflow, without claiming retained per-case reports. Maintainer code review/merge remains pending. Full old-ABI runtime and a fresh full SYSTEM build are NOT RUN. No current CI failure remains; maintainer review and merge remain separate.
API and Format
Add exported
PlanningBudgetand an additive static scan factory. Existing interfaces and persisted formats are unchanged. Native plan/split outputs use existing data readers. Reading, metrics, serialization and caller copies after planning require their own allocation policy.Documentation
Add the bounded-planning guide covering contract, admitted/rejected profile, ownership/concurrency, allocation qualification, limitations and usage.
Generative AI tooling
Generated-by: OpenAI Codex (GPT-6)