-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add ability to link libduckdb statically #618
Conversation
Should we make one of the CI jobs use |
Static:
Shared:
|
pg_duckdb is not the only Postgres extension using DuckDB these days. If two extensions both dynamically link to libduckdb.so and install that library into the Postgres libdir, then it is a race to see which extension installs their libduckdb last. This can lead to undefined symbols in any of the DuckDB Postgres extensions. Signed-off-by: Tristan Partin <[email protected]>
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.
Thanks for the work on this. Some notes:
- I get the following error when using this locally, so something is not fully working it seems. I assume you don't get this error?
2025-02-21 13:44:45.002 CET [184341] FATAL: could not load library "/home/jelte/.pgenv/pgsql-17beta9/lib/pg_duckdb.so": /home/jelte/.pgenv/pgsql-17beta9/lib/pg_duckdb.so: undefined symbol: _ZTVN6duckdb21CachedHttpfsExtensionE
- The
install-duckdb
make target (which is called byinstall
too) should not install the.a
file to the postgres directory. - Regarding CI: yeah I think it would be good if one of the jobs used static linking.
Okay I pushed a few changes to make the resulting |
The |
if [ '${{ matrix.version }}' = REL_17_STABLE ]; then | ||
ERROR_ON_WARNING=1 make -j8 install DUCKDB_BUILD=Static |
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.
It seems error-prone to only build one version that way no?
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.
I'm not too worried about the static library build of DuckDB working for one PG version, but not for another. I mainly added this CI coverage to make sure we don't accidentally break the static build logic in our makefile completely. So I think running it for just one version is fine. I'll add a code comment explaining that though.
Makefile
Outdated
else ifeq ($(DUCKDB_BUILD), Static) | ||
DUCKDB_BUILD_CXX_FLAGS = | ||
DUCKDB_BUILD_TYPE = release | ||
DUCKDB_MAKE_TARGET = bundle-library |
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.
static
vs dynamic
and debug
vs release
are orthogonal concepts.
Can we have ReleaseStatic
instead? (and we'll add DebugStatic
if/when need be.
(I don't think it's worth it to rename Release
/Debug
into ReleaseDynamic
/DebugDynamic
)
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.
I totally agree, but DuckDB its make targets don't. There's no way to build a "debug" static library version currently, because the bundle-library hardcodes the release path. I'll rename it to ReleaseStatic though, because I agree that's clearer.
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.
LGTM, thanks!
pg_duckdb is not the only Postgres extension using DuckDB these days. If two extensions both dynamically link to libduckdb.so and install that library into the Postgres libdir, then it is a race to see which extension installs their libduckdb last. This can lead to undefined symbols in any of the DuckDB Postgres extensions.