-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support for embeded database #39
Comments
This is likely not going to happen until we get closer to 1.0. I have said in the past that I want to support other databases eventually. My gut was to close this, as there's not anything immediately actionable. However, as you've presented a reasonable use case that isn't "I prefer MySQL", I'm accepting this feature for 1.0. |
make sure (pre 1.0) that the libraries design allows generating database-agnostic sql (or well plugging in custom sql generators for different databases), i think that might be easily forgotten when mainly using postgress as backend. |
Yup, supporting 1 means supporting N |
Could you describe what has to be done to implement a new database ? |
So I've started to give this some serious thought. @tafia has started some of the work on this in #72. Here's a rough overview of what has to change, in order of easiest to hardest, along with what I think might be possible solutions, but I'm not yet sure. Just as a note, while I haven't decided if I will support MySQL in the long term, I do want whatever changes we make here to make it possible to theoretically support MySQL, as that will help ensure that we can truly support N adapters and not just 2. In an ideal world, this is done in a way that additional adapters can be added as third party crates. I'm writing this up to help clarify my thoughts, get some eyes on it to see if I'm missing anything, and give a roadmap for anyone who wants to take on a piece of this. I would prefer that we solve the harder bits (or at least have a strong indication that we have a solution that will work) before we bother with the easier pieces.
|
@mfpiccolo @samphippen @mcasper It's long, and pretty technical, but I'd like to get thoughts on the above. Also if any of you are interested in helping tackle some of this, have at it. @tenderlove, @rafaelfranca, @senny, @metaskills -- I know none of you are involved in this project or Rust, but I think you guys would be able to provide some valuable insight as well. I'm happy to clarify our API and how these problems relate to it in ways that don't require knowledge of the language if you'd like. @metaskills: In addition, I'd be interested to hear if you think this will be enough for SQL Server. I've more or less looked at this from a high level WRT MySQL as the third case, but knowing that it can handle another DB is a good indication that we will in fact be generic after this. After writing this up, I think I've got my thoughts organized enough that I can see a reasonable path to implementing this. From what I can tell, the proposed solutions will also put us in a place for others to write additional database adapters without having to have them in the primary crate. We can essentially put this to the test by having postgres and sqlite live in separate crates. @e-oz, you've expressed interest in having MySQL support in the past. It'd be great if you were interested in working on that as a third party crate, as well. |
@matthewd If you don't mind, you might have some thoughts too |
About differences in types supported by different databases: |
Would it be an option to just put support for different engines into different modules, so you can just import the right one for your application? That would remove the ability to dynamically configure your application to use another database, but might reduce coding complexity. |
@iqualfragile That's essentially the scorched earth option at the end, but it doesn't work terribly well if you want to have traits people can depend on regardless of backend. Ideally our core types (in the rust sense not the SQL sense, e.g. what comes out of the But yes, not having to be properly generic over the backend would reduce coding complexity, and I'm open to it as a last resort. I'm confident after writing up these thoughts that we can properly become generic though. Bind params are the hardest part and I think we have some good options. |
Probably.
Probably not, unfortunately. Since it can't inline a virtual call, it needs to allocate the full return value on the stack for the function to use.
Depending on the size of the returned value, it probably won't make a huge difference. When dealing with databases, you have to compare this stuff to the performance of I/O. Reading/writing to the network or a file is going to dominate any profile, so worrying about a few extra copies here and there is probably not going to matter. |
I agree. That said, it'd still be nice to have benchmarks to back up the fact that we haven't caused any regressions in cases like finding a single record by primary key, where our query builder has the largest hit. Additionally, being able to show that our query builder is effectively 0 cost is a nice bullet point. That said, yeah I don't think the metadata case will be nearly as big of a hit as changing |
there is another big problem with sqlite, as its column types are more of a recommendation, i.e. you can store a string into an integer field and sqlite won't complain. while i strongly doubt this should be supported for the insert/update, it needs to be kept in mind on the select side of things. |
This is true. However, I think leaning on the type safety provided by our current query builder is fine here. Based on where sqlite has caused issues in Active Record, I believe we should be safe. |
Nice job on the roadmap. I agree that if we can pull it off the adapters should be third party crates. Here are some of my thoughts:
This sounds a bit rough but like you said we will be handled by
👍
Meaning loss as in they will need to be supported in an external crate?
Ensuring that we don't have regressions seems like a good move if diesel is to power much of the database interaction in the Rust community.
The small subset of types that SQLite supports is going to be the default types that are supported in diesel core, so this will affect all the adapters even though it wouldn't be necessary if we didn't support SQLite. I certainly defer to your better judgement on this:
but it might be prudent to be sure of this before deciding if it is worth it to support SQLite with this route. |
Thanks for including me Sean. I think I can speak to one point well about bind params and my experience with SQL Server and Ruby's TinyTDS gem. I know of no C API outside of the ODBC layer for the DBLIB protocol that is essentially SQL Server. I punted on this use TSQL when I did support for bind params in the SQL Server adapter. Check out this really old engineyard post for details. Basically, we do bind params at the SQL layer by wrapping the SQL as a param to a stored procedure which has n arguments that include the type and value of each param. I am unsure if this, like SQLite, is at odds with your architecture. Happy to go into details on any topic if you think it helps. |
Loss as in they will need to be adapter specific (which likely means being in another crate)
If our type system doesn't cover any potential problems, it's an issue regardless of if we're supporting SQLite or not. |
Moving the timeframe on this forward to 0.5, as it's our most immediately actionable feature. I expect to have this finished this weekend. |
While not perfect, www.freetds.org does have a 'C' library that can connect to Sybase and MS SQL Server, including some support for the dblib API. We used rust-bindgen with it, and are testing it out now. I'm very interested in seeing if it can be made to work with Diesel (eventually). |
@placrosse The TinyTDS gem makes exclusive use of the DBLIB API in FreeTDS. Love it. |
@placrosse I'm interested in making sure that it's possible to add additional adapters that aren't part of the main diesel crate. If you're interested in trying to add TDS support as a third party crate once we finish the SQLite stuff, let me know -- I'd be interested in working with you on it. |
Yes, @sgrif , I am very interested in doing just that. |
This commit adds all of the guts for SQLite3 support to Diesel. There are several outstanding issues which will need to be resolved before this is ready to be merged into master. This commit will not be merged into master until these issues have been resolved. Ultimately the most difficult remaining problem which we need to address is the lack of a `DEFAULT` keyword. I will detail why this is a problem and what I think we can do to solve it in a separate issue. SQLite has a fundamentally different mechanism for handling bind parameters than pretty much every other backend out there, where the function you call differs based on the type, rather than sending an array of bytes and telling the backend how to interpret it. `FromSql` had to be updated to address this (see 8a33029 for details on that). I have not applied the same change to `ToSql`, as it was possible to shoehorn SQLite into our existing structure. While this may end up changing in the future, I do not believe it is necessary for 0.5. I'm not super happy about the structure of `StatementIterator`, `SqliteRow`, and `SqliteValue`, as ultimately the `Row` is a giant lie and `Statement` is the the value that maintains all of the state. Ultimately this comes out of the weird interaction between the fact that `Row#take` returns a reference, but sqlite's C api doesn't actually return a single value that works in that sense (and really doesn't have the concept of a row at all). As with much of our code overall, this can probably be cleaned up in the future by having the `Backend::RawValue` directly be a reference for `Pg`. I need to figure out a way to accomplish this without adding a lifetime to the `Pg` struct. I've broken our test suite into postgres specific and general where possible, but this is a *very* incomplete process. There are several modules which are entirely scoped to PG that do not need to be, which can be addressed independently in later PRs. Additionally, the entire `expression` module needs to be for expressions which are specific to PG. I do not believe that SQLite has anything specific to it which is not also supported by PG. Fixes #39 (mostly. Enough to close the issue at least)
This commit adds all of the guts for SQLite3 support to Diesel. There are several outstanding issues which will need to be resolved before this is ready to be merged into master. This commit will not be merged into master until these issues have been resolved. Ultimately the most difficult remaining problem which we need to address is the lack of a `DEFAULT` keyword. I will detail why this is a problem and what I think we can do to solve it in a separate issue. SQLite has a fundamentally different mechanism for handling bind parameters than pretty much every other backend out there, where the function you call differs based on the type, rather than sending an array of bytes and telling the backend how to interpret it. `FromSql` had to be updated to address this (see 8a33029 for details on that). I have not applied the same change to `ToSql`, as it was possible to shoehorn SQLite into our existing structure. While this may end up changing in the future, I do not believe it is necessary for 0.5. I'm not super happy about the structure of `StatementIterator`, `SqliteRow`, and `SqliteValue`, as ultimately the `Row` is a giant lie and `Statement` is the the value that maintains all of the state. Ultimately this comes out of the weird interaction between the fact that `Row#take` returns a reference, but sqlite's C api doesn't actually return a single value that works in that sense (and really doesn't have the concept of a row at all). As with much of our code overall, this can probably be cleaned up in the future by having the `Backend::RawValue` directly be a reference for `Pg`. I need to figure out a way to accomplish this without adding a lifetime to the `Pg` struct. I've broken our test suite into postgres specific and general where possible, but this is a *very* incomplete process. There are several modules which are entirely scoped to PG that do not need to be, which can be addressed independently in later PRs. Additionally, the entire `expression` module needs to be for expressions which are specific to PG. I do not believe that SQLite has anything specific to it which is not also supported by PG. Fixes #39 (mostly. Enough to close the issue at least)
We still have a little bit more work to do, but I've merged our SQLite support branch into master. We'll be releasing 0.5 with this feature later this week. |
In regards to MySQL, are there any plans for support in the near future, or is it still slated for close-to-1.0? This is not an "I prefer MySQL" as in "I don't want to work with PostgreSQL", but the project I'm working on uses MySQL for other services and it's just not feasible to move to PostgreSQL and SQLite is not a good alternative due to its performance. |
@ixjf the diesel core team have explicitly decided we will not be adding first party support for MySQL. That said, we welcome anyone who is interested in building a 3rd party crate that integrates with diesel to leverage this support. If you'd like to do that, we'd absolutely love to see it and we'd be able to support you to some degree. Thanks ❤️ |
To make this useful for more than web-applications that are backed by a database server, for example a dektop application that needs to save some data, it would be nice to have support for at least one embedded database like sqlite, which happens to be following pgs design decisions if in doubt.
The text was updated successfully, but these errors were encountered: