build: link storage controller with system libpq #10258
Closed
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
jcsp
added
t/bug
jcsp
force-pushed
the
jcsp/controller-system-libpq
branch
from
6a802d5 to
e0b329a
Compare
7095 tests run: 6797 passed, 0 failed, 298 skipped (full report)Flaky tests (2)Postgres 15
Postgres 14
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
e09b33b at 2025-01-02T15:40:36.244Z :recycle: |
jcsp
force-pushed
the
jcsp/controller-system-libpq
branch
from
e0b329a to
e09b33b
Compare
|
Related: #10108 Same comment as in #10108 applies, we use v16 instances in prod. However, here we need to use the debian package. But version 16 is not available in the default Debian 12 repos :/. I suppose it's okay to use the v15 client with v16 instances? |
jcsp
added a commit
that referenced
this pull request
Jan 3, 2025
This is for use by the storage controller, which links dynamically with `libpq`. We currently use the neon-built libpq, but this may be unsafe for use from multi-threaded programs like the controller, as it uses a statically linked openssl Precursor to #10258
This was referenced Jan 3, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 3, 2025
## Problem We are chasing down segfaults in the storage controller neondatabase/cloud#21010 This is for use by the storage controller, which links dynamically with `libpq`. We currently use the neon-built libpq, but this may be unsafe for use from multi-threaded programs like the controller, as it uses a statically linked openssl Precursor to #10258 ## Summary of changes - Include `postgresql-15` in container builds. The reason for using version 15 is simply because that is what's available in Debian 12 without adding any extra repositories, and we don't have any special need for latest version in our libpq usage.
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.
Problem
We see occasional storage controller segfaults (https://github.com/neondatabase/cloud/issues/21010), which are correlated with database connection errors, and it is known that using statically linked openssl from multi-threaded programs is risky (https://github.com/neondatabase/cloud/issues/16155)
Summary of changes
CARGO_CMD_PREFIXto link with our custom postgres build when building.Important: helm charts set LD_LIBRARY_PATH to point to a v16 path -- because we'll be building binaries against v15 (because that's what's available on debian bookworm), that might not work seamlessly, we might need to coordinate a helm chart change deploy with this change.
Why is it okay to only use statically-linked openssl for our other binaries and not for the storage controller? Because the purpose of that static linking is to make our postgres builds agnostic wrt the specific openssl version on the distro where we run. The controller doesn't need that, because it can use a totally vanilla upstream postgres for its client library, and that vanilla system postgres package will already use whichever openssl is available on the distro where we're running.