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 extension packaging could be improved #38616

Closed
thoughtpolice opened this issue Apr 8, 2018 · 12 comments
Closed

PostgreSQL extension packaging could be improved #38616

thoughtpolice opened this issue Apr 8, 2018 · 12 comments

Comments

@thoughtpolice
Copy link
Member

thoughtpolice commented Apr 8, 2018

Problem Description

Currently, when using PostgreSQL on NixOS, I see two major annoyances when using Postgresql extensions.


Problem 1: Extensions must be explicitly overridden to refer to the right PostgreSQL package

By default, Nixpkgs sets the top-level postgresql attribute to an arbitrary version. At the time of this writing, this version is 9.6, while the latest stable version available is actually 10.2.

However, all packaged extensions (postgis, pg_hll, timescaledb, and more) are currently only built against the postgres attribute (equivalently: they are only built as 9.6 extensions.) This means that if I want to use PostgreSQL 10.2 on my NixOS machines, I must explicitly override the postgres argument to point to the right package. Otherwise, things aren't going to work when I run CREATE EXTENSION in postgresql v10 and it tries to load a .so file built for v9.6.

As an example, here's a service description, snipped from my nixos configurations:

  services.postgresql =
    let
      mkPlug = p: p.override { postgresql = config.services.postgresql.package; };
    in {
      enable = true;
      package = pkgs.postgresql100;
      dataDir = "/data/pgsql";

      extraConfig = "shared_preload_libraries = 'timescaledb'";
      extraPlugins = with pkgs; map mkPlug
        [ postgis
          timescaledb
          cstore_fdw
          pg_hll
          pg_cron
          pg_topn
        ];
    };

The problem here is that this is error prone (it's easy to forget), and even after that it's relatively non-obvious to a user how to properly configure the extensions unless they're already Nix-savvy. It's not unreasonable to want the most recent version of Postgres on a greenfield deployment, while the Nixpkgs default version may be different.

Ideally, extraPlugins really should do this attribute mapping itself, IMO. But at the same time...


Problem 2: The set of postgresql versions and extensions is not namespaced

If we are going to have multiple PostgreSQL versions, each with extensions, I strongly suggest we namespace them appropriately, yielding a top-level attribute set that can be used.

For example, we could have something like the following in all-packages.nix:

postgresql96Packages = ...;
postgresql10Packages = ...;

postgresqlPackages = postgresql96Packages;

Then, much like Linux kernel modules, a PostgreSQL extension for a particular version will be available under the corresponding attrset; e,g.

postgresql96Packages.timescaledb
postgresql96Packages.pg_hll
...

The motivation here is twofold, for me: one, these packages can't even be used without postgres, so having them in the top-level namespace seems wrong. Second, as a result of the first point, finding what extensions we support and updating them will be significantly easier IMO. Right now the only convenient thing tying all these packages together is that all related expressions are under ./pkgs/servers/sql/postgresql/. This should be reflected in the namespace, IMO.


Napkin sketch of new module API

If we had something like the above, we could simplify the above example of using Postgresql 10.x to something like:

  services.postgresql.enable = true;
  services.postgresql.dataDir = "/data/pgsql";
  services.postgresql.packages = pkgs.postgresql10Packages;
  services.postgresql.plugins = pgsqlPkgs: with pgsqlPkgs;
    [ postgis
      timescaledb
      cstore_fdw
      pg_hll
      pg_cron
      pg_topn
    ];
  services.postgresql.extraConfig = "shared_preload_libraries = 'timescaledb'";

Namely, the services.postgresql.plugins attribute contains a function which takes the attrset of all PostgreSQL extension packages as an argument, as specified by .packages. Essentially you can think of it like a kind of filter predicate that picks out the extensions you want, from the attrset containing every possible extension. By default this would be _: [] or const [].

@thoughtpolice
Copy link
Member Author

thoughtpolice commented Apr 8, 2018

I'd also like to throw out there that postgresql100 is actually a total misnomer as an attribute name, too.

In the past, PostgreSQL had a supermajor.major.minor versioning system. Nixpkgs reflects this only using the naming scheme of postgresql<supermajor><major>. Point releases are not considered, as they have compatible on-disk formats and are simply bugfixes. Starting with v10, however, this changed to a major.minor versioning scheme. This means that 10.0 is the first release, 10.1 is the first patch release, 10.2 is the second patch release, etc... And 11.0 will be the next major release, and the first of the 11.x series.

Thus, the set of PostgreSQL packages for version 10 and beyond should have the naming convention postgresql<major> and nothing else. So we should really rename postgresql100 to postgresql10, and make sure for 11 it's only ever postgresql11...

@thoughtpolice thoughtpolice changed the title Postgresql extension packaging could be improved PostgreSQL extension packaging could be improved Apr 8, 2018
@thoughtpolice
Copy link
Member Author

Another related issue to suss out, apparently having something really damn annoying to do with store permissions and the hacks we use to bundle the right libdirs: #38469

@thoughtpolice
Copy link
Member Author

master...thoughtpolice:pgsql-fixes for more

@thoughtpolice
Copy link
Member Author

thoughtpolice commented Apr 9, 2018

They might have to be changes to always use the latest available client library (10.x instead of 9.6), even if the running server is 9.x

I personally don't see this as much of an issue. Picking a stable variant (more or less arbitrarily, I admit) and setting it as the default postgres attribute and letting libraries link against its chosen libpq is fine, IMO. AFAIK, the PostgreSQL protocol is very stable, and while not all clients support every feature, it doesn't just go and break things randomly. Unless a package explicitly requires a newer version, or it requires it for a feature that cannot be worked around (which would be an issue anyway since if the feature was mandatory, it wouldn't work against older postgres, e.g. SCRAM authoentication), I see no need to override all uses of postgresql (or whatever) to the latest version.

If #22653 is not relevant as of today, then it might be closed and "mkdir -p $out/bin" removed from other extensions too

There is this and the related #38469 I want to look into... The extension is in a state of some disarray and it's rather surprising it's gone on this long. (My indications seem to tell me almost nobody uses anything except postgis, honestly, or at least they always use it, which has masked related issues)

@thoughtpolice
Copy link
Member Author

(Oh, all that said @volth -- if you look at my branch, I actually did bump PostgreSQL to 10.x by default for 18.09 :)

@domenkozar
Copy link
Member

+1 for the proposal @thoughtpolice

@thoughtpolice
Copy link
Member Author

FWIW, #38698 now fixes all these issues, and many more.

thoughtpolice added a commit to thoughtpolice/nixpkgs that referenced this issue Aug 6, 2018
See NixOS#38616. This is a minor issue, and for now we keep around the
old attribute name with a warning to tell people how to switch.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
thoughtpolice added a commit to thoughtpolice/nixpkgs that referenced this issue Aug 14, 2018
See NixOS#38616. This is a minor issue, and for now we keep around the
old attribute name with a warning to tell people how to switch.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
basvandijk pushed a commit to LumiGuide/nixpkgs that referenced this issue Sep 9, 2018
See NixOS#38616. This is a minor issue, and for now we keep around the
old attribute name with a warning to tell people how to switch.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
(cherry picked from commit 677c607)
basvandijk pushed a commit to LumiGuide/nixpkgs that referenced this issue Sep 10, 2018
See NixOS#38616. This is a minor issue, and for now we keep around the
old attribute name with a warning to tell people how to switch.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
basvandijk pushed a commit to LumiGuide/nixpkgs that referenced this issue Sep 28, 2018
See NixOS#38616. This is a minor issue, and for now we keep around the
old attribute name with a warning to tell people how to switch.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
@danbst
Copy link
Contributor

danbst commented Jul 16, 2019

State of things

Now second problem is kinda solved, as we have postgresql.pkgs, postgresql11.pkgs and so on for postgresql plugins. We also have postgresql.withPackages (ps: [...]) which builds a postgresql environment with specific plugins (function extracted from NixOS module).

So, the first problem can now be solved in 2 ways:
1.

services.postgresql =
    let
      package = pkgs.postgresql11;
    in {
      enable = true;
      package = package;
      extraConfig = "shared_preload_libraries = 'timescaledb'";
      extraPlugins = with package.pkgs;
        [ postgis
          timescaledb
          cstore_fdw
          pg_hll
          pg_cron
          pg_topn
        ];
    };
services.postgresql =
    let
      package = pkgs.postgresql11.withPackages (ps: with ps; [
          postgis
          timescaledb
          cstore_fdw
          pg_hll
          pg_cron
          pg_topn
      ]);
    in {
      enable = true;
      package = package;
      extraConfig = "shared_preload_libraries = 'timescaledb'";
    };

Not sure about mergeability of configs in last example...

Docs are missing though.

@danbst
Copy link
Contributor

danbst commented Jul 23, 2019

I'm closing this, as it's possible now to

services.postgresql.extraPlugins = with config.services.postgresql.package.pkgs; [
   pg_repack
   postgis
   ...
];

@danbst danbst closed this as completed Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants
@thoughtpolice @domenkozar @danbst and others