Add support to updatable virtual tables #1141
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1141 +/- ##
==========================================
+ Coverage 78.21% 78.89% +0.68%
==========================================
Files 47 48 +1
Lines 5927 6028 +101
==========================================
+ Hits 4636 4756 +120
+ Misses 1291 1272 -19
Continue to review full report at Codecov.
|
|
I don't understand why 6d885c8 doesn't compile. |
|
We might have to explicitly put it as a const or something, and return a reference to that. Const promotion doesn't work that reliably. It's possible that keeping this const isn't viable also. |
| pub fn read_only_module<'vtab, T: CreateVTab<'vtab>>() -> &'static Module<'vtab, T> { | ||
| // The xConnect and xCreate methods do the same thing, but they must be | ||
| // different so that the virtual table is not an eponymous virtual table. | ||
| macro_rules! module { |
There was a problem hiding this comment.
Oh, good idea for how to do this.
| @@ -1153,6 +1203,49 @@ where | |||
| } | |||
| } | |||
|
|
|||
| unsafe extern "C" fn rust_update<'vtab, T: 'vtab>( | |||
There was a problem hiding this comment.
After this lands I'm going to do a follow-up to fix FFI-panics in some code (including this).
Our current code doesn't handle it correctly either, FWIW.
| ) -> Result<(String, VTabLog)> { | ||
| static N_INST: AtomicUsize = AtomicUsize::new(1); | ||
| let i_inst = N_INST.fetch_add(1, Ordering::SeqCst); | ||
| println!( |
There was a problem hiding this comment.
It's definitely not okay for us to println! for stuff like this. Even eprintln! is dodgy since it will panic if the stderr is closed (for example, inside a daemon). I've vaguely wanted rusqlite to pull in the log crate for a while now, and this seems like a reasonable place to start.
Edit: Never mind, this is behind #[cfg(test)]. I didn't notice it. I think it might be better as an example than a test, but this is fine.
| /// not used | ||
| /// | ||
| /// SQLite >= 3.9.0 | ||
| EponymousOnly, |
There was a problem hiding this comment.
We need to add a check for crate::version_number() >= 3009000 for attempts to register an EponymousOnly module, and return an Err(...).
Prior to 3.9.0 SQLite will crash on eponymous-only modules, because it doesn't check for xCreate being NULL.
There was a problem hiding this comment.
https://sqlite.org/vtab.html#eponymous_only_virtual_tables
Note that prior to version 3.9.0 (2015-10-14), SQLite did not check the xCreate method for NULL before invoking it. So if an eponymous-only virtual table is registered with SQLite version 3.8.11.1 (2015-07-29) or earlier and a CREATE VIRTUAL TABLE command is attempted against that virtual table module, a jump to a NULL pointer will occur, resulting in a crash.
A crash seems safe to me.
There was a problem hiding this comment.
Crash from null pointer dereference is undefined behavior. Absolutely not safe. Just because it is deterministic and is not exploitable does not mean it's memory safe.
| @@ -57,6 +57,19 @@ use crate::{str_to_cstring, Connection, Error, InnerConnection, Result}; | |||
| // ffi::sqlite3_vtab => VTab | |||
| // ffi::sqlite3_vtab_cursor => VTabCursor | |||
|
|
|||
| /// Virtual table kind | |||
There was a problem hiding this comment.
I think this needs better docs. Can you use this? Do a quick read-through of it to make sure I didn't make any obvious mistakes, also.
/// Virtual table kind, indicating whether or not the table is
/// [eponymous](https://sqlite.org/vtab.html#eponymous_virtual_tables).
#[non_exhaustive]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum VTabKind {
/// Non-eponymous tables, which are the most flexible.
Default,
/// Eponymous tables exist implicitly in the main schema of the database,
/// and do not require setup such as `CREATE VIRTUAL TABLE` to be used.
///
/// These should implement [`CreateVTab::create`] and [`VTab::connect`]
/// identically. (Which is the default behavior if [`CreateVTab::create`] is
/// not explicitly overridden)
Eponymous,
/// Eponymous-only tables are tables which may only be used eponymously, and
/// cannot be used with `CREATE VIRTUAL TABLE`.
///
/// This is often useful for table valued functions, although note that this
/// prevents use as a temporary table, in patterns like the following:
///
/// ```sql
/// CREATE VIRTUAL TABLE temp.something USING my_vtab(args, here);
/// ```
///
/// But whether or not this pattern makes sense to support depends on the
/// implementation of your table.
///
/// Eponymous-only tables will not have the [`CreateVTab::create`] and
/// [`CreateVTab::destroy`] methods called. [`VTab::connect`] will be used
/// instead (and cleanup will be performed by [`Drop::drop`]).
///
/// If the SQLite version is older than 3.9.0, `rusqlite` will produce an
/// error when you attempt to register a `EponymousOnly` virtual table
EponymousOnly,
}
Fix #730, #900
TODO
[X] simple implementation at least for test / example.
[X] eponymous updatable VTab