Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sqlite] Access to connection handle #430

Closed
agentsim opened this issue Jun 21, 2020 · 12 comments
Closed

[sqlite] Access to connection handle #430

agentsim opened this issue Jun 21, 2020 · 12 comments
Labels
E-easy enhancement New feature or request good first issue Good for newcomers

Comments

@agentsim
Copy link
Contributor

It would be useful to be able to get the underlying sqlite3* handle so that the database can be further configured, or to be able to customize the creation and configuration of the connection using the handle.

For instance, I would like to be able to call sqlite3_create_collation_v2 to set up a collating sequence using the unicase crate.

@abonander abonander added E-easy enhancement New feature or request good first issue Good for newcomers labels Jun 21, 2020
@mehcode
Copy link
Member

mehcode commented Jun 21, 2020

SQLite calls it a database connection handle so I'd probably expect a method named as_raw_handle() ( trying to mimic how the std exposes raw internals ) on SqliteConnection that returns the raw sqlite3 pointer.

@agentsim
Copy link
Contributor Author

Thanks for the tip.
Looking at this a little more, as_raw_handle() doesn't seem like it would be enough. It is unlikely (I think) that a user would want some connections configured with a custom collation, and others not.

So perhaps the correct approach is to extend SqliteConnectOptions to allow for this kind of customization?

@abonander
Copy link
Collaborator

We're actually discussing the option to configure an on-connect callback to Pool here: #263

@agentsim
Copy link
Contributor Author

Yes, that sounds like the right idea.

@abonander
Copy link
Collaborator

While the method to get the raw handle is being added in #438, it also sounds like having a way to safely introduce new collations would be a good feature-add for the SQLite API. Maybe something like:

impl SqliteConnection {
    pub fn add_collation(&mut self, name: &str, collation: impl Fn(&str, &str) -> Ordering + Send + Sync + 'static) { ... }
}

@abonander
Copy link
Collaborator

The implementation would be simpler but the interface less flexible if the callback was defined as extern "C" fn(&str, &str) -> Ordering.

@agentsim
Copy link
Contributor Author

I'll give this a shot. The feature already exists in rusqlite. Should I aim to copy what they've done or pull in that crate as a dependency?

It would still require #263 to ensure that connections are configured consistently.

@abonander
Copy link
Collaborator

abonander commented Jun 23, 2020

We probably don't want to pull in rusqlite just for this feature. However, their implementation looks a lot more sensible than the one I was formulating in my head so I think it makes a decent starting point.

Let's not directly copy it though. Now I'm thinking that add_collation() should actually be a part of SqliteConnectionOptions which adds it to a map, and then if that map is available/nonempty when a connection is created it then adds the collations to the database. This also means it doesn't need #263 to work with Pool, but the data pointer needs to be based on Arc instead of Box since the destroy callback would be invoked for each connection.

I would like to somehow make this work with sqlite3_collation_needed to lazily add the collations but I'm not sure what we're supposed to do if we don't have a collation for a given name; the callback itself is supposed to add the collation but if it doesn't add the collation does the callback just continually get invoked in a loop or does it error? If it errors we can propagate that out but otherwise it could cause a deadlock. I'll need to dig through the source to see because the documentation doesn't say.

@abonander
Copy link
Collaborator

It looks like it invokes the callback once for any collation that it can't find and emit error if it still can't find the collation after invoking the callback: https://github.com/sqlite/sqlite/blob/42a630b1daf1df13850095251cc73be84dbdeac8/src/callback.c#L217-L231

This means we can definitely use sqlite3_collation_needed if we want to lazily install the collations, although I'm not sure what benefit that would confer, maybe slightly lower memory usage?

@agentsim
Copy link
Contributor Author

I'm not clear on the utility of sqlite3_collation_needed unless your schema has tons and tons of collations. Then again, if it is easy to integrate the only downside to it would presumably be slightly higher latency the first time the collation is hit. So maybe it boils down to implementation details.

@abonander
Copy link
Collaborator

abonander commented Jun 24, 2020

I'm thinking we could have both relatively easily:

impl SqliteConnection {
    pub fn add_collation(&mut self, ...) -> &mut Self { ... }
    pub fn install_collations_lazily(&mut self) -> &mut Self { ... }
}

and then if install_collations_lazily is set it uses the sqlite3_collation_needed callback instead of immediately invoking sqlite3_create_collation on new connections.

Unless maybe people would want to mix both eagerly and lazily created collations? This sounds like some quite niche optimizations though. I think for an MVP all we need is add_collation().

I also imagine part of the utility in lazily creating collations would be to actually run user code when they need to be created, so that may warrant a different interface entirely.

@mehcode
Copy link
Member

mehcode commented Jun 24, 2020

Personally I'm of the opinion this stuff should be exposed fairly directly on SqliteConnection ( by directly I mean close to 1:1 the C API ). I think rusqlite does a very good job here: https://docs.rs/rusqlite/0.23.1/rusqlite/struct.Connection.html#method.create_collation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants