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

Overhaul PostgreSQL packaging, native extensions, and NixOS support #38698

Closed
wants to merge 61 commits into from

Conversation

@thoughtpolice
Copy link
Member

@thoughtpolice thoughtpolice commented Apr 10, 2018

Motivation for this change

I'm brief on time now, but this isn't done yet. See #38616. In short, this overhauls the namespacing and extension management of PostgreSQL packages, greatly easing the ability to test, build, and package PostgreSQL extensions properly. It also overhauls the NixOS module to support the proposed API in #38616, making it far easier to add custom extensions to your server in a correct way, no matter what version you choose.

I've been running these changes with some light testing for a few days, but I'm not done yet. I'll be amending and adding more comments to the commits and maybe squash one or two. These changes otherwise seem solid and shouldn't break anything.

Note that the NixOS module remains backwards compatible. Nothing should break, and you're unlikely to even notice if you don't use extensions. I consider this a good thing and ideally supporting it to 18.09 should not be terrible. However, warnings will be thrown when using the old APIs, and assertions are thrown if both are used at once in incompatible ways. (In fact you can even use a mix of the old and new APIs, in theory -- e.g. use the new .packages attribute with the old .extraPlugins, instead of the new .plugins.) I'm open to dropping backwards compatibility if other people think we can give it up for 18.09, though.

Finally, this also updates Nixpkgs and NixOS to use PostgreSQL 10 and libpq 10 by default. Although this isn't necessary, it seems like a reasonable time to make the push, since 9.6 has been the default since 17.09. PostgreSQL 11 will ideally be out around September, near the 18.09 release, but it's unlikely we'll have time to fit it in by then (depending on how things go for us, and them.)

I have also stepped up and added myself as the maintainer of the PostgreSQL module, and will be finishing the manual section.

I suggest reviewing the commits sequentially, in order, rather than at once. They are all individually small, work, and should be much easier to follow.

Things done
  • Actually finish writing the PostgreSQL section of the manual.

  • Look into deprecation warnings for old top-level attrs; using builtins.trace is annoying because e.g. it triggers on nix search -u ...

  • Look into #22653 and #38469 as well -- these should be fixed while I'm at it, IMO.

  • Bikeshed the naming of the new options? .packages is really close to .package, but I'm not sure of a better name. On the other hand, I think .plugin is a good name vs .extraPlugins

  • Migrate all tests and NixOS modules to new API

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)

  • Built on platform(s)

    • NixOS
    • macOS
    • other Linux distributions (Fedora 25, sandboxing enabled)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)

  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"

  • Tested execution of all binary files (usually in ./result/bin/)

  • Fits CONTRIBUTING.md.


@thoughtpolice
Copy link
Member Author

@thoughtpolice thoughtpolice commented Apr 11, 2018

Please note that GitHub hates actually showing you the linear stream of commits as it will appear in the history at merge-time, and instead always prefers showing things sorted-by-date, even when this is completely, totally, 100% factually wrong. I rebased and re-arranged a few commits... Ugh.

@thoughtpolice thoughtpolice force-pushed the thoughtpolice:pgsql-fixes branch 2 times, most recently Apr 16, 2018
@thoughtpolice
Copy link
Member Author

@thoughtpolice thoughtpolice commented Apr 16, 2018

Note to future archaeologists who will later discover my corpse here: this was the magical incantation needed to make GitHub display commits in order, as it should, by resetting the commit/author date of every individual commit to be 2-3 seconds apart.

for x in $(git log --format=oneline master..pgsql-fixes | tac | awk '{print $1}'); do
  git cherry-pick $x; git commit --amend --reset-author --no-edit;
  sleep 2;
done

Surely this will be the solution to an untold future terror and save the world, one day.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Apr 16, 2018

Really good work so far @thoughtpolice 👍

@thoughtpolice thoughtpolice force-pushed the thoughtpolice:pgsql-fixes branch 2 times, most recently Apr 20, 2018
@thoughtpolice thoughtpolice force-pushed the thoughtpolice:pgsql-fixes branch May 1, 2018
@domenkozar
Copy link
Member

@domenkozar domenkozar commented May 11, 2018

@thoughtpolice can this go as-is and then we add further improvements?

@basvandijk
Copy link
Member

@basvandijk basvandijk commented Sep 28, 2018

@thoughtpolice to fix the merge conflicts I've rebased pgsql-fixes on master again at my pgsql-fixes-master-rebase-2018-09-28 branch. Shall I force-push that to this PR or do you want to take the lead with this? Note that staging merges without conflicts into pgsql-fixes-master-rebase-2018-09-28.

@danbst
Copy link
Contributor

@danbst danbst commented Oct 2, 2018

API to override postgresqlPackages isn't exposed.

I can make an override for postgresql:

       pg11 = (import lumiPkgs { }).postgresql11Packages.postgresql.overrideAttrs (super: rec {
         name = "postgresql-${version}";
         version = "11beta4";
         src = pkgs.fetchurl {
           url = "mirror://postgresql/source/v${version}/${name}.tar.bz2";
           sha256 = "0qxlfh1a7bhhamrbs3msk71pny7jxx0c0fs26zlmp7jjn138zqi1";
         };
       });

but can't convert it into postgresql11beta4Packages - makePackageSet isn't exposed. And extensions won't be built against my patched postgresql.

@danbst
Copy link
Contributor

@danbst danbst commented Oct 3, 2018

I propose the following:

  1. Solve merge conflicts and merge to master. I've tried this PR (however in LumiGuide fashion) and pg11 works great!
  2. Don't backport to 18.09. This is contra Nixpkgs policy, too late.
@danbst
Copy link
Contributor

@danbst danbst commented Oct 3, 2018

They way, how extensions are filtered by PG version, isn't user friendly. For example, pg_repack is said to be compatible only with PG < 11, so it is absent from postgresql11Packages. This results into attribute pg_repack not found error, which wasn't obvious "why" (I know it should be there, because I've packaged it!).

Maybe show some warning "pg_repack extension is available only for PG < 11" too?

@danbst
Copy link
Contributor

@danbst danbst commented Oct 31, 2018

@thoughtpolice @basvandijk PG11 is released, let's merge this?

@thoughtpolice
Copy link
Member Author

@thoughtpolice thoughtpolice commented Nov 2, 2018

Unfortunately #48664 and related changes have caused a bit of a rebasing nightmare in the mean time due to attribute-related changes, so there's a cascade of fixes I need to do. This will also complicate the merge to staging, I think. I plan on spending the rest of the day trying to work this out but it'll probably take some time.

In the mean time I'm going to add myself as the owner of Postgres-related changes under Nixpkgs to make myself a blocking reviewer, so this doesn't happen in the future and I have time to review the changes people make, e.g. this PR already has PG 11 so the prior one was a bit redundant in the long run. (Not that anyone could be blamed about this, really, except for myself perhaps for not doing this sooner, and not keeping up with the changes people do submit!)

@thoughtpolice thoughtpolice mentioned this pull request Nov 2, 2018
3 of 9 tasks complete
thoughtpolice added a commit that referenced this pull request Nov 2, 2018
Since the development of #38698, I have been radically overhauling the
PostgreSQL support and plan on maintaining it in the long run, taking
over from @ocharles, who authored most of it initially.

However, while #38698 has not been merged (yet), I'm taking the time to
go ahead and add myself as the owner of all the related code, so that I
can be marked as a reviewer for things people submit in the mean time.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
@thoughtpolice
Copy link
Member Author

@thoughtpolice thoughtpolice commented Nov 3, 2018

Update: I made a lot of good progress yesterday but I basically slowly rewrote and compressed several of the patches from the original branch to be fewer in number, and easier to grok. I hope later today I'll be ready to overwrite this branch with a new version composed on staging instead and just slap the merge button.

@thoughtpolice
Copy link
Member Author

@thoughtpolice thoughtpolice commented Nov 4, 2018

4569ee7 also fixes a recent-ish plv8 build failure.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Nov 28, 2018

I think this PR is a good example why we encourage contributions to be split up into small pieces.

nixpkgs moves very fast and PRs that hang around half a year, accumulating changes across 50+ files are too big pain to review and merge in timely manner (given that we work async).

It's a shame, I love the work that @thoughtpolice did. Maybe time to start breaking it up?

@thoughtpolice
Copy link
Member Author

@thoughtpolice thoughtpolice commented Nov 28, 2018

I've mostly been gone for vacation and thanksgiving, FWIW. I just didn't get around to posting my staging delta, which is now substantially smaller than this PR (in number of commits and changes.) In fact, prior to my leave, I was piece-wise upstreaming several pieces directly into staging -- hence why I was doing several regular merges (master -> staging) a few weeks ago, to prep things (to make sure master wouldn't needlessly conflict, later on). So my changes have gotten smaller and more pointed, I believe.

The real issue is that multiple changes here cause mass-rebuilds and reordering them or splitting them up doesn't really change that fact, so you might as well change a bunch of it at once, if you ask me. From my count, my new branch has roughly 5 major commits that will rebuild the vast majority of all PostgreSQL builds and their downstream consumers. They each imply large, delicate changes so keeping them distinct is proper. I consider Hydra and Borg somewhat sacred resources so I try not to stress them too hard without a good reason. It also improves my cycle time to do this anyway: my local machine is extremely fast and can build hundreds of complex expressions on the order of minutes, giving me much better turnaround than borg, at least until I hit the 'long tail' of every-downstream consumer, where I just can't have enough cores. For instance, any delicate change to the layout of the postgresql expression causes all extensions and downstream consumers to rebuild, so you're looking at a minimum of something like ~100 builds just to do every variant of postgresqlPackages including all extensions, or close to a 1000 builds total if you build all downstream consumers.

This isn't really a case of me being "right" though, it's just a justification for why I've acted so. It could certainly be argued that's all a small price to get it in faster.

That said I understand the frustration; the reality is that my time is simply much slimmer these days (for a number of reasons, ranging from being busy to personal health) and I'm not actively paid to do maintenance like this, but at the time I needed it, I really wanted it. It probably does not help that I'm a fairly established maintainer -- I can willingly deal with occasional rebases and whatnot for my personal work and I have a good idea of what to do if things go wrong. But many ordinary users (as well as other devs) who want these changes now probably don't want to do that, understandably.

FWIW, my current delta looks like this for staging:

93e05631b04390a94ecbd7ebd44d8e5c93b81d7a postgresqlPackages.postgis: remove outdated comment
230049105532faf9e16954fde92b8859ced5055a postgresqlPackages: add new withPackages combinator, fix man page
68216f892e08d11af411b5907a95c813e494135c postgresql_11: use llvmPackages_7
eaac5719c672a8743141692e0f14945202108504 postgresql11: implement and enable JIT support
80e9a93579adabfbc63cb1310d4759fd42f1ccfc postgresqlPackages: make PGXS install phase Nix-compatible
6590a941ea9190af871ee51d192e86aec6bdf226 postgresql: always create $out/bin
bc6996beb431b4b925bd4cbad25145c85238000c nixos/tests: migrate to new postgresql api
9802c1eb06684d0ff3a16821bc82e513fc04dc25 nixos/tests: migrate postgis test to new postgres API
2dae770378b17efa617554f9a750b6a14a7694fb nixos/postgresql: give the plugins option the selectorFunction type
f9c33be8329f345f3ceb5dce322e83a663094474 nixos/postgresql: add new nixos module API
f195107caad0a26c533a715e45a249a375ee3038 postgresql: enable systemd service notification on 9.6+
9b86b39cc8792e03d3779992484d733da47663a3 postgresqlPackages: add versionCheck passthru for checking server compatibility
74ef5728a645fdda3b4f3b09e212d53c6e144593 nixpkgs: migrate postgis to new postgresqlPackages infrastructure
d0a41fde11b8706268a5f6692a13efda158f5952 nixpkgs: implement new postgresqlPackages infrastructure
c876b993145acdcd7e605faf9744690292b3a910 nixpkgs: reorganize the postgresql extensions
3afeb2d7895512754f68ef6335846346ffa7fd60 nixpkgs: move postgresql patches into a common directory

I can of course go ahead and merge that to staging. In fact it should work quite fine, I think. So perhaps I should finally start cycling borg once I get these changes posted. (Some of it is arguably out of scope, such as the LLVM work; this could also be removed, I suppose, but again it's going to cause a big rebuild anyway.)

This is quite minimal and isolated in my opinion; close to what you'd need to hit all the high points. The remaining points are:

  • Test migration isn't actually done
  • Manual needs to be rewritten (I had most of this but the merge conflicts were too great to work out, due to a pass of automatic formatting, so this is "done" but I have to reformat, prepare it, etc).

Other than that I do think it's almost ready. Realistically though I'm just very busy and testing all these combinations takes a bit of time with each iteration and packaging them up nicely; I think fundamentally the scope of the changes is a little large no matter how you slice it.

ingenieroariel added a commit to piensa/nur-packages that referenced this pull request Jan 11, 2019
@danbst
Copy link
Contributor

@danbst danbst commented Jan 17, 2019

@thoughtpolice probably we can split this PR somehow, to speedup merge? Don't want to miss pg11 in 19.03

danbst added a commit to danbst/nixpkgs that referenced this pull request Jan 19, 2019
Extracts some useful parts of NixOS#38698,
in particular, it's vision that postgresql plugins should be namespaced.

Original approach had several problems:
- not gonna happen in forseeable future
- did lots of deprecations
- was all-in-one solution, which is hard to sell to nixpkgs
- even that we have postgresqlPackages now, we can't do arbitrary overrides
  to postgresql and plugins. Several required functions were not exported

Here I've fixed all of those problems:
- deprecates nothing (though plugins were moved now into `aliases.nix`)
- this doesn't touch NixOS at all, and doesn't break anything
- hashes for plugins and PGs are not changed (I hope)
- no extra fixes to pg itself
- default PG is not changed
- plugins and PGs are extensible

Last one is the most interesting thing, because it introduces novel way
to manage `XXX withPackages` problem. It is novel, but has got lots of
inspiration from existing approaches:
- python, so we have now `postgresql.pkgs.*`, `postgresql_11.pkgs.*`
  which all contain plugins compiled with correct PG.
- python, so we have now `postgresql.withPackages` as well
- in contrast to python, there are no `postgresql11Packages`, only
  `postgresql_11.pkgs`
- NixOS#44196, so plugins are referenced starting at self-fixpoint.
  This allows override/add plugins with mere `//` in overlay. This works for
  both `postgresqlPackages` (overrides are applied to all postgresql_xx.pkgs)
  and `postgresql_xx.pkgs` (overrides are specific to this postgresql) sets
- I've made it compatible with proposed mergeable overlays (NixOS#54266)
  however this PR doesn't depend on it
- last, but not least, `postgresql/default.nix` is now an overlay! This
  replaces previous `callPackages` approach with a modern, extensible concept.
@danbst danbst mentioned this pull request Jan 19, 2019
3 of 10 tasks complete
@danbst
Copy link
Contributor

@danbst danbst commented Jan 19, 2019

I've started split. My plan:

  • postgresqlPackages, and make them extensible! #54319
  • fix stuff, so all should work with pg11
  • change default to pg11. This is minimal plan for 19.03. Eveything else can go step by step
  • enable JIT in pg11
  • make aliases low-prio, so they don't appear in nix search. Instead, force postgresqlPackages to be present there
  • rething NixOS module options (in regard to plugins)
  • everything else
@thoughtpolice
Copy link
Member Author

@thoughtpolice thoughtpolice commented Jan 19, 2019

I had actually started up on this again last week, but I've basically been in-and-out of the house quite a lot. I also split up some things that I think can go into staging quicker now, so I'll try to take a look at #54319 when I get a chance and we'll see if we can meet in the middle.

@thoughtpolice thoughtpolice mentioned this pull request Jan 19, 2019
2 of 10 tasks complete
danbst added a commit to danbst/nixpkgs that referenced this pull request Jan 20, 2019
Extracts some useful parts of NixOS#38698,
in particular, it's vision that postgresql plugins should be namespaced.

Original approach had several problems:
- not gonna happen in forseeable future
- did lots of deprecations
- was all-in-one solution, which is hard to sell to nixpkgs
- even that we have postgresqlPackages now, we can't do arbitrary overrides
  to postgresql and plugins. Several required functions were not exported

Here I've fixed all of those problems:
- deprecates nothing (though plugins were moved now into `aliases.nix`)
- this doesn't touch NixOS at all, and doesn't break anything
- hashes for plugins and PGs are not changed (I hope)
- no extra fixes to pg itself
- default PG is not changed
- plugins and PGs are extensible

Last one is the most interesting thing, because it introduces novel way
to manage `XXX withPackages` problem. It is novel, but has got lots of
inspiration from existing approaches:
- python, so we have now `postgresql.pkgs.*`, `postgresql_11.pkgs.*`
  which all contain plugins compiled with correct PG.
- python, so we have now `postgresql.withPackages` as well
- in contrast to python, there are no `postgresql11Packages`, only
  `postgresql_11.pkgs`
- NixOS#44196, so plugins are referenced starting at self-fixpoint.
  This allows override/add plugins with mere `//` in overlay. This works for
  both `postgresqlPackages` (overrides are applied to all postgresql_xx.pkgs)
  and `postgresql_xx.pkgs` (overrides are specific to this postgresql) sets
- I've made it compatible with proposed mergeable overlays (NixOS#54266)
  however this PR doesn't depend on it
- last, but not least, `postgresql/default.nix` is now an overlay! This
  replaces previous `callPackages` approach with a modern, extensible concept.
danbst added a commit to danbst/nixpkgs that referenced this pull request Jan 21, 2019
Another part of NixOS#38698, though I did cleanup even more.
Moving docs to separate output should save another 30MB.

I did pin poppler to 0.61 just to be sure GDAL doesn't break again next
time poppler changes internal APIs.
danbst added a commit to danbst/nixpkgs that referenced this pull request Jan 26, 2019
* postgresql: reorganize package and it's extensions

Extracts some useful parts of NixOS#38698,
in particular, it's vision that postgresql plugins should be namespaced.
danbst added a commit that referenced this pull request Jan 26, 2019
postgis: cleanup

Another part of #38698, though I did cleanup even more.
Moving docs to separate output should save another 30MB.

I did pin poppler to 0.61 just to be sure GDAL doesn't break again next
time poppler changes internal APIs.
@FRidh FRidh modified the milestones: 19.03, 19.09 May 12, 2019
@danbst
Copy link
Contributor

@danbst danbst commented Dec 15, 2019

@thoughtpolice this PR is heavily outdated now, I'll close it. Most of this stuff is already ported. The only non-ported things (AFAIK):

  • JIT support
  • PG version constraints for extensions

Feel free to open a PRs for that.

@danbst danbst closed this Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.