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

postgresql: provide PL/Perl, PL/Python and PL/Tcl as packages #385859

Merged
merged 12 commits into from
Mar 21, 2025

Conversation

wolfgangwalther
Copy link
Contributor

@wolfgangwalther wolfgangwalther commented Feb 28, 2025

This follows up on #372655 (comment) and allows this:

postgresql.withPackages (pgps: [ pgps.plpython3.withPackages (pyps: [ pyps.base58 ]) ])

The same is done for plperl and pltcl.

While the extensions are split at run-time, they are still built in the main postgresql build and only stored in separate outputs. This allowed me to enable them by default, which has the following benefits:

  • Those code paths are tested, they were not before. Neither tclSupport nor perlSupport worked properly before.
  • Users avoid rebuilds when actually using them.
  • They are not provided out of the box, reducing the attack surface for stuff like https://www.postgresql.org/support/security/CVE-2024-10979 (and if only by being able to confidently say "nope, I don't have it installed").

This is a breaking change for users of pythonSupport.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
  • Tested, as applicable:
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@wolfgangwalther wolfgangwalther force-pushed the postgresql-python branch 2 times, most recently from 427d335 to 6ccc655 Compare February 28, 2025 20:53
@wolfgangwalther
Copy link
Contributor Author

Tested by cherry-picking onto master, since some reverse dependencies are currently broken on the base of staging that I am using here right now. All builds succeeded.

@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Mar 7, 2025
@wolfgangwalther
Copy link
Contributor Author

Rebased and added release notes. This should build on this branch as well now, but will confirm.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

LGTM

@wolfgangwalther
Copy link
Contributor Author

This should build on this branch as well now, but will confirm.

LLVM still broken on this branch for aarch64 (darwin+linux), but as said above, this builds fine on master.


@Ma27 any objections?

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 8, 2025
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

I'm thinking out loud for a moment:

We can easily build postgres with tcl/python/perl support by default given the closure-size overhead is rather small (and at least python/perl are on most systems anyways).
Now, wouldn't it be easier to give the wrapper the ability to not only add postgresql extensions to its env, but also tcl/perl/python packages? E.g.

postgresql.buildEnv {
  postgresqlExtensions = ps: [ ... ];
  python3Packages = ps: [ ... ];
  python3Interpreter = pkgs.python3;
}

and let postgresql.withPackages be a wrapper for this (I'd argue that this is still what you want in most cases and I'd expect the fallout of just deprecating it rather high).

I'm wondering if this is a little simpler than splitting the pl* part into their own outputs and then "re-importing" this via the extension loading wrapper.

The implementation looks good so far.

Oh, and I'd like to apologize for not reviewing this earlier: was kinda busy with university exam stuff and being sick, so I didn't have the spoons for reviewing larger PRs.

@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Mar 11, 2025

We can easily build postgres with tcl/python/perl support by default given the closure-size overhead is rather small (and at least python/perl are on most systems anyways).

I agree here.

Now, wouldn't it be easier to give the wrapper the ability to not only add postgresql extensions to its env, but also tcl/perl/python packages? E.g.
[...]
and let postgresql.withPackages be a wrapper for this (I'd argue that this is still what you want in most cases

The one thing you can't do easily with your approach: Get rid of PL/Python and PL/Perl. At least not without rebuilding. That's the cool thing about my approach: Everything is built, but you still have a choice!

This is something that people might want to be able to do. I certainly don't want to ship PL/Perl, when I don't need it.

I'm wondering if this is a little simpler than splitting the pl* part into their own outputs and then "re-importing" this via the extension loading wrapper.

It would probably turn out simpler, although I find the split not too bad. But I might be biased, just having written the code. Not sure how my future self would think about it in 6 months ;)

@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Mar 15, 2025

Now, wouldn't it be easier to give the wrapper the ability to not only add postgresql extensions to its env, but also tcl/perl/python packages? E.g.

postgresql.buildEnv {
  postgresqlExtensions = ps: [ ... ];
  python3Packages = ps: [ ... ];
  python3Interpreter = pkgs.python3;
}

One thing that came to my mind is how to deal with 3rd party extensions implementing procedural languages - those might need a similar interface.

We currently package PL/R and PL/v8, but there are others like PL/Rust, PL/Haskell, PL/Java etc. as well. I assume some of those might need similar interfaces to provide packages - so it would be more consistent overall to have all of them work the same way instead of having some of them work via postgresql.buildEnv and others via the extension itself.

@wolfgangwalther
Copy link
Contributor Author

We currently package PL/R and PL/v8, but there are others like PL/Rust, PL/Haskell, PL/Java etc. as well. I assume some of those might need similar interfaces to provide packages - so it would be more consistent overall to have all of them work the same way instead of having some of them work via postgresql.buildEnv and others via the extension itself.

I added the same interface to PL/R, which demonstrates the similarity / consistency of this approach.

@nix-owners nix-owners bot requested a review from qoelet March 15, 2025 11:42
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 15, 2025
@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Mar 17, 2025
This only applies to CFLAGS, not all of env.
We accidentally enabled building plperl in versions before 17
unconditionally, which was not intended.

Happened in be8612e.
This could be slightly faster and since "postgres" is called for every
connection, it could theoretically make a difference with high
throughput.
We don't need to pass it as build input anymore, because we pass it via
PYTHON environment variable already.
Perl is available in nativeBuildInputs, but the perl used at runtime,
must be a different one. Thus, we need to explicitly tell configure
about it, similar to how we did with python.
Can't find tcl anymore ever since we enabled strictDeps. Need to do the
same as for perl and python.
This allows us to always build the extension, but still have the user
explicitly enable it without causing rebuilds.
This allows us to always build the extension, but still have the user
explicitly enable it without causing rebuilds.
This allows us to always build the extension, but still have the user
explicitly enable it without causing rebuilds.
This allows passing packages to PL/R without rebuilding anything.
@wolfgangwalther
Copy link
Contributor Author

Rebased to resolve merge conflicts. Will merge later today.

@wolfgangwalther wolfgangwalther merged commit 783a290 into NixOS:staging Mar 21, 2025
23 of 27 checks passed
@wolfgangwalther wolfgangwalther deleted the postgresql-python branch March 21, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 501-1000 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants