Make the VTab interface safe #416
Conversation
This will prevent the macro generating a warning in edition 2024
Contrary to the previous docs, the instances passed to these functions are *not* initialized by the caller. Rather, the called function is responsible for writing into uninitialized memory.
Rather than passing a pointer to a block of uninitialized memory, which can easily lead to UB, these functions now just return Rust objects. This improves duckdb#414 by reducing the amount of unsafe code needed from extensions.
BindInfo and InitInfo will be dropped in the usual way when freed by duckdb core. Any necessary destructors can be in Drop impls.
It's not completely clear but it looks like the engine could run the table fn from multiple threads, so requiring this seems safer
|
Cool, I think this make sense. Although I imagine we could pass the init state to the callback already wrapped in an RwLock just to be extra safe instead of having the user deal with synchronization. But I think this is fine for now as it is already an improvement. |
Yep, there is more that we could do; I think ideally the whole interface would be safe including with regard to multithreading from C++. I just didn't want to sink too much time before confirming that it could in principal be merged, and in particular that dropping the |
With this PR, simple table function extensions no longer need any unsafe code, and so are easier to implement and less likely to crash the process.
This partially solves #414.
I have not yet converted every interface to be safe, but if you like the general approach then I could go on to update everything in
crates/duckdb/src/vtab/function.rs.This is an API break for Rust extensions. In my opinion it's worth it because the existing API is easy to misuse and it would be better for callers to use a safe interface. It would not be impossible to preserve the existing unsafe interface, but I think it would leave a fair amount of complexity both in the implementation and in the user-facing API.
Rather than passing a pointer to a block of uninitialized memory, which can easily lead to UB, the setup functions now just return Rust objects.
The
Freetrait is no longer required, as the Rust type will now just be dropped. If extensions want custom behavior on drop, such as closing a connection, they can do that fromimpl Drop.It looks like the core DuckDB engine will, in at least some cases, run the table function from multiple threads, so to make that safe I've made the references always shareable. Extensions can use mutexes or atomic types internally as desired.
This builds on and goes further than #415.