-
Notifications
You must be signed in to change notification settings - Fork 409
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
Rust PostgreSQL connection client #7605
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
641d9e5
to
fc1b12b
Compare
mmastrac
commented
Aug 15, 2024
fb539d3
to
8d0279e
Compare
cdd91f6
to
bf8060b
Compare
mmastrac
added a commit
that referenced
this pull request
Aug 28, 2024
Extracted from #7605 This implements a connection string parser in Rust and replaces the older Python DSN parser. The DSN parsing code behaves effectively the same as the previous version of the code with a few small differences: - Backslash escaping/unescaping in pgpass and the connection string now fully match the behaviour of libpq - A few additional warnings were added in cases where an explicit passfile was specified but cannot be read (we still don't warn for implicit passfiles). What's left in the Python DSN parsing code is just SSL setup, which will eventually be migrated to Rust as well. --------- Co-authored-by: Fantix King <[email protected]>
61a6e49
to
b6f2118
Compare
198d86e
to
a763e48
Compare
8d76fd9
to
156e13d
Compare
mmastrac
commented
Sep 13, 2024
Co-authored-by: Fantix King <[email protected]>
fantix
approved these changes
Oct 1, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Epic! LGTM
(There're a few println!("Read:");
in the future connection Rust code; a kind reminder to replace them in the next PR)
msullivan
added a commit
that referenced
this pull request
Oct 10, 2024
PR #7605 broke the inplace-upgrade tests, since it removed host and port from the `pg_addr` field on the `server-info` endpoint. Instead, grab pgdsn from the settings on the connection.
msullivan
added a commit
that referenced
this pull request
Oct 10, 2024
PR #7605 broke the inplace-upgrade tests, since it removed host and port from the `pg_addr` field on the `server-info` endpoint and turned the `server_settings` field into the toplevel. Fix it by exposing the full DSN to server-info as well. (An earlier iteration made the inplace upgrade tests use the testbase python connection to read pgdsn from settings.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements a basic Rust postgres connection library and re-uses its internal state machine to replace the guts of
pgcon
.User-facing changes:
max_connections
or the maximum number of connections defined for the user's currentrole
to prevent hitting connection limits.sslmode
: SSL is always requested unless sslmode=disable. If the server returnsS
to indicate SSL support and then fails to accept the user credentials, we do not retry without SSL.Changes:
PGRawConn
that behaves as a transport and fully connects to postgres before returning the transport to the protocol. This transport can then be used transparently and with zero overhead as it only uses Rust code for connection setup.source_description
parameter wherever we use connections directly or indirectly to assist with tracking down what particular part of the system requested a connection. This is not currently propagated into the parameters we send to the server, but we could do this in the future.pgconnparams.py
, but it delegates all logic to the Rust side of things.class CreateParamsKwargs(TypedDict)
is used wherever we hold a bag of connection parameters to assist with typing.pgaddr
has been removed from the EdgeDB server settings as it was not sufficient to connect to the backend. It has been replaced with a fully-rendered DSN.edgedb-rust
will need to be updated to support this.Testing strategies
DSN parsing: we have a full battery of DSN parsing tests that are compared against libpq. The tests that were previously implemented for pgconnparams have been migrated into this rust codebase. The tests used internally for postgres to test DSN parsing were also extracted and added to the library. On top of this, a number of "hard" examples were added to exercise the code.
Auth code: auth code is tested against a "capture" postgres that we inittb, boot up on a socket and test. Tests for each auth method (scram, md5, password) are tested with and without SSL.
DSN notes
The DSN parsing code behaves effectively the same as the previous version of the code with a few small differences:
Examples:
Parse a DSN:
Query a running server: