Add unix domain socket support for Postgres #253
Conversation
Nilix007
force-pushed
the
postgres_uds_support
branch
2 times, most recently
from
999e2ee to
7f195ac
Compare
Nilix007
force-pushed
the
postgres_uds_support
branch
from
7f195ac to
51e8f65
Compare
Nilix007
force-pushed
the
postgres_uds_support
branch
from
51e8f65 to
4f0410b
Compare
| enum Inner { | ||
| NotTls(TcpStream), | ||
| #[cfg(all(feature = "postgres", unix))] | ||
| UnixStream(crate::runtime::UnixStream), |
There was a problem hiding this comment.
I don't like how this in MaybeTlsStream. I think it conflates what the type here is doing.
I'd rather something like:
-
A separate
Socket(name?) type that is internally an enumeration betweenUnixStreamandTcpStream. -
This
MaybeTlsStreamis then generalized around anStype parameter.
Thoughts?
There was a problem hiding this comment.
I don't see much reason to parameterize MaybeTlsStream if the type is always going to be Socket.
There was a problem hiding this comment.
We could call it MaybeUdsStream and reverse the relationship so that MaybeUdsStream is an enumeration between UnixStream and MaybeTlsStream.
There was a problem hiding this comment.
I guess before we get ahead of ourselves here.. is TLS over UDS even a reasonable thing? If it is, we probably need to at least support that with the type sandwhich.
There was a problem hiding this comment.
The wording in the documentation doesn't suggest TLS support at all over the domain socket: https://www.postgresql.org/docs/12/ssl-tcp.html
It doesn't really make sense anyways to encrypt over the UDS because if someone can sniff the traffic on that socket then they can probably also just inspect your process' memory or Postgres' memory/files for juicy secrets (since they'd have to be running on the same machine with elevated privileges already).
| // and falls back to postgres@.../postgres | ||
| let username = url | ||
| .username() | ||
| .or_else(|| std::env::var("USER").map(Cow::Owned).ok()) |
There was a problem hiding this comment.
Perhaps https://crates.io/crates/whoami is a better idea to be more resilient?
| .expect("percent-encoded hostname contained non-UTF-8 bytes") | ||
| }) | ||
| .or_else(|| url.param("host")) | ||
| .unwrap_or("/var/run/postgresql".into()); |
There was a problem hiding this comment.
By default we may want to iterate potential locations for the socket and not just hard-code this one.
/tmp/var/run/postgresql/private/tmp( macOS )
|
First off, awesome enhancement here.
Postgres doesn't support UDS on Windows yet. I did see a patch get merged for that due in Postgres 13, but we have some time to think about it. I'm not sure if Postgres supports named pipes in Windows though. I'd say that's a different improvement if it does. |
|
Thanks for getting this started. I'm doing some refactoring of Postgres and want to roll this in. Thanks again. 👍 |
|
Thank you for building this awesome piece of software 🥇 PS: I've just pushed a couple of changes that piled up for this PR. You might take 'em as inspiration or something: Nilix007/sqlx@postgres_uds_support^^^...Nilix007:postgres_uds_support |
|
@Nilix007 Thanks. I rolled that into |
3 participants
This PR adds unix domain socket support for Postgres. It also changes the default values of multiple connection parameters to match libpq behaviour, e.g.
postgres:///was equivalent topostgres:///postgres@localhost/postgresfor sqlx, but now sqlx would connect to the DB via UDS with the current user as username and database name (assuming we're running a Unix).Current state: Works on my Linux 😆 What kind of tests would you like to see?
Open questions:
$USERto get the name of the current user. Does this work on all (supported) Unixes? What about Windows? Do we want to be smarter? (e.g. callgeteuidandgetpwuid)Fixes #144