-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
postgresql: provide PL/Perl, PL/Python and PL/Tcl as packages #385859
Conversation
427d335
to
6ccc655
Compare
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. |
6ccc655
to
e56bd16
Compare
Rebased and added release notes. This should build on this branch as well now, but will confirm. |
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
LLVM still broken on this branch for aarch64 (darwin+linux), but as said above, this builds fine on master. @Ma27 any objections? |
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 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.
I agree here.
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.
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 ;) |
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 |
05da693
to
d16b84c
Compare
I added the same interface to PL/R, which demonstrates the similarity / consistency of this approach. |
1f1abf0
to
4ccea07
Compare
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.
4ccea07
to
08883bb
Compare
Rebased to resolve merge conflicts. Will merge later today. |
This follows up on #372655 (comment) and allows this:
The same is done for
plperl
andpltcl
.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:tclSupport
norperlSupport
worked properly before.This is a breaking change for users of
pythonSupport
.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
Add a 👍 reaction to pull requests you find important.