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

Rust PostgreSQL connection client #7605

Merged
merged 28 commits into from
Oct 1, 2024
Merged

Rust PostgreSQL connection client #7605

merged 28 commits into from
Oct 1, 2024

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Jul 26, 2024

Implements a basic Rust postgres connection library and re-uses its internal state machine to replace the guts of pgcon.

User-facing changes:

  • Bugfix: EdgeDB will choose the smaller of server max_connections or the maximum number of connections defined for the user's current role to prevent hitting connection limits.
  • Change: sslmode: SSL is always requested unless sslmode=disable. If the server returns S to indicate SSL support and then fails to accept the user credentials, we do not retry without SSL.
  • Feature: Plain-text password authentication in backends is now supported, though discouraged.

Changes:

  • New 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.
  • New 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.
  • Major improvements to DSN parsing, as well as moving all DSN code to Rust. There is now a thin shim in 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:

  • 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).

Examples:

Parse a DSN:

cargo run --example dsn 'postgres://username@/database_name?host=path-to-socket'

Query a running server:

cargo run --example connect -- -U foo -P secret -u <<direct path to unix socket .s.PGSQL.5432>>

@mmastrac mmastrac force-pushed the pgrust branch 4 times, most recently from 641d9e5 to fc1b12b Compare August 14, 2024 19:06
@mmastrac mmastrac changed the title wip: Rust postgres connection Rust postgres connection Aug 14, 2024
@mmastrac mmastrac marked this pull request as ready for review August 14, 2024 19:28
@mmastrac mmastrac force-pushed the pgrust branch 3 times, most recently from fb539d3 to 8d0279e Compare August 17, 2024 15:46
@mmastrac mmastrac changed the title Rust postgres connection wip: Rust postgres connection Aug 21, 2024
@mmastrac mmastrac force-pushed the pgrust branch 5 times, most recently from cdd91f6 to bf8060b Compare August 26, 2024 19:00
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]>
@mmastrac mmastrac force-pushed the pgrust branch 2 times, most recently from 61a6e49 to b6f2118 Compare September 3, 2024 20:20
@mmastrac mmastrac force-pushed the pgrust branch 3 times, most recently from 198d86e to a763e48 Compare September 12, 2024 15:01
@mmastrac mmastrac force-pushed the pgrust branch 2 times, most recently from 8d76fd9 to 156e13d Compare September 13, 2024 16:30
@mmastrac mmastrac changed the title wip: Rust postgres connection Rust PostgreSQL connection client Sep 13, 2024
Copy link
Member

@fantix fantix left a 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)

@mmastrac mmastrac merged commit 1cc1fb3 into geldata:master Oct 1, 2024
23 checks passed
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants