Get raw sqlite3 connection pointer?

[ColonelThirtyTwo]

Can we get a function to get the raw *mut sqlite3 behind a connection?

Ok, what I really want is a way to make an FTS5 tokenizer in diesel. But frankly, sqlite has a lot of extensions and I don't think diesel is going to implement them all (or if it will, it will be a bit). Getting the pointer is an easy way for users to utilize the extensions they need to.

[weiznich]

Can you give an concrete example what exactly is missing for you and what would be required to have as much support in diesel to support your use case in a safe way. Diesel provides already a lot of foundational building blocks that should allow you to add a lot of extension functionality by using the extensions points (custom sql functions, custom type defs, query dsl extensions, …) in third party crates.

See also #1640 for a past proposal.

That all written: If someone proposes an interface that holds up all the invariants that need to meet for the required unsafe code in implementation we can talk about that. Otherwise I see that more as something that does not fit that well in diesels API design, as we provide a completely safe API.

[ColonelThirtyTwo]

I don't understand what you are wanting in a "safer" API that is accomplishable with raw pointers. You could have something like db.exec_with_raw_ptr(|ptr| { ... }), to restrict how long the pointer is used for, but you could just transfer the pointer from inside of the closure.

Creating a Diesel database from the raw pointer is doable, and would solve the "registration" use cases for FTS tokenizers and R-Tree query functions, but it would require Diesel users to duplicate the code for opening the database, and it would not work for the Backup or Sessions APIs.

I admit I don't know much about Diesel's design, but... are there actually that much safety restrictions around getting the raw handle? The comment you linked to mentions threading, but that's fairly easy to accommodate, especially since SQLite doesn't support multi-threaded access anyway. You mentioned cacheable queries, so maybe banning statement-related functions while one of those are checked out? That's still fairly easy to do. I don't see how get_raw_handle is a huge issue.

[weiznich]

I don't understand what you are wanting in a "safer" API that is accomplishable with raw pointers. You could have something like db.exec_with_raw_ptr(|ptr| { ... }), to restrict how long the pointer is used for, but you could just transfer the pointer from inside of the closure.

At least for me there are slight differences between different unsafe API designs here. There are that ones like get_raw_handle() that are just totally unsafe, as it seems the user can do everything there. They are quite similar to std::mem::transmute, which is horribly unsafe. And then there may be different designs that point the user slightly into the right direction, so that you must at least actively try to do something wrong with the API to cause undefined behaviour. exec_with_raw_ptr seems to be a bit better here, but not perfect. That's the reasons why I requested a few possible different designs with comments when they would be usable and when not. Maybe just do a little overview about different options, how they would apply to the different extensions API's and how they would interact with internal diesel code. I mean the corresponding code is not that large or complex that it is that hard to understand. Have a look at SqliteConnection, StatementCache, RawConnection and RawConnection for relevant parts.

I admit I don't know much about Diesel's design, but... are there actually that much safety restrictions around getting the raw handle?

At least for me that's a problem. I mean you come here and propose a feature. I react with a skeptic comment and state that at least I wont implement it that way and request a better design. That's the point where you can say: Ok, maybe the maintainer knows better or where you can say: Ok, I need to find a good argument based on the code base how this could work. Stating that you don't know much about the design is not really helpful. Now my point here is that I do not need that API and I do not have the time or motivation to implement it. This means you (or anyone else interested in this) needs to sit down and learn the necessary details about Diesels design, to make a good argument.

You mentioned cacheable queries, so maybe banning statement-related functions while one of those are checked out? That's still fairly easy to do.

Prepared statement caching is a really important part of diesel. Banning functions using this caching is impossible, as this would disable all query related functionality for this connection. Disabling prepared statement caching comes with a serve performance cost. You will likely loose 2x, which is in my opinion not acceptable. This becomes more serve as we don't have any way to tell when the user dropped the corresponding pointer, as it is a raw pointer. We don't know anything about how long it may live, we must assume that after giving out the pointer a user could cause undefined behaviour at any point by manipulating this pointer.

I don't see how get_raw_handle is a huge issue.

It's a huge issue in that way that there is no other unsafe function in diesels public API, as diesels goal is to provide a safe API. At least in my opinion it's a thing that needs to discussed quite a bit if we even want to add such a thing at all, even if we can come up with a relatively good API for this. I would likely be more open to feature requests and followup pull requests that just add a safe interface for the API you want to implement outside of diesel.

[ColonelThirtyTwo]

I don't think this is going anywhere, so I'm going to continue using rusqlite. This will probably be my last post in this thread.

I need the *mut sqlite3 pointer from a SqliteConnection object to use its extensions. Currently it's not possible to get that, so I came here to request that a way to get it be added.

The easiest way would be a simple getter function. But apparently that's not acceptable. The only safety issues you've mentioned are using the raw connection in different threads (trivial; the C API requires single threaded use anyway), and issues with messing with prepared statements (though I don't see how; you don't have pointers to those sqlite3_stmt objects and most extensions don't interact with statements anyway).

So... is there anything concretely wrong with PR #1640? Or no?

It's a huge issue in that way that there is no other unsafe function in diesels public API

So what? 99% of what people are going to be using is through the safe API. Having an escape hatch for the 1% of users who need a bit more should not be an issue.

It's like saying "We shouldn't add Box::into_raw because it makes Box unsafe, even though people need to use it".

[Mingun]

@weiznich , you could add access to the raw connection under a feature flag disabled by default. If you personally don't like to expose potential unsafe API that could be problematic in very rare cases you just don't enable it. But the others will have the ability to accept risks and do what they want to do

[weiznich]

I need the *mut sqlite3 pointer from a SqliteConnection object to use its extensions. Currently it's not possible to get that, so I came here to request that a way to get it be added.

Again this is not a wish list where you magically get things you want to have. It's a place to propose and discuss designs. If I go over the current thread I did not see anything concrete proposed here. You only stated that you want to have a way to get the raw connection pointer. I've linked a previous proposal for this + stated that I would like to see some alternative designs first. I cannot see them yet.

The easiest way would be a simple getter function. But apparently that's not acceptable. The only safety issues you've mentioned are using the raw connection in different threads (trivial; the C API requires single threaded use anyway), and issues with messing with prepared statements (though I don't see how; you don't have pointers to those sqlite3_stmt objects and most extensions don't interact with statements anyway).

Have you proposed a concrete signature of such a function yet + listed what safety invariant needed to be fulfilled?
Also stating that most extensions don't interact with statements anyway is totally unrelated to the argument that such an API may infer with the statement caching. You get a raw connection pointer there, which is totally unrelated to extensions. Technically you can use the whole sqlite API using this pointer and this can violate some internal lifetime assumptions made for statement caching. (Like that a statement cannot outlive the corresponding connection to just give a simple example)

So... is there anything concretely wrong with PR #1640? Or no?

It's literally in there as comment from some of the other maintainers. As it seems hard to read that let me cite that here again:

Thank you for the pull request, but we've rejected similar features in the past, as we're not interested in exposing the concrete implementations of our connections as part of our public API.

(Highlighting of important section done by me)

So one of the arguments that needs to be addressed is how do you prevent exposing internal implementation details? To word that differently: Exposing libsqlite3_sys::sqlite3 would make that type part of our public API, which in turn means we cannot update to a new "major" version of libsqlite3_sys anymore (at least without doing a breaking change) or replace the dependency on libsqlite3_sys completely. There are probably ways around that, but again that's something that should be included in your proposal.

So what? 99% of what people are going to be using is through the safe API. Having an escape hatch for the 1% of users who need a bit more should not be an issue.

That's exactly my argument why I don't even want to spend more time discussing it. It's not important for most of our users. I'm fine if someone figures out a nicer API for this, but I personally don't want to spend time to design an API that is only used by 1% of the users, at least not as long as there are much more important things left.
Additionally at least some of those 99% of other users don't want to use a crate that exposes unsafe API's for their own reasons, so it is at least worth to discuss if we even should expose such an API at all.

It's like saying "We shouldn't add Box::into_raw because it makes Box unsafe, even though people need to use it".

There are a few important differences between Box::into_raw and "just a getter function:

1. Box::into_raw is not even marked as unsafe
2. Box::into_raw consumes the box, so there is no way that you ever can violate some internal assumption of Box

If you can come up with an API with similar "guarantees" than Box::into_raw the chances are much higher that we may accept this feature.

@Mingun See my answers for the reasoning why I expect at least a well written proposal that addresses the issues listed above before I even consider merging something like that. Also I personally wont implement any proposed API anytime soon, as this is for me not an important feature. So if you would like to see that in diesel start contributing.