Expose sqlite's sqlite3_create_collation() #2495

z33ky · 2020-09-04T22:16:37Z

This PR adds diesel::sqlite::SqliteConnection::register_collation(), which wraps sqlite3_create_collation_v2() (ref https://sqlite.org/c3ref/create_collation.html).
The registered collations can be used via the COLLATE-constraint on table columns, as is demonstrated in the tests.

z33ky · 2020-09-04T22:18:07Z

diesel/src/sqlite/connection/raw.rs

+        let f = match user_ptr.as_mut() {
+            Some(f) => f,
+            None => {
+                //FIXME


I'm not really sure what to do here if sqlite passes us null pointers. Can it just be assumed that this will not happen (i.e. unwrap() here)?

It seems to me unreachable! in case of None could be appropriate.

diesel/diesel/src/sqlite/connection/raw.rs

Lines 307 to 309 in 44d0096

unreachable!(

"We've written the aggregator above to that location, it must be there"

)

It wouldn't be, since "A panic! across an FFI boundary is undefined behavior." (https://doc.rust-lang.org/nomicon/ffi.html#ffi-and-panics).

According to https://www.sqlite.org/c3ref/create_collation.html we can be sure that we get our pouter to the callback here. Following that reasoning I would use unreachable! or something similar + a comment explaining why this cannot happen to indicate that this is guaranteed by other things.

Then I'd either go to "full undefined behavior" territory and use std::hint::unreachable_unchecked(), or stay on the safe route and call std::process::abort() (with an eprintln!() in front).
IMO unreachable!() gives the impression that we can just panic and it's fine. Which it may eventually be (rust-lang/rust#74990), but at the moment it is not.

Using comment +eprintln! + abort() sounds like the better solution for me. Can you have a quick lock at the other callbacks for normal and aggregate functions and change that there as well?

Yes, will do!

z33ky · 2020-09-04T22:28:19Z

I should mention, my use-case is allowing for proper case-insensitive matching, since the NOCASE-collation available with sqlite only works with ASCII (ref §7 https://www.sqlite.org/datatype3.html).
Perhaps this use-case can be mentioned in the method-documentation?

weiznich

In general I'm in favor of this change. I've left some comments at places where I think things can be improved. Additionally such a change requires an entry to our changelog.

diesel/src/sqlite/connection/functions.rs

diesel/src/sqlite/connection/mod.rs

diesel/src/sqlite/connection/raw.rs

weiznich · 2020-09-08T09:44:47Z

diesel/src/sqlite/connection/raw.rs

+        let f = match user_ptr.as_mut() {
+            Some(f) => f,
+            None => {
+                //FIXME


According to https://www.sqlite.org/c3ref/create_collation.html we can be sure that we get our pouter to the callback here. Following that reasoning I would use unreachable! or something similar + a comment explaining why this cannot happen to indicate that this is guaranteed by other things.

diesel/src/sqlite/connection/raw.rs

z33ky · 2020-09-08T17:10:05Z

sqlite3_create_collation() can also remove collation function by passing NULL as callback. Shall I add a SqliteConnection::unregister_collation() for this?

weiznich · 2020-09-09T09:16:37Z

sqlite3_create_collation() can also remove collation function by passing NULL as callback. Shall I add a SqliteConnection::unregister_collation() for this?

The question is how often would such a function be used?

z33ky · 2020-09-09T18:45:24Z

The question is how often would such a function be used?

It guess it would only be of potential use when dropping tables or some such. It certainly won't find use from me (at least at the moment).
Can I gather from the inquisition that I shell leave it absent then?

z33ky · 2020-09-09T21:18:42Z

I hit a snag while making run_aggregator_step_function() unwind-safe: SqliteAggregateFunction::step() takes a &mut self. The general idea is that when step() panics, little to no care may be taken that self restores temporary invalid state (see https://doc.rust-lang.org/std/panic/trait.UnwindSafe.html#what-is-unwindsafe).
We can communicate that we expect the SqliteAggregateFunction to be UnwindSafe, but apparently &mut T is never considered UnwindSafe, even if T: UnwindSafe, (I'm guesing) since the object continues to exist after the panic.

We can wrap it in a AssertUnwindSafe, which means that we assure the type system about SqliteAggregateFunction's continue existence after panics on the user's behalf, or we can std::process::abort() as well if it panics.
We could add our own marker trait which the user has to implement. Implementing this trait would be a promise that it's fine to wrap in AssertUnwindSafe for the purpose of calling step().

weiznich · 2020-09-11T08:02:00Z

I think AssertUnwindSafe + a comment mentioning that the implementation is responsible for cleaning up stuff after a panic should be fine there.

z33ky · 2020-09-11T09:51:31Z

That should address all requested changes.
On top it also does some light refactoring of the aggregator function handling.
I don't know if reducing the scope of unsafe is desired since it adds a bit of "noise" to open a few more, but smaller, unsafe scopes than before. That's its own commit though and I can easily remove it if not desired.

weiznich

Looks really good, modulo some small stylistic things. Thanks for working on this 👍

diesel/src/sqlite/connection/raw.rs

weiznich · 2020-09-11T12:16:08Z

diesel/src/sqlite/connection/raw.rs

-    }
+    let args = unsafe { slice::from_raw_parts(value_ptr, num_args as _) };
+    let args = build_sql_function_args::<ArgsSqlType, Args>(args);
+    let mut aggregator = std::panic::AssertUnwindSafe(aggregator);


I had some time to think a bit more about this, so here some reasoning why this actually should be OK and users do not need to care about this:

We catch the panic and get a Err

We return an error message to sqlite, so that sqlite will abort executing this aggregate, so the old instance will not be used anymore.

On the next call to this aggregate function we will create a new instance of A, so we cannot have compromised state here either.

(Maybe add that as comment here)

From that point of view: I'm not sure if we really need to have bounds on UnwindSafe on the corresponding types.

Ah, I didn't really follow how the aggregator is initialized.
Unwind-safetly is still a useful property though because finalize() will be called whether or not step() errored out.

I'm not certain if having the UnwindSafe-bound actually does much in any case, since this AssertUnwindSafe is where the unwind-safety rubber hits the road. Though without it we'd need another AssertUnwindSafe-wrapping in run_aggregator_final_function() if the bound does not exist. And that may be relevant Drop impementations, even if we don't use the aggregator any further.

diesel/src/sqlite/connection/raw.rs

weiznich · 2020-09-11T12:20:16Z

diesel/src/sqlite/mod.rs

+    /// The `step()` method is called once for every record of the query.
+    ///
+    /// This is called through a C FFI, as such panics do not propagate to the caller. Panics are
+    /// caught and cause a return with an error value. The implementation must still ensure that


As explained above I think we do not need to have that actually here, because I see no way how the state could be reused here.

diesel/src/sqlite/connection/raw.rs

Panics across FFI boundaries cause undefined behavior in current Rust. The aggregator functions are callbacks invoked from C (libsqlite). To safe-guard against panics, std::panic::catch_unwind() is used. On panic the functions now return with an error result indicating the unexpected panic occurred. std::panic::catch_unwind() requires types to implement std::panic::UnwindSafe, a marker trait indicating that care must be taken since panics introduce control-flow that is not very visible. Refer to https://doc.rust-lang.org/std/panic/trait.UnwindSafe.html for a more detailed explanation. For SqliteAggregatorFunction::step() we must use std::panic::AssertUnwindSafe, since &mut references are never considered UnwindSafe, and the requirement to ensure unwind-safety is documented on the method. Of note is that in safe Rust, even if the method is not unwind-safe the language still guarantees memory-safety. The marker trait is mainly to prevent logic bugs.

z33ky · 2020-09-14T16:08:31Z

diesel/src/sqlite/connection/raw.rs

+        A: SqliteAggregateFunction<Args, Output = Ret>
+            + 'static
+            + Send
+            + std::panic::UnwindSafe


I've changed it to both RefUnwindSafe and UnwindSafe, since ::step() passes by reference and ::terminate() by value. As stated before, I'm not sure if the RefUnwindSafe-bound for ::step() provides any practical safety since we just use AssertUnwindSafe instead to pass the mut& across and RefUnwindSafe is only for non-mut references.

This encompasses the pattern of eprintln!() + std::process::abort() and adds a request to open an issue if the message is observed.

This is essentially the same treatment that custom aggregate functions got in ee2f792.

…ation

z33ky · 2020-09-14T19:50:54Z

The failed CI job looks to me like a transient failure; it seems MySQL didn't start up correctly there.

weiznich · 2020-09-15T12:43:50Z

diesel/src/sqlite/connection/raw.rs

@@ -204,7 +251,8 @@ extern "C" fn run_custom_function<F>(
 ) where
    F: FnMut(&RawConnection, &[*mut ffi::sqlite3_value]) -> QueryResult<SerializedValue>
        + Send
-        + 'static,
+        + 'static
+        + std::panic::RefUnwindSafe,


As this is a FnMut and not a Fn I'm would guess that RefUnwindSafe is not the right trait here. We do potentially mutate F here.

weiznich · 2020-09-15T12:43:54Z

diesel/src/sqlite/connection/raw.rs

+            }
+            Err(_) => {
+                let msg = format!("{} panicked", std::any::type_name::<F>());
+                unsafe { context_error_str(ctx, &msg) };


Is there any reason why we need a nested unsafe block here?

weiznich · 2020-09-15T12:47:29Z

diesel/src/sqlite/connection/raw.rs

+    context_error_str(ctx, NULL_AG_CTX_ERR)
+}
+
+unsafe fn context_error_str(ctx: *mut ffi::sqlite3_context, error: &str) {


We should cite https://sqlite.org/c3ref/result_blob.html here, stating that sqlite will copy our error message, so it's fine to free it afterwards. (Just to make that clear for any future person looking at this code.)
Also we maybe can take a NonNull<ffi::sqlite3_context> here to make this function not unsafe to use anymore?

weiznich · 2020-09-15T12:49:57Z

diesel/src/sqlite/connection/raw.rs

-    A: SqliteAggregateFunction<Args, Output = Ret> + 'static + Send,
-    Args: FromSqlRow<ArgsSqlType, Sqlite>,
+    A: SqliteAggregateFunction<Args, Output = Ret> + 'static + Send + std::panic::RefUnwindSafe,
+    Args: FromSqlRow<ArgsSqlType, Sqlite> + std::panic::UnwindSafe,


I'm not sure why the Args need to be UnwindSafe. They get reconstructed everytime the aggregator is called, so a potential panic cannot corrupt anything there.

weiznich · 2020-09-15T12:52:24Z

diesel/src/sqlite/connection/raw.rs

-    A: SqliteAggregateFunction<Args, Output = Ret> + 'static + Send,
-    Args: FromSqlRow<ArgsSqlType, Sqlite>,
+    A: SqliteAggregateFunction<Args, Output = Ret> + 'static + Send + std::panic::UnwindSafe,
+    Args: FromSqlRow<ArgsSqlType, Sqlite> + std::panic::UnwindSafe,


We do not even construct the arguments here, so UnwindSafe is not required here.

weiznich · 2020-09-15T12:54:33Z

diesel/src/sqlite/connection/raw.rs

-    F: FnMut(&RawConnection, &[*mut ffi::sqlite3_value]) -> QueryResult<SerializedValue>
-        + Send
-        + 'static,
+    F: Fn(&str, &str) -> std::cmp::Ordering + Send + 'static,


I think this should be restricted to RefUnwindSafe as we cannot mutate the closure.

weiznich · 2020-09-15T12:55:42Z

diesel/src/sqlite/connection/raw.rs

+        )
+    };
+
+    // It doesn't matter if f is UnwindSafe, since we abort on panic.


I think similar to the sql function handler we should just return an error here, instead of abort the program.

weiznich · 2020-09-15T12:59:56Z

This looks almost good, noted some minor improvements otherwise I'm fine with this now.

weiznich · 2020-11-02T11:50:04Z

@TaKO8Ki This is another candidate for rebasing + fixing the remaining comments if your are interested

TaKO8Ki · 2020-11-02T12:33:49Z

@weiznich
I'm going to tackle this!

weiznich · 2020-11-10T09:17:48Z

Likely relevant: rusqlite/rusqlite#839

weiznich · 2021-04-15T19:12:16Z

Closed by #2564

z33ky commented Sep 4, 2020

View reviewed changes

z33ky force-pushed the sqlite-collation-function branch from 79a45f1 to 548ece5 Compare September 4, 2020 22:35

weiznich requested a review from a team September 5, 2020 02:22

weiznich requested changes Sep 8, 2020

View reviewed changes

Expose sqlite's sqlite3_create_collation()

cbed866

z33ky force-pushed the sqlite-collation-function branch from 548ece5 to 7af7f5c Compare September 11, 2020 09:47

z33ky force-pushed the sqlite-collation-function branch from 7af7f5c to d694a55 Compare September 11, 2020 09:55

weiznich reviewed Sep 11, 2020

View reviewed changes

z33ky added 3 commits September 14, 2020 18:00

Add convenience function to return errors for sqlite contexts

3464a40

Reduce scopes of unsafe in sqlite aggregate functions

1568d9f

z33ky force-pushed the sqlite-collation-function branch from d694a55 to c2f4e7f Compare September 14, 2020 16:01

z33ky commented Sep 14, 2020

View reviewed changes

z33ky force-pushed the sqlite-collation-function branch from c2f4e7f to bf4c952 Compare September 14, 2020 18:54

z33ky added 3 commits September 14, 2020 20:55

Add assert_fail!() convenience function

eaa78e1

This encompasses the pattern of eprintln!() + std::process::abort() and adds a request to open an issue if the message is observed.

Prevent panics from escaping SQLite custom functions

5d134c1

This is essentially the same treatment that custom aggregate functions got in ee2f792.

Make note about unwind-safety for SQLite custom functions in document…

bf4c952

…ation

weiznich reviewed Sep 15, 2020

View reviewed changes

weiznich approved these changes Sep 15, 2020

View reviewed changes

TaKO8Ki mentioned this pull request Nov 12, 2020

Expose sqlite's sqlite3_create_collation() #2564

Merged

weiznich closed this Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose sqlite's sqlite3_create_collation() #2495

Expose sqlite's sqlite3_create_collation() #2495

z33ky commented Sep 4, 2020

z33ky Sep 4, 2020

p-alik Sep 5, 2020

z33ky Sep 5, 2020

weiznich Sep 8, 2020

z33ky Sep 8, 2020

weiznich Sep 9, 2020 •

edited

Loading

z33ky Sep 9, 2020

z33ky commented Sep 4, 2020

weiznich left a comment

weiznich Sep 8, 2020

z33ky commented Sep 8, 2020

weiznich commented Sep 9, 2020

z33ky commented Sep 9, 2020

z33ky commented Sep 9, 2020

weiznich commented Sep 11, 2020

z33ky commented Sep 11, 2020

weiznich left a comment

weiznich Sep 11, 2020

z33ky Sep 11, 2020 •

edited

Loading

weiznich Sep 11, 2020

z33ky Sep 14, 2020

z33ky commented Sep 14, 2020

weiznich Sep 15, 2020

weiznich Sep 15, 2020

weiznich Sep 15, 2020

weiznich Sep 15, 2020

weiznich Sep 15, 2020

weiznich Sep 15, 2020

weiznich Sep 15, 2020

weiznich commented Sep 15, 2020

weiznich commented Nov 2, 2020

TaKO8Ki commented Nov 2, 2020

weiznich commented Nov 10, 2020

weiznich commented Apr 15, 2021

	unreachable!(
	"We've written the aggregator above to that location, it must be there"
	)

Expose sqlite's sqlite3_create_collation() #2495

Expose sqlite's sqlite3_create_collation() #2495

Conversation

z33ky commented Sep 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z33ky commented Sep 4, 2020

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z33ky commented Sep 8, 2020

weiznich commented Sep 9, 2020

z33ky commented Sep 9, 2020

z33ky commented Sep 9, 2020

weiznich commented Sep 11, 2020

z33ky commented Sep 11, 2020

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z33ky Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z33ky commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich commented Sep 15, 2020

weiznich commented Nov 2, 2020

TaKO8Ki commented Nov 2, 2020

weiznich commented Nov 10, 2020

weiznich commented Apr 15, 2021

weiznich Sep 9, 2020 •

edited

Loading

z33ky Sep 11, 2020 •

edited

Loading