Skip to content

feat(scan): serve pinned entries through the split_provider machinery via a pinned-serving mode on the format ingestibles (#819 PR2) - #1126

Closed
bwyogatama wants to merge 1 commit into
sirius-db:devfrom
bwyogatama:feat/duckdb-mvcc-pr2-serving-reorg
Closed

bwyogatama wants to merge 1 commit into
sirius-db:devfrom
bwyogatama:feat/duckdb-mvcc-pr2-serving-reorg

Conversation

@bwyogatama

Copy link
Copy Markdown
Collaborator

Part of #819 (query-time MVCC delta merge for pinned caches) — the serving reorg that PR3 (DELETE masks) and PR4 (INSERT delta) build on. Behavior-identical: no MVCC logic in this PR.

What

Pinned/cached-entry serving moves off its dedicated coalescer-direct path and onto the same split_provider / metadata-task machinery disk scans use. On a pin hit, prepare_for_query switches the operator's format ingestible into pinned-serving mode (gpu_ingestible::serve_from_pinned_chunks, default-false virtual) and the plain, unmodified split_provider fans the resident chunks out as work items on the scan manager's dispatcher threads. The serving tail is now uniform — cached and disk scans both live in _providers_by_op and are driven by start_metadata_processing; the entire databatch_provider / use_cached_entries_for_pipeline / process_cached_entries bypass is deleted.

Pieces

  • pinned_chunk_source (op/scan): self-contained chunk inventory built by the scan manager from a matched pinned entry (shared_ptr copies, no reference into the pinned-entries map); lock-free chunk claim + per-chunk resident batch assembly (GPU column wrap stamped with the chunk's memory_space / HOST slice), with per-batch telemetry attribution (Add data_batch telemetry #1068's quent_data_batch_probe) preserved.
  • cached_scan_info: the format-neutral resident-split carrier. The sequencer's emit path unwraps it into a resident scan_operator_input and skips the balancer call, so task_creator keeps routing pinned scans by the batch's memory_space (home-GPU / NUMA locality) and the shared round-robin cursor stays untouched for disk pipelines in mixed queries.
  • cached_batch_coalescer: pass-through slot coalescer — pinned chunks were already batch-sized at pin time. flush() emits nothing, so a zero-chunk pin still closes its connector with zero splits.
  • Format ingestibles: duckdb_native_gpu_ingestible and parquet_gpu_ingestible each gain the override plus three one-line pinned-mode forks in has_processed_all_metadata / next_split_provider / create_batch_coalescer.
  • build_pinned_chunk_source validates the entry shape and throws (caught → disk fallback) where the old path silently truncated the scan mid-stream; work-item exceptions now surface as clean query errors (try_tconnector->close(exception)) instead of an unclosed-connector hang.

Why this shape (for #819)

MVCC is DuckDB-specific, so the serving seam is deliberately placed such that all MVCC work lands in duckdb_native_gpu_ingestible's pinned mode in PR3/PR4 — visibility work items on dispatcher threads, a mask-join coalescer variant, and delta splits decoded by its own materialize_metadata_to_table — next to the DuckDB table handles, serial-prep discipline, and decode machinery it needs. The shared pieces (pinned_chunk_source, cached_scan_info, the pass-through coalescer) stay format-neutral and MVCC-free permanently; parquet's pinned mode is trivial forever. This also follows the IO-framework refactor's direction: split_provider stays a concrete class composing gpu_ingestible (no provider subclasses).

Intentional divergences (all edge-case, all strictly better)

  • A work-item exception used to be swallowed (connector never closed → query hang, later pipelines starved); it now propagates as a query error.
  • A malformed pinned entry (uneven per-column chunk counts, short chunk_memory_spaces, null host chunk) used to truncate the scan silently; it now throws in the builder and falls back to the disk read.
  • Chunk emission order within one scan is completion-order instead of index-order (splits are independent tasks; cross-pipeline order is still sequencer-enforced).
  • Cached serving is now stop-token-aware during teardown.

Verification

  • New [pinned_chunk_source] gates (9 cases, 130 assertions): source claim/assembly with a permuted column subset, per-chunk memory_space preservation, HOST slice, zero-chunk close, builder validation throws, coalescer pass-through/foreign-drop/empty-flush, the unmodified split_provider::run driving pinned work items on pool threads with the end-of-stream sentinel, and an end-to-end pin → SELECT gate asserting the pinned-serving log marker.
  • Behavior-identical suites pass unchanged: [pin_table], [pin_table_host], [pin_table_duckdb], [pin_table_duckdb_host], [pin_table_cols_subset], [pin_table_host_streaming] (GPU high-water preserved), [host_serve] (served-from-pin-not-disk proof), merge and column-order regressions, [can_serve].
  • Disk-path regressions: [duckdb_native], [overflow_string], [parquet_schema_mapping], [scan][batch_coalescer].

Draft until the multi-GPU gates ([pin_mgpu]) run on a 2-GPU box (this branch was validated on a single-GPU GB10).

🤖 Generated with Claude Code

… via a pinned-serving mode on the format ingestibles (sirius-db#819 PR2)

Pinned/cached-entry serving moves off the coalescer-direct path
(databatch_provider / use_cached_entries_for_pipeline /
process_cached_entries, all deleted) and onto the same
split_provider/metadata-task machinery disk scans use: on a pin hit,
prepare_for_query switches the operator's format ingestible into
pinned-serving mode (gpu_ingestible::serve_from_pinned_chunks) and the
plain, unmodified split_provider fans the resident chunks out as work
items on the scan manager's dispatcher threads.

Pieces:
- pinned_chunk_source (op/scan): self-contained chunk inventory built by
  the scan manager from a matched pinned entry (shared_ptr copies, no
  entry reference); lock-free chunk claim + per-chunk resident batch
  assembly (GPU column wrap with the chunk's memory_space / HOST slice).
- cached_scan_info: the format-neutral resident-split carrier; the
  sequencer's emit path unwraps it into a resident scan_operator_input,
  skipping balancer stamping so task_creator keeps routing pinned scans
  by the batch's memory_space.
- cached_batch_coalescer: pass-through slot coalescer (pinned chunks were
  batch-sized at pin time); flush emits nothing so zero-chunk pins still
  close with zero splits.
- duckdb_native/parquet ingestibles: three one-line pinned-mode forks
  each. MVCC serving (issue sirius-db#819 PR3/PR4) extends duckdb-native's pinned
  mode only — visibility work items, mask join, delta staging — next to
  the DuckDB handles and decoder it needs; parquet's stays trivial.
- build_pinned_chunk_source validates entry shape and throws (caught ->
  disk fallback) where the old direct path silently truncated; work-item
  exceptions now surface as clean query errors instead of hangs.

Behavior-identical: all pin suites ([pin_table], host, duckdb, cols
subset, host streaming, host_serve, merge, column order), [duckdb_native],
[overflow_string] and [parquet_schema_mapping] pass unchanged; new
[pinned_chunk_source] gates cover the source, the validation, the
coalescer, plain-provider composition on pool threads, and an end-to-end
pin+SELECT marker.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bwyogatama
bwyogatama marked this pull request as ready for review July 8, 2026 22:43
@bwyogatama
bwyogatama marked this pull request as draft July 8, 2026 23:28
@bwyogatama

Copy link
Copy Markdown
Collaborator Author

Closing in favor of keeping dev's coalescer-direct pinned serving (design revision, 2026-07-08).

The MVCC CPU work for #819 (PR3 delete-visibility masks, PR4 insert-delta staging) will instead run as ad-hoc prepare-time tasks on the scan manager's dispatcher — launched from prepare_for_query right after try_assign_cached_entries detects a pin hit, and joined before start_metadata_processing(), so masks are finished buffers before serving begins. At that point the dispatcher is fresh and idle, and it already accepts ad-hoc tasks (spawn_workers precedent) — this reorg bought parallelism the dispatcher offers directly, at the cost of migrating a proven serving path and adding a pinned-serving mode to every format ingestible. A second constraint reinforced the pivot: the coalescer's single serial sequencer can never wait on masks (head-of-line stall across all scan ops), so gating emission at the coalescer stage bought nothing over joining in prepare.

The three cached-path hardening fixes that were entangled in this reorg (silent query hang on provider throw, silent truncation on malformed pinned entries, ignored stop token) land separately as a small PR2-lite against dev — see #819's updated PR ladder.

Design record: #819 (updated). This branch stays frozen as the historical record of the serving-reorg approach.

🤖 Generated with Claude Code

@bwyogatama bwyogatama closed this Jul 9, 2026
pull Bot pushed a commit to muzammilar/sirius that referenced this pull request Jul 9, 2026
…te) (sirius-db#1136)

Part of sirius-db#819 (**PR2-lite** — replaces the serving reorg previously
drafted in sirius-db#1126, closed unmerged: the MVCC CPU work moves to
prepare-time dispatcher jobs and the serving path stays
coalescer-direct; see sirius-db#1126's closure comment and sirius-db#819's updated
design).

## Summary

Three pre-existing bugs in the pinned-table cached serving path — they
affect **all** pinned serving today (parquet + duckdb, GPU + host
tiers), independent of MVCC:

- **Silent query hang on provider failure.** `process_cached_entries`
had no try/catch: a throwing `databatch_provider` escaped into the
dispatcher (which swallows task exceptions), the operator's
`split_connector` never closed, and the consumer blocked in
`get_next_split()` forever. The drain is restructured into a testable
public static
`load_balancing_scan_batch_coalescer::drain_cached_provider(provider,
connector, stop)` whose `catch(...)` does
`connector.close(current_exception())` — the consumer now rethrows a
clean query error, mirroring `process_provider_inputs`' hardening on the
disk path.
- **Silent truncation on malformed entries.** The cached provider
returns `nullptr` mid-stream when a selected column has fewer chunks
than the others, `chunk_memory_spaces` is short/null, or a chunk is null
— indistinguishable from end-of-stream, so the query completed on fewer
rows than requested with no error. New
`validate_pinned_entry_for_serving` runs in `try_assign_cached_entries`
before the provider attaches: a malformed entry throws, the existing
catch logs the reason (`e.what()` was previously dropped) and falls back
to the disk read.
- **Ignored stop token.** The cached drain took its stop token
`[[maybe_unused]]`; it now stops between batches and still closes the
connector so consumers unblock during teardown.

Drive-by: the coalescer header's stale slot-storage comment
(`unique_ptr` → actual `shared_ptr`, which the bridge lambda relies on).

**Deliberately not fixed** (documented in sirius-db#819 → Known hazards): the
unpin-mid-query dangling reference (`cached_databatch_provider` holds
`const pinned_entry&`; `_pinned_entries` is mutated by `unpin_table`
without the query-lifecycle mutex) — a separate follow-up.

## Test plan

- [x] New `[cached_serving]` suite (5 cases, 24 assertions): drain
forwards-then-closes; provider throw → consumer rethrows (no hang);
pre-stopped token → closed without draining; validator accepts
well-formed + zero-chunk entries and refuses six malformed shapes
(chunk-count mismatch, missing selected column, short/null memory
spaces, null GPU/host chunks).
- [x] Pin regression suites pass unchanged: `[pin_table]` (394
assertions incl. the column-order regression), `[pin_table_host]`,
`[pin_table_duckdb]`, `[pin_table_duckdb_host]`,
`[pin_table_cols_subset]`, `[pin_table_host_streaming]`.
- [x] `pre-commit` clean on touched files.
- [ ] `[pin_mgpu]` on a 2-GPU box (kept as the undraft gate per
convention; note this change touches no placement logic — balancer
stamping and memory_space routing are untouched).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant