Skip to content

Conversation

@weiznich
Copy link
Member

This commit adds a second connection implementation for postgresql,
based on the pure rust postgres crate. This allows us to remove the
dependency on libpq in certain cases, by providing a separate feature
for using a the pure rust implementation for example. Maybe this also
simplifies cross platform support for a async postgres connection?

Test suite is passing for the new connection implementation if I change diesel-tests to use this one instead of the old PgConnection implementation.

I've done a small unscientific benchmark with those two implementations. The code is here. Results are here. (Take them with a grain of salt, just run them once without actual complicated benchmark setup). From a coarse look it seems like the pure rust implementation is minimally slower the the native libpq version, for the given benchmarks. (Or it's just noise?)

Open questions:

  • Do we even want this? (That's currently just a experiment seeing where there are problems with doing this. I've found no bigger issue…)
  • How to handle testes for this? Just run diesel tests again with a different feature?
  • How to organize features in diesel itself? Currently this is behind the postgres feature, but we may want to put this behind some separate feature flag?

This commit adds a second connection implementation for postgresql,
based on the pure rust postgres crate. This allows us to remove the
dependency on libpq in certain cases, by providing a separate feature
for using a the pure rust implementation for example. Maybe this also
simplifies cross platform support for a async postgres connection?
@weiznich weiznich requested a review from a team December 28, 2019 12:22
@kiljacken
Copy link

Another benefit: Using a pure rust postgres implementation greatly decreases the complexity in building a statically linked binary that uses diesel as a dependency.

Currently you have to recompile openssl, libz, and libpq using musl and configured for static linking, which is quite the journey. With a pure rust postgres impl it becomes just running cargo build --release with the right target and RUSTFLAGS.

@weiznich weiznich force-pushed the feature/pure_postgres branch 3 times, most recently from 0483629 to 98e4572 Compare January 2, 2020 18:44
@weiznich weiznich force-pushed the feature/pure_postgres branch from 98e4572 to 9a5a6ec Compare January 2, 2020 21:29
@sgrif
Copy link
Member

sgrif commented Jan 9, 2020

I haven't had a chance to review this in depth yet, but rather than shipping both the libpq backend and the postgres backend, which requires both impls to co-exist (which in turn requires breaking changes to things like TransactionBuilder), can we just toggle between a single implementation with cfg?

@kiljacken
Copy link

@sgrif Yeah, a feature gate is also a must if my previously described benefits with regards to static builds are to be realized.

@weiznich
Copy link
Member Author

weiznich commented Jan 9, 2020

Sure, it shouldn't be to hard to put that behind a feature flag. The harder part is to discuss how we would like to have those features be organized? To minimize breaking stuff probably something like:

  • The postgres feature continue to exists and enables the existing connection implementation based on libpq
  • A new postgres_rust (open for a better name) feature is added that enables the new connection implementation.

I haven't had a chance to review this in depth yet, but rather than shipping both the libpq backend and the postgres backend, which requires both impls to co-exist (which in turn requires breaking changes to things like TransactionBuilder)

That should be the only breaking change with the current implementation, all other changes (PgMetadataLookup) are internal to diesel. We could make this easily non breaking by just setting the new type parameter of TransactionBuilder as default to PgConnection (at least if this feature is enabled).

Another thing that could be useful: We could change the implementation of diesel_cli to use the new connection type as this greatly simplifies the setup, as people don't need to have libpq installed anymore. On the other hand this will increase the required compilation time quite significantly, because postgres pulls in tokio and so on.

@sgrif
Copy link
Member

sgrif commented Jan 9, 2020

If we ship it I would definitely want unstable_ in the name and tell folks that we're looking at a re-implementation, and would like feedback on if it affects their apps

This is supposedly marked as unstable, as we want to experiment a bit
with this before actually committing into supporting this.
* Allow to construct a connection directly from a existing
`postgres::Client` (Probably usefull if someone want's to have special
configurations for example ssl)
* Allow to access the inner client
* Add some documentation
@weiznich weiznich added this to the 2.1 milestone Jan 10, 2020
@kiljacken
Copy link

Wanting to provide some real world feedback, I've been trying to get this running against our code base by [patch]-ing this branch in our workspace Cargo.toml, but with little luck so far. I get a lot of compile errors in diesel.

Is the code knowingly in a state where it doesn't really work, or am I probably doing something wrong?

@weiznich
Copy link
Member Author

weiznich commented Feb 7, 2020

The code is at least in a state where it compiles and passes the test-suite. Without knowing the exact error messages it's a bit hard to tell what's exactly wrong. I would guess that you need to also [patch] diesel_derives.

@kiljacken
Copy link

diesel_derives was the key, thank you. Will report back once I get our code up to date with the PgValue changes and get to give it a spin.

@kiljacken
Copy link

Okay, that took a bit due to a vacation, but I've got our code base up and running with the Rust based PostgreSQL connection, and it's looking good so far. No discernible difference in performance, but a nice cleanup of our release build flow.

quickcheck = "0.4"

[features]
default = ["with-deprecated", "32-column-tables"]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove 32-column-tables from default? It bit me in the behind while porting to the new system as the error message when lacking it isn't really that great.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's only a left over artefact from local development. I tend to disable the 32-column-tables feature, because this reduces the time needed to compile of diesel quite significantly.

(But yes, this should be fixed before merging)

@jamessewell
Copy link

Is there any progress here? This is a killer feature imo

@weiznich
Copy link
Member Author

weiznich commented Apr 6, 2021

@jamessewell Asking for updates on a stalled pull request won't magically make this feature appear, so please don't do this.
If you are interested in having this feature here are a few possibilities to help make this happen:

  • Start providing answers to the open questions listed in the opening post, rebase the PR, cleanup the implementation and rerun the benchmarks
  • Provide a third party crate implementing exactly this connection type, as there is no real reason for this to be part of diesel itself
  • Talk with the rust-postgres maintainer to provide a diesel connection implementation for their connection type, because again there is no real reason for this to be part of diesel itself.
  • Pay someone to do one of the points listed above for you
  • Wait till one of the volunteers finds the time to work on this

@weiznich
Copy link
Member Author

Closed as this is currently not planed as part of diesel. I encourage interested users to publish such a connection implementation as third party crate for now. After such a crate reaches a stable state we can talk about upstreaming parts of that work into diesel.

@weiznich weiznich closed this Aug 29, 2022
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

Successfully merging this pull request may close these issues.

4 participants