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

libpq: init at 16.1 #234470

Closed
wants to merge 1 commit into from
Closed

libpq: init at 16.1 #234470

wants to merge 1 commit into from

Conversation

szlend
Copy link
Member

@szlend szlend commented May 27, 2023

Description of changes

Re: #61580

In my opinion libpq being a standalone derivation separate from postgresql makes the most sense. Here's why:

  • libpq is one of the most popular libraries out there. It's a dependency of every single application that connects to a postgres database. As such I feel it deserves special considerations.
  • libpq will likely be used in variety of different ways (static linking, cross-compilation, etc.). Being an output of the entire postgresql derivation causes additional friction as it has to build a ton of additional dependencies that may or may not work in those configurations. By minimizing the number of build-time dependencies we make the whole build process much smoother.
    • Having something like pkgsCross.aarch64-multiplatform.libpq just work is much nicer than playing whack-a-mole with (pkgsCross.aarch64-multiplatform.postgresql.overrides { ... }).lib, trying to figure out which magic set of overrides it will work with (or break in the next nixpkgs release).
  • Changing postgresql.lib to only include libpq is a breaking change and has so far caused a ton of bike-shedding conversations on what it should be, while 4 years later we're the only major package repository that doesn't ship this.
  • We don't need multiple packages of libpq. libpq is guaranteed to be backwards compatible so it only needs to be based on postgresql_<latest>.src.
  • Having one additional package that builds in under a minute in a package repository of 80k+ is not a serious concern
  • libpq is really easy to package on its own.
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

homepage = "https://www.postgresql.org";
description = "Client API library for PostgreSQL";
license = licenses.postgresql;
maintainers = with maintainers; [ szlend ];
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably would want to include existing postgresql maintainers.

pkgs/servers/sql/postgresql/libpq.nix Outdated Show resolved Hide resolved
pkgs/servers/sql/postgresql/libpq.nix Outdated Show resolved Hide resolved
@szlend szlend mentioned this pull request May 27, 2023
4 tasks
@@ -26027,6 +26027,8 @@ with pkgs;

timescaledb-tune = callPackage ../development/tools/database/timescaledb-tune { };

libpq = callPackage ../servers/sql/postgresql/libpq.nix { postgresql = postgresql_15; };
Copy link
Member Author

@szlend szlend Jun 2, 2023

Choose a reason for hiding this comment

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

This should always point to the latest release of postgresql. Maybe leave a comment? Or maybe we need a postgresql_latest alias?

I think we either need a postgresql_latest alias, or collaboration with existing postgresql maintainers to make sure this is always pointing to the latest version.

@szlend szlend marked this pull request as ready for review June 2, 2023 06:31
@szlend szlend changed the title libpq: init at 15.3 libpq: init at 16.0 Oct 9, 2023
@szlend
Copy link
Member Author

szlend commented Oct 9, 2023

Still relevant. Updated to 16.0 and rebased.

@mweinelt mweinelt requested review from ivan, globin and Ma27 November 12, 2023 19:51
@szlend szlend marked this pull request as draft December 9, 2023 14:38
@Ma27
Copy link
Member

Ma27 commented Dec 9, 2023

It's a dependency of every single application that connects to a postgres database. As such I feel it deserves special considerations.

What other distros solve by adding more packages can be solved in nixpkgs by adding outputs (most common example is .dev in nixpkgs vs -dev/-devel elsewhere), so it's not entirely fair to compare the existence of packages in this repository with the existence of packages in others.

Changing postgresql.lib to only include libpq is a breaking change and has so far caused a ton of bike-shedding conversations on what it should be, while 4 years later we're the only major package repository that doesn't ship this.

$ nix path-info -Sh /nix/store/pfl3xbpff2fw6l64d8n1qspb2dx7nl0c-postgresql-16.0-lib
/nix/store/pfl3xbpff2fw6l64d8n1qspb2dx7nl0c-postgresql-16.0-lib	 48.9M
$ nix path-info -Sh /nix/store/5r79b3yb2plzk0wgfvpz99rnk9w5vv3f-libpq-16.0
/nix/store/5r79b3yb2plzk0wgfvpz99rnk9w5vv3f-libpq-16.0	 41.5M

From a closure size perspective, this isn't much of a difference, so I think that using postgresql.lib is OK here or am I missing something?

I randomly checked a few packages that connect to postgres (hydra_unstable, gdal, lemmy-server) and all of them correctly depend on postgresql.lib in their runtime closure only.

Please correct me if I'm wrong, but that said, the main concern are cross-builds requiring libpq.so somewhere, correct?

Having one additional package that builds in under a minute in a package repository of 80k+ is not a serious concern

I have a different one though: you don't add libpq anywhere as it's mostly for cross/static builds (assuming my stance above is correct).

The concern I have is that on postgres upgrades, required fixes for this package will be either missed or an additional burden for the postgresql maintainers because it's e.g. necessary to find out which make incantations are actually needed to only get libpq and nothing else.

I'm somewhat afraid that libpq will only hide potential breakage the same way relying on .override switches does.

Also, in an ideal world, not much of .override {} should be needed in the first place: defaults can be chosen per-platform (e.g. fooSupport ? stdenv.isLinux as a very simple example). In fact I didn't need anything to cross-build postgresql.lib against pkgsCross.aarch64-multiplatform and pkgsMusl.

Regarding (purely) static builds: even with a quick eval fix from me (removing the stdenv'.hostPlatform.extensions.sharedLibrary) it broke with numerous undefined reference errors unfortunately. Would be curious to know what's up there, but right now the libpq.a from this package seems broken as well, so this problem won't be fixed by this change anyways.


Finally, please note that I'm only raising concerns I have as a package-maintainer. This is not a strict 👎, so please tell me if you disagree!

@nbdd0121
Copy link
Contributor

nbdd0121 commented Dec 9, 2023

I think the main issue with current postgresql.lib is that it couples the default postgres server version with the default library version. The main advantage of separate libpq is that we can keep libpq updated without touching the binary.

Or perhaps we should just create a postgresql_latest alias and always use postgresql_latest.lib instead of using postgresql.lib. EDIT: But doing so can be problematic because many of libraries in there (apart of libpq) might not be both backward compatible.

@szlend
Copy link
Member Author

szlend commented Dec 9, 2023

I can give a shot making postgresql.lib be libpq only. That would solve the postgresql_latest.lib compatibility issue, right?

@Ma27
Copy link
Member

Ma27 commented Dec 9, 2023

If a package is incompatible with a postgresl version for whatever reason shouldn't that error out loudly at build time?

@szlend szlend mentioned this pull request Dec 9, 2023
13 tasks
@szlend
Copy link
Member Author

szlend commented Dec 10, 2023

From a closure size perspective, this isn't much of a difference, so I think that using postgresql.lib is OK here or am I missing something?

It's a 8MB, or 92% reduction in the package size. I don't think it's fair to compare full closures as most of that size comes from openssl and glibc which are shared with pretty much every other package. It's not an insignificant difference when you're trying to ship compact container images. This is something nix/nixpkgs is supposed to be good at, right?

Please correct me if I'm wrong, but that said, the main concern are cross-builds requiring libpq.so somewhere, correct?

Yes, trying to cross-compile postgresql from darwin to linux has been incredibly painful as a lot of dependencies (that otherwise wouldn't be necessary for libpq) just didn't build. I initially had to do a lot of override workarounds to get it to work. At one point those overrides were spanning 3 layers deep into the dependency tree. I already upstreamed most of those fixes to nixpkgs, so it's a lot easier to cross-compile today. As of right now you only need to disable systemd support to get it to build, so it's not so bad anymore. It still takes a while to build all that though compared to just libpq in isolation. I ended up just using the custom libpq derivation for that reason alone.

The concern I have is that on postgres upgrades, required fixes for this package will be either missed or an additional burden for the postgresql maintainers because it's e.g. necessary to find out which make incantations are actually needed to only get libpq and nothing else.

I would argue it's probably easier to maintain libpq than the current set of patches and workarounds that try to make it work with a separate lib output. It feels like half of the workarounds in that derivation are for moving outputs around and fixing references. None of those would be necessary if libpq was a separate package.

I agree that collaboration with postgresql maintainers is a prerequisite for this to go through. They've been pretty resistant to supporting a separate libpq package though and at the of the day it's their time and effort so I'm not gonna try to push more work on them if that's how they feel about this change. I came up with an alternative libpq-only postgresql.lib proposal in #273175. It doesn't solve all my problems, but with most cross-compilation issues fixed, I think it's fine too.

pkgs/servers/sql/postgresql/libpq.nix Outdated Show resolved Hide resolved
src = postgresql.src;

configureFlags = [
"--with-openssl"
Copy link
Member

Choose a reason for hiding this comment

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

What about using lib/strings functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

buildPhase = ''
runHook preBuild

make -C src/bin/pg_config
Copy link
Member

Choose a reason for hiding this comment

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

There is no other way to build this?
It looks a strange thing to me:

You are basically doing a foreach-like loop.
What happens when you try to run each of them separately?
Can you install each of them separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out there's a submake-libpq makefile build target. But there's no equivalent for installing these unfortunately.

@szlend szlend changed the title libpq: init at 16.0 libpq: init at 16.1 Dec 23, 2023
installPhase = ''
runHook preInstall

make -C src/bin/pg_config install
Copy link
Member Author

Choose a reason for hiding this comment

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

This is wrong:

$ pg_config

...
LIBDIR = /nix/store/mvwrr8g7m7xk39k47qbnwsdg28cby2x2-libpq-16.1-dev/lib
...

This probably needs to be patched the same way as the complete postgres package. Hence it's necessary to reach an agreement with the maintainers first.

@szlend
Copy link
Member Author

szlend commented Apr 20, 2024

Closing in favor of #294504

@szlend szlend closed this Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants