feat: per-worker proxy for table function execute callback - #1367
Conversation
Wire the table execute path to per-worker proxy threads on DuckDB >= 1.5.0. A local_init callback registered via duckdb_table_function_set_local_init runs once per worker thread, creates a proxy (allocating its Ruby thread under the GVL through the global executor, since local_init runs on a non-Ruby thread), and stores it as thread-local init data via duckdb_init_set_init_data. The execute callback retrieves that proxy with duckdb_function_get_local_init_data and dispatches through it via rbduckdb_function_executor_dispatch_via_proxy, so callbacks from different workers run concurrently instead of serializing on the single global executor. bind and init stay on the global executor. DuckDB frees each proxy through rbduckdb_worker_proxy_destroy. The proxy-creating wrapper runs rbduckdb_worker_proxy_create under rb_protect, implementing the raise contract documented on that function: the executor runs callbacks unprotected, so an uncaught raise would longjmp past its done-signaling and block the waiting DuckDB worker forever. On failure the proxy stays NULL and the execute callback falls back to the global executor. On DuckDB < 1.5.0 the local_init hook is absent and the execute callback keeps using the global executor unchanged. The added test records which Ruby threads run the execute callback and asserts more than two distinct threads, which the old implementation can never produce (calling thread plus the single global executor), in addition to result correctness. Verified to fail against a build without this change. Simultaneity assertions are avoided as scheduler-dependent. sample/issue1136.rb gains a table UDF section: a GVL-releasing emitter with both planner hints (set_cardinality, max_threads) that demonstrates the throughput win (locally about 4x at SET threads=4, execute callbacks on 4 distinct Ruby threads instead of 1).
📝 WalkthroughWalkthroughThis PR enables DuckDB ≥1.5.0 per-worker proxy dispatch for table-function execute callbacks. It adds thread-detection, registers a per-worker local-init callback that creates a worker proxy on non-Ruby threads, and modifies the callback dispatch to retrieve and use that proxy. The changes include an integration test validating multi-thread execution and sample benchmarks demonstrating the feature for both scalar and table UDFs. ChangesPer-Worker Proxy Table Function Dispatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)ext/duckdb/table_function.cIn file included from ext/duckdb/table_function.c:1: ... [truncated 743 characters] ... r-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sample/issue1136.rb (1)
79-87: 💤 Low valuePotential off-by-one: decrement-then-check may emit one fewer chunk than intended.
(remaining -= 1) >= 0decrements first, so whenremaining=1, it becomes0and emits; whenremaining=0, it becomes-1and stops. This yields exactlyCHUNKSiterations, which is correct. However, this pattern is subtle—consider a comment or usingremaining.positive?before decrement for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sample/issue1136.rb` around lines 79 - 87, The emitter_execute_block method's decrement-then-check on remaining is subtle; update emitter_execute_block so the mutex-synchronized block checks remaining.positive? before decrementing (or atomically decrement after confirming >0) to make intent explicit, or at minimum add a clarifying comment next to the (remaining -= 1) >= 0 expression; refer to the emitter_execute_block function, the remaining variable and the mutex.synchronize block when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@sample/issue1136.rb`:
- Around line 79-87: The emitter_execute_block method's decrement-then-check on
remaining is subtle; update emitter_execute_block so the mutex-synchronized
block checks remaining.positive? before decrementing (or atomically decrement
after confirming >0) to make intent explicit, or at minimum add a clarifying
comment next to the (remaining -= 1) >= 0 expression; refer to the
emitter_execute_block function, the remaining variable and the mutex.synchronize
block when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4817496d-0a96-4f23-939c-fe4ebf74ff6a
📒 Files selected for processing (3)
ext/duckdb/table_function.csample/issue1136.rbtest/duckdb_test/table_function_test.rb
2 participants
GitHub: close GH-1136
Last step of the per-worker proxy work: on DuckDB >= 1.5.0, table function
execute callbacks now run on one proxy thread per DuckDB worker, same as scalar
functions since GH-1366. bind and init stay on the global executor — they run
once per query, not per worker, so there is no per-worker hook and nothing to
parallelize.
Why
Same ceiling as the scalar case: with one global executor, execute callbacks
from different workers can never overlap, even when they release the GVL.
Measured with the table section added to
sample/issue1136.rb(GVL-releasingemitter, 200 chunks):
The before run caps at 2 threads (calling thread + global executor) no
matter how many workers DuckDB spawns. Note the emitter needs both planner
hints (
set_cardinalityin bind,max_threads=in init) — without themDuckDB assigns a single worker and nothing distributes.
Notes for review:
duckdb_table_function_set_local_initis a1.5.0 API, so everything is behind
HAVE_DUCKDB_H_GE_V1_5_0.threads. The old path structurally caps at two, so the test fails without
this change. Timing/simultaneity assertions were deliberately avoided as
scheduler-dependent.
Summary by CodeRabbit
New Features
Tests