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

chore(proxy): vendor a subset of rust-postgres #9930

Merged
merged 11 commits into from
Nov 29, 2024

Conversation

conradludgate
Copy link
Contributor

@conradludgate conradludgate commented Nov 28, 2024

Our rust-postgres fork is getting messy. Mostly because proxy wants more control over the raw protocol than tokio-postgres provides. As such, it's diverging more and more. Storage and compute also make use of rust-postgres, but in more normal usage, thus they don't need our crazy changes.

Idea:

  • proxy maintains their subset
  • other teams use a minimal patch set against upstream rust-postgres

Reviewing this code will be difficult. To implement it, I

  1. Copied tokio-postgres, postgres-protocol and postgres-types from https://github.com/neondatabase/rust-postgres/tree/00940fcdb57a8e99e805297b75839e7c4c7b1796
  2. Updated their package names with the 2 suffix to make them compile in the workspace.
  3. Updated proxy to use those packages
  4. Copied in the code from tokio-postgres-rustls 0.13 (with some patches applied correctly parse x509 certificates for channel binding jbg/tokio-postgres-rustls#32 remove unsafe code jbg/tokio-postgres-rustls#33)
  5. Removed as much dead code as I could find in the vendored libraries
  6. Updated the tokio-postgres-rustls code to use our existing channel binding implementation

Copy link

github-actions bot commented Nov 28, 2024

6310 tests run: 6020 passed, 0 failed, 290 skipped (full report)


Flaky tests (3)

Postgres 15

Postgres 14

Code coverage* (full report)

  • functions: 30.2% (8177 of 27038 functions)
  • lines: 47.7% (64724 of 135801 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
4abd7b0 at 2024-11-28T19:39:23.469Z :recycle:

@conradludgate conradludgate marked this pull request as ready for review November 29, 2024 08:58
@conradludgate conradludgate requested a review from a team as a code owner November 29, 2024 08:58
@conradludgate
Copy link
Contributor Author

conradludgate commented Nov 29, 2024

Future planned changes:

@conradludgate conradludgate changed the title [wip] chore(proxy): vendor a subset of rust-postgres chore(proxy): vendor a subset of rust-postgres Nov 29, 2024
@conradludgate
Copy link
Contributor Author

This is a net reduction in code to maintain. Instead of maintaining the full fork of rust-postgres

===============================================================================
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 Shell                   1          102           90            5            7
 Plain Text              1          497            0          410           87
 TOML                   10          315          280            0           35
 YAML                    1           10           10            0            0
-------------------------------------------------------------------------------
 Markdown                8          790            0          472          318
 |- Rust                 1           15           15            0            0
 (Total)                            805           15          472          318
-------------------------------------------------------------------------------
 Rust                  120        22627        19331           95         3201
 |- Markdown            55         2874           72         2346          456
 (Total)                          25501        19403         2441         3657
===============================================================================
 Total                 141        24341        19711          982         3648
===============================================================================

We only need to maintain a relatively simple patch set of 1600 lines and this new set of code

===============================================================================
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 Markdown                1            6            0            4            2
 TOML                    3           52           48            0            4
-------------------------------------------------------------------------------
 Rust                   41         9708         8212           74         1422
 |- Markdown            29         1201            0         1071          130
 (Total)                          10909         8212         1145         1552
===============================================================================
 Total                  45         9766         8260           78         1428
===============================================================================

8260 + 1600 << 19711

Copy link
Contributor

@cloneable cloneable left a comment

Choose a reason for hiding this comment

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

Looks good. Nice work!

I didn't review every line, not even every file. We will need to rely on testing here.

You added the rustls module to proxy proper so the new crates are and stay crypto provider agnostic?

@conradludgate
Copy link
Contributor Author

You added the rustls module to proxy proper so the new crates are and stay crypto provider agnostic?

There wasn't much reason to it for now. Just needed to get it to compile. I could have put it in it's own crate or even into tokio-postgres directly. I didn't think much about it at the time. We can shuffle stuff around later

@conradludgate conradludgate added this pull request to the merge queue Nov 29, 2024
Merged via the queue into main with commit 1d642d6 Nov 29, 2024
81 checks passed
@conradludgate conradludgate deleted the conrad/vendor-and-reduce-rust-postgres-for-proxy branch November 29, 2024 11:08
@awarus awarus mentioned this pull request Dec 5, 2024
awarus pushed a commit that referenced this pull request Dec 5, 2024
Our rust-postgres fork is getting messy. Mostly because proxy wants more
control over the raw protocol than tokio-postgres provides. As such,
it's diverging more and more. Storage and compute also make use of
rust-postgres, but in more normal usage, thus they don't need our crazy
changes.

Idea: 
* proxy maintains their subset
* other teams use a minimal patch set against upstream rust-postgres

Reviewing this code will be difficult. To implement it, I
1. Copied tokio-postgres, postgres-protocol and postgres-types from
https://github.com/neondatabase/rust-postgres/tree/00940fcdb57a8e99e805297b75839e7c4c7b1796
2. Updated their package names with the `2` suffix to make them compile
in the workspace.
3. Updated proxy to use those packages
4. Copied in the code from tokio-postgres-rustls 0.13 (with some patches
applied jbg/tokio-postgres-rustls#32
jbg/tokio-postgres-rustls#33)
5. Removed as much dead code as I could find in the vendored libraries
6. Updated the tokio-postgres-rustls code to use our existing channel
binding implementation
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