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

Provide a implementation of diesel::connection::Connection as part of the connection type of this crate #890

Closed
weiznich opened this issue Apr 23, 2022 · 9 comments

Comments

@weiznich
Copy link

This is more a feature request/suggestion than a bug report.

As part of the diesel 2.0 release we reworked large parts of diesels internals, such that it should now be possible to provide a custom Connection implementation for the existing diesel postgres backend outside of the main crate. Your crate would be a good candidate for this as it already provides a pure rust postgres connection implementation. I expect that there would be a certain demand in having a pure rust connection implementation supported by diesel as well.

A diesel connection implementation would enable the usage of any supported diesel feature with a connection type from your crate. This includes all of the existing query DSL, all methods to load and map results to rust struct and even the existing migration infrastructure.

This would require implementing diesel::connection::Connection either directly for your connection type or indirectly via a wrapper type. Providing this implementation as part of your crate would enable a better integration with optimizations like prepared statement caching.

If you are interested in such feature I'm happy to provide guidance on writing the corresponding code. I might even be able to help writing parts of the implementation.

@weiznich
Copy link
Author

@sfackler Is there any additional information I could provide to help you making a decision on this? Or is it just that someone needs to implement it?

@sfackler
Copy link
Owner

It might make more sense to do this on the diesel side of things?

@weiznich
Copy link
Author

Not necessarily. Doing this in diesel would require wrapping the connection type into a new type wrapper to store additional state, so users would loose access to the underlying connection type. Additionally this likely would duplicate prepared statement and type oid caching as the connection impl? Another potential problem is around data (de)serialization. While that is possible to do as part of diesel (there is a older PR already there), it might be easier to implement this as part of postgres, because here such an impl would have easier access to the underlying byte buffers.

That written: If you are complete opposed to this idea this is totally acceptable for me. I just like to go through all possible solutions and see which one might be the best solution.

@sfackler
Copy link
Owner

If extra state is required, it'd probably need a wrapper type either way since I don't think I'd want to add thing to Client's internals just for diesel's sake.

The client doesn't perform any statement caching internally.

Third party code can have direct access to the underlying buffers by implementing FromSql.

@weiznich
Copy link
Author

So let me summarize different potential options with some advantages and disadvantages from my point of view here:

Include a rust-postgres connection implementation in diesel itself

This would need to provide an wrapping struct around postgres::Client to include a prepared statement cache, a type oid cache the necessary state for transaction depth tracking. Otherwise the implantation should be relatively straight forward by just implementing the relevant traits. Some tricks were required to get (de)serialization working, as far as I remember from my experimental implementation a while back.

Pros:

  • Tightly coupled to diesel, although basically all required traits/types are part of the stable public API
  • Better discoverable for diesel users because it's part of diesel

Cons:

  • It uses a wrapper type around postgres::Client
  • We likely won't be able to expose the underlying client, at least as long as there is no 1.0 release of rust-postgres
  • More code to maintain for diesel maintainers, the crate is already really large
  • Another feature added to diesel, which needs to be tested. I'm not sure how this could be even added to our ci setup at all.
  • Only indirectly visible for rust-postgres users, although you can provide links
  • Tricky (de)serialization implementation, although this likely can be solved by exposing the relevant parts on rust-postgres side

Include a diesel connection implementation in rust-postgres

This needs either a wrapper struct around postgres::Client, or some (conditional) modifications to said struct to include the relevant state keeping there. Otherwise the implementation would basically the same as doing that in diesel, as all relevant types/traits from diesels side are exposed publicly. The (de)serialization implementation might be cleaner this way, as this can access rust-postgres internals.

Pros:

  • Tightly integrated with rust-postgres, so users here can find it easily
  • Potentially more straight forward (de)serialization implementation.
  • Directly available for rust-postgres users
  • Potential to share some infrastructure regarding to caching and type lookup
  • Ability to expose the underlying postgres::Client struct, as the crate version always matches.

Cons:

  • Only indirectly visible for diesel users, although we can provide documentation/space for this
  • More code to maintain for rust-postgres (I estimate around 500 LoC + documentation + some tests)

Publish such a connection implantation as separate crate

The implementation idea described for diesel should also work in any other third party crate, so theoretically we could publish a diesel_rust_postgres (or whatever) crate that contains only this implementation.

Pros:

  • Neither part of diesel nor rust-postgres. So it could increase it's version whenever needed.

Cons:

  • Needs an maintainer

@sfackler
Copy link
Owner

I'm not sure what kind of OID tracking you need, but you can get them directly out of the Type objects accessible from a prepared statement: https://docs.rs/tokio-postgres/latest/tokio_postgres/types/struct.Type.html#method.oid.

I think I would lean towards this being a separate crate.

@weiznich
Copy link
Author

weiznich commented Jun 3, 2022

I'm not sure what kind of OID tracking you need, but you can get them directly out of the Type objects accessible from a prepared statement: https://docs.rs/tokio-postgres/latest/tokio_postgres/types/struct.Type.html#method.oid.

OID tracking is required by diesel for constructing prepared statements containing custom types. That would essentially require to construct a postgres::Type based on a type and schema name, at least for user defined types like enums or other composite types. In essence that does require querying the database to get those information. To skip this query most of the time each diesel connection caches these information. As this is already implemented in a generic way it's no big deal if rust-postgres does not offer an API for this. On the other hand if there is already such a functionality available it might be useful to reuse this in the diesel connection implementation build on top.

EDIT: I'm closing this issue now, as I got my answer and this seems to be out of scope here.

@weiznich weiznich closed this as not planned Won't fix, can't repro, duplicate, stale Jun 3, 2022
@AlbertMarashi
Copy link

Would really like to see this!

@banool
Copy link

banool commented Sep 30, 2023

I'd really love to see this too, it'd be great to be able to use Diesel without relying on libpq.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants