sdk: typed prerun/postrun#551
Conversation
42a3436 to
787481d
Compare
787481d to
1363ce3
Compare
330a160 to
711fc10
Compare
| vef_vdf_result_t *result, | ||
| std::index_sequence<Is...>) { | ||
| using Params = typename FuncParamTypes<decltype(Func)>::type; | ||
| State &state = *static_cast<State *>(args->user_data); |
There was a problem hiding this comment.
A VDF whose first parameter is State& or void* can be registered without .prerun(), but this wrapper unconditionally dereferences args->user_data. That makes the new public SDK shape a runtime null-deref footgun. Can we reject state-parameter signatures unless a prerun/state setup is attached, or defer this shape until the managed .state<T>() API exists?
There was a problem hiding this comment.
Yes for State&; I believe not for void*.
For void*, it's plausible to imagine something like "I might not need user_data, so I won't write it in pre_run, but will in the per-row function".
| return PrerunArgType(&a_->arg_types[i]); | ||
| } | ||
|
|
||
| // Returns the serialized constant bytes if argument i is a literal, |
There was a problem hiding this comment.
PrerunArgs::const_at() is exposed/documented as returning literal argument bytes, but the server currently initializes vef_prerun_args_t::const_values and const_lengths to nullptr in vdf_handler.cc, so this API always returns std::nullopt. Either populate those arrays in prerun setup, or hide/remove const_at() until it is implemented.
|
|
||
| namespace vsql { | ||
|
|
||
| // ============================================================================= |
There was a problem hiding this comment.
Repo guidance says not to add section-separator comments like this. Can we remove these separator blocks from the new header?
|
Dan, what do you think about something like this: where SDK manages the new/delete? user: SDK: Then the SDK-generated raw prerun does this order: And SDK-generated raw postrun does: |
| // or anything that doesn't fit emplace_state<T>. The wrapper forwards | ||
| // args->user_data straight through. | ||
| // | ||
| // Param tuple shape: <void*, TypedArg..., ResultWrapper>. Same indexing as |
There was a problem hiding this comment.
How come State (the type above) can't just be void* in this case? Does something break in the casting?
There was a problem hiding this comment.
WrapperVoidState is importantly different because it does not deref args->user_data.
As I said to tomas's comment:
For void*, it's plausible to imagine something like "I might not need user_data, so I won't write it in pre_run, but will in the per-row function".
a624b5c to
4c7d041
Compare
Yes, I want to do this; I'm just trying to keep the change smaller. In fact, I think we can do even better and just pass along a function to delete instead of a fully-general postrun function altogether. |
| } | ||
| }; | ||
|
|
||
| // Thunks that convert a typed user prerun/postrun (void(PrerunArgs, |
There was a problem hiding this comment.
I know "Thunk" is a term of art, but we have never used that term before; why introduce it now?
There was a problem hiding this comment.
Changed to Wrapper
There was a problem hiding this comment.
Please don't add anything to this file. This is the v1 API that we are trying to get rid of.
4c7d041 to
02be86e
Compare
NB: this is analogous to #254 , but rebased and simplified sdk: typed varargs + required explicit arity Introduce typed C++ wrappers for varargs VDFs (vsql::VarArgs, vsql::AnyArg) and the matching .varargs() / zero-arity .param() builder methods, so an extension author can write a variadic SQL function without touching raw ABI types. Also tightens the v2 builder to require an explicit arity declaration before .build(). What changes for users Before, an extension author who wanted a varargs SQL function had to drop out of the typed SDK and write a raw-ABI body — the typed wrappers covered fixed-arity only: // Old: raw vef_vdf_func_t signature, manual invalue extraction void ba_concat_all(vef_context_t *ctx, vef_vdf_args_t *args, vef_vdf_result_t *result) { for (unsigned int i = 0; i < args->value_count; i++) { vef_invalue_t v = vsql::func_builder::get_invalue(ctx, args, i); if (v.is_null) { result->type = VEF_RESULT_NULL; return; } memcpy(result->str_buf + i * N, v.bin_value, N); } result->type = VEF_RESULT_VALUE; result->actual_len = args->value_count * N; } After: // New: typed VarArgs + AnyArg, no raw ABI in the body void ba_concat_all(vsql::VarArgs args, vsql::StringResult out) { auto dst = out.buffer(); size_t off = 0; for (auto a : args) { if (a.is_null()) { out.set_null(); return; } auto bytes = a.as_custom(); memcpy(dst.data() + off, bytes.data(), bytes.size()); off += bytes.size(); } out.set_length(off); } Registration picks up .varargs() (and zero-arity .param()): .func(make_func<&ba_len>("ba_len") .returns(INT) .param() // explicit zero-arity .build()) .func(make_func<&ba_concat_all>("ba_concat_all") .returns(STRING) .varargs() .prerun<&ba_concat_all_prerun>() .build()) The prerun hook is still raw ABI in this PR; the typed variant lands with #551 (typed vsql::PrerunArgs / PrerunResult), and ba_concat_all_prerun will convert to typed form when that merges. Required-arity change The v2 builder now rejects make_func<&fn>("name").returns(...).build() without an arity call. You must pick exactly one of: .param(TYPE) (one or more, for fixed-arity typed args) .param() (zero-arity) .varargs() Eight in-tree test-extensions had implicit zero-arity functions and pick up an explicit .param() in this PR. The v1 builder (villagesql::func_builder, raw-ABI) is unchanged — that cow has left the barn.
Introduce typed C++ wrappers for the per-statement lifecycle hooks
(prerun and postrun). Extension authors no longer touch raw ABI
structs to write these hooks; only the typed signatures are accepted.
Why
---
Extension authors writing prerun/postrun previously had to:
void my_prerun(vef_context_t *, vef_prerun_args_t *args,
vef_prerun_result_t *result) {
if (args->arg_count == 0) {
result->type = VEF_RESULT_ERROR;
snprintf(result->error_msg, VEF_MAX_ERROR_LEN, "...");
return;
}
result->result_buffer_size = N;
result->user_data = new MyState{};
}
Two problems: raw ABI in the user-facing API (against the SDK's
direction; see #548) and bespoke error-reporting plumbing.
After this PR:
void my_prerun(vsql::PrerunArgs args, vsql::PrerunResult out) {
if (args.size() == 0) {
out.error("at least one argument required");
return;
}
out.request_buffer_size(N);
out.set_user_data(new MyState{});
}
void my_postrun(vsql::PostrunArgs args) {
args.delete_state<MyState>();
}
void my_vdf(MyState &state, IntResult out) { ... }
State lifetime stays explicit in this PR: prerun stashes the pointer,
postrun frees it. A follow-up will add auto-cleanup via .state<T>()
(implementable without ABI changes; see TODO in pre_post_run.h).
Where to start reviewing
------------------------
Read the layers in this order:
1. villagesql/sdk/include/villagesql/vsql/pre_post_run.h
New public types: PrerunArgs, PrerunArgType, PrerunResult,
PostrunArgs. This is what extension authors see.
2. villagesql/examples/vsql-simple/src/extension.cc
The ba_call_index demo: set_user_data in prerun, mutable State&
in the VDF, delete_state in postrun.
3. mysql-test/suite/villagesql/extension/t/extension_simple_type_usage.test
What it looks like at the SQL surface.
4. villagesql/sdk/include/villagesql/extension.h
Refreshed user-facing docs for the typed hook shape.
The remaining file is SDK plumbing:
5. villagesql/sdk/include/villagesql/vsql/func_builder.h
- .prerun<>() / .postrun<>() are typed-only; static_assert
rejects raw vef_prerun_func_t / vef_postrun_func_t.
- FuncBuilder gained a HasPrerun template flag; .prerun<>() flips
it true, and build() static_asserts that void(State&,...) and
void(void*,...) signatures require it. Registering a state-style
VDF without a prerun is a compile error, not a runtime null deref.
- WrapperTypedState / WrapperVoidState dispatch on the first
param of the VDF: State& / const State& / void* selects how
user_data is forwarded.
- typed_prerun_thunk / typed_postrun_thunk adapt the user's
typed signature to the raw ABI shape the server invokes.
Not in this PR
--------------
- A `.state<T>()` builder that records State at the template level
and routes typed signatures of the form `void(T&, ...)`. The SDK
would install both prerun and postrun thunks to manage the typed
state's lifetime via the existing user_data slot — no ABI side
channel needed. Will land as a follow-up.
- Typed VarArgs/AnyArg wrappers for varargs VDFs (the other raw-ABI
place in extension code, only reached via PR #254).
- vef_postrun_result_t is empty in the ABI today, so there is no
PostrunResult wrapper. Will add when the struct grows.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
267bb9e to
5dbadce
Compare
5dbadce to
13a8452
Compare
sdk: typed prerun/postrun hooks
Introduce typed C++ wrappers for the per-statement lifecycle hooks
(prerun and postrun). Extension authors no longer touch raw ABI
structs to write these hooks; only the typed signatures are accepted.
Why
Extension authors writing prerun/postrun previously had to:
void my_prerun(vef_context_t *, vef_prerun_args_t *args,
vef_prerun_result_t *result) {
if (args->arg_count == 0) {
result->type = VEF_RESULT_ERROR;
snprintf(result->error_msg, VEF_MAX_ERROR_LEN, "...");
return;
}
result->result_buffer_size = N;
result->user_data = new MyState{};
}
Two problems: raw ABI in the user-facing API (against the SDK's
direction; see #548) and bespoke error-reporting plumbing.
After this PR:
void my_prerun(vsql::PrerunArgs args, vsql::PrerunResult out) {
if (args.size() == 0) {
out.error("at least one argument required");
return;
}
out.request_buffer_size(N);
out.set_user_data(new MyState{});
}
void my_postrun(vsql::PostrunArgs args) {
args.delete_state();
}
void my_vdf(MyState &state, IntResult out) { ... }
State lifetime stays explicit in this PR: prerun stashes the pointer,
postrun frees it. A follow-up will add auto-cleanup (see "Not in this
PR" below).
Where to start reviewing
Read the layers in this order:
villagesql/sdk/include/villagesql/vsql/pre_post_run.h
New public types: PrerunArgs, PrerunArgType, PrerunResult,
PostrunArgs. This is what extension authors see.
villagesql/examples/vsql-simple/src/extension.cc
The ba_call_index demo: set_user_data in prerun, mutable State&
in the VDF, delete_state in postrun.
mysql-test/suite/villagesql/extension/t/extension_simple_type_usage.test
What it looks like at the SQL surface.
villagesql/sdk/include/villagesql/extension.h
Refreshed user-facing docs for the typed hook shape.
The remaining file is SDK plumbing:
rejects raw vef_prerun_func_t / vef_postrun_func_t.
param of the VDF: State& / const State& / void* selects how
user_data is forwarded.
typed signature to the raw ABI shape the server invokes.
Not in this PR
(e.g. a user_data_deleter slot on vef_prerun_result_t) so the SDK
can register a destructor independent of the user_data pointer
that extensions also use for raw/custom allocations. Marked
TODO(villagesql-beta) in pre_post_run.h.
.state<T>()builder that records State at the template leveland routes typed signatures of the form
void(T&, ...). Will landwith the auto-cleanup mechanism above.
place in extension code, only reached via PR sdk: prerun+varargs #254).
PostrunResult wrapper. Will add when the struct grows.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com