postgresql: split dev output

wolfgangwalther · wolfgangwalther · commit 435f51c37faf · 2024-08-23T21:37:49.000+02:00
This splits a dev output to make the default output not depend on any
build dependencies anymore. This also avoids removing references from
pgxs' Makefile this way, which should, at least theoretically, be good
to build extensions via pgxs, making sure they use the same tooling.

ecpg is the "embedded SQL C preprocessor", which is certainly a dev
tool.

Most important, for closure size anyway, is to move pg_config to the dev
output, since it retains paths to all the other outputs.

The only thing with references to the dev output remaining is then the
postgres binary itself. It contains all the output paths, because it
shows those in the pg_config system view. There is no other way than
to nuke those references to avoid circular dependencies between outputs
- and blowing up closure size again.
diff --git a/nixos/doc/manual/release-notes/rl-2411.section.md b/nixos/doc/manual/release-notes/rl-2411.section.md
@@ -298,6 +298,8 @@
 
 - Docker now defaults to 27.x, because version 24.x stopped receiving security updates and bug fixes after [February 1, 2024](https://github.com/moby/moby/pull/46772#discussion_r1686464084).
 
+- `postgresql` was split into default and -dev outputs. To make this work without circular dependencies, the output of the `pg_config` system view has been removed. The `pg_config` binary is provided in the -dev output and still works as expected.
+
 - `keycloak` was updated to version 25, which introduces new hostname related options.
   See [Upgrading Guide](https://www.keycloak.org/docs/25.0.1/upgrading/#migrating-to-25-0-0) for instructions.
 
diff --git a/nixos/modules/services/databases/postgresql.md b/nixos/modules/services/databases/postgresql.md
@@ -362,3 +362,7 @@ postgresql.withJIT.pname
 ```
 
 evaluates to `"foobar"`.
+
+## Notable differences to upstream
+
+- To avoid circular dependencies between default and -dev outputs, the output of the `pg_config` system view has been removed.
diff --git a/pkgs/servers/sql/postgresql/generic.nix b/pkgs/servers/sql/postgresql/generic.nix
@@ -6,6 +6,7 @@ let
       , glibc, zlib, readline, openssl, icu, lz4, zstd, systemd, libossp_uuid
       , pkg-config, libxml2, tzdata, libkrb5, substituteAll, darwin
       , linux-pam
+      , removeReferencesTo
 
       # This is important to obtain a version of `libpq` that does not depend on systemd.
       , systemdSupport ? lib.meta.availableOn stdenv.hostPlatform systemd && !stdenv.hostPlatform.isStatic
@@ -64,10 +65,26 @@ let
 
     hardeningEnable = lib.optionals (!stdenv'.cc.isClang) [ "pie" ];
 
-    outputs = [ "out" "lib" "doc" "man" ];
-    setOutputFlags = false; # $out retains configureFlags :-/
+    outputs = [ "out" "dev" "doc" "lib" "man" ];
+    outputChecks.out = {
+      disallowedReferences = [ "dev" "doc" "man" ];
+      disallowedRequisites = [
+        stdenv'.cc
+      ] ++ (
+        map lib.getDev (builtins.filter (drv: drv ? "dev") finalAttrs.buildInputs)
+      ) ++ lib.optionals jitSupport [
+        llvmPackages.llvm.out
+      ];
+    };
     outputChecks.lib = {
-      disallowedReferences = [ "out" "doc" "man" ];
+      disallowedReferences = [ "out" "dev" "doc" "man" ];
+      disallowedRequisites = [
+        stdenv'.cc
+      ] ++ (
+        map lib.getDev (builtins.filter (drv: drv ? "dev") finalAttrs.buildInputs)
+      ) ++ lib.optionals jitSupport [
+        llvmPackages.llvm.out
+      ];
     };
 
     buildInputs = [
@@ -90,6 +107,7 @@ let
     nativeBuildInputs = [
       makeWrapper
       pkg-config
+      removeReferencesTo
     ]
       ++ lib.optionals jitSupport [ llvmPackages.llvm.dev nukeReferences patchelf ];
 
@@ -116,7 +134,6 @@ let
       "--with-libxml"
       "--with-icu"
       "--sysconfdir=/etc"
-      "--libdir=$(lib)/lib"
       "--with-system-tzdata=${tzdata}/share/zoneinfo"
       "--enable-debug"
       (lib.optionalString systemdSupport' "--with-systemd")
@@ -133,6 +150,7 @@ let
 
     patches = [
       (if atLeast "16" then ./patches/relative-to-symlinks-16+.patch else ./patches/relative-to-symlinks.patch)
+      (if atLeast "15" then ./patches/empty-pg-config-view-15+.patch else ./patches/empty-pg-config-view.patch)
       ./patches/less-is-more.patch
       ./patches/paths-for-split-outputs.patch
       ./patches/paths-with-postgresql-suffix.patch
@@ -154,43 +172,52 @@ let
 
     postPatch = ''
       substituteInPlace "src/Makefile.global.in" --subst-var out
-      # Hardcode the path to pgxs so pg_config returns the path in $out
-      substituteInPlace "src/common/config_info.c" --subst-var out
+      # Hardcode the path to pgxs so pg_config returns the path in $dev
+      substituteInPlace "src/common/config_info.c" --subst-var dev
     '';
 
     postInstall =
       ''
-        moveToOutput "lib/libpgcommon*.a" "$out"
-        moveToOutput "lib/libpgport*.a" "$out"
-
-        # Prevent a retained dependency on gcc-wrapper.
-        substituteInPlace "$out/lib/pgxs/src/Makefile.global" --replace ${stdenv'.cc}/bin/ld ld
+        moveToOutput "bin/ecpg" "$dev"
+        moveToOutput "lib/pgxs" "$dev"
+
+        # Pretend pg_config is located in $out/bin to return correct paths, but
+        # actually have it in -dev to avoid pulling in all other outputs.
+        moveToOutput "bin/pg_config" "$dev"
+        # To prevent a "pg_config: could not find own program executable" error, we fake
+        # pg_config in the default output.
+        cat << EOF > "$out/bin/pg_config" && chmod +x "$out/bin/pg_config"
+        #!${stdenv'.shell}
+        echo The real pg_config can be found in the -dev output.
+        exit 1
+        EOF
+        wrapProgram "$dev/bin/pg_config" --argv0 "$out/bin/pg_config"
+
+        # postgres exposes external symbols get_pkginclude_path and similar. Those
+        # can't be stripped away by --gc-sections/LTO, because they could theoretically
+        # be used by dynamically loaded modules / extensions. To avoid circular dependencies,
+        # references to -dev, -doc and -man are removed here. References to -lib must be kept,
+        # because there is a realistic use-case for extensions to locate the /lib directory to
+        # load other shared modules.
+        remove-references-to -t "$dev" -t "$doc" -t "$man" "$out/bin/postgres"
 
         if [ -z "''${dontDisableStatic:-}" ]; then
           # Remove static libraries in case dynamic are available.
-          for i in $out/lib/*.a $lib/lib/*.a; do
+          for i in $lib/lib/*.a; do
             name="$(basename "$i")"
             ext="${stdenv'.hostPlatform.extensions.sharedLibrary}"
             if [ -e "$lib/lib/''${name%.a}$ext" ] || [ -e "''${i%.a}$ext" ]; then
               rm "$i"
             fi
           done
         fi
+        # The remaining static libraries are libpgcommon.a, libpgport.a and related.
+        # Those are only used when building e.g. extensions, so go to $dev.
+        moveToOutput "lib/*.a" "$dev"
       '' + lib.optionalString jitSupport ''
         # In the case of JIT support, prevent a retained dependency on clang-wrapper
-        substituteInPlace "$out/lib/pgxs/src/Makefile.global" --replace ${stdenv'.cc}/bin/clang clang
         nuke-refs $out/lib/llvmjit_types.bc $(find $out/lib/bitcode -type f)
 
-        # Stop out depending on the default output of llvm
-        substituteInPlace $out/lib/pgxs/src/Makefile.global \
-          --replace ${llvmPackages.llvm.out}/bin "" \
-          --replace '$(LLVM_BINPATH)/' ""
-
-        # Stop out depending on the -dev output of llvm
-        substituteInPlace $out/lib/pgxs/src/Makefile.global \
-          --replace ${llvmPackages.llvm.dev}/bin/llvm-config llvm-config \
-          --replace -I${llvmPackages.llvm.dev}/include ""
-
         ${lib.optionalString (!stdenv'.isDarwin) ''
           # Stop lib depending on the -dev output of llvm
           rpath=$(patchelf --print-rpath $out/lib/llvmjit.so)
@@ -210,8 +237,6 @@ let
     # autodetection doesn't seem to able to find this, but it's there.
     checkTarget = "check";
 
-    disallowedReferences = [ stdenv'.cc ];
-
     passthru = let
       this = self.callPackage generic args;
       jitToggle = this.override {
diff --git a/pkgs/servers/sql/postgresql/patches/empty-pg-config-view-15+.patch b/pkgs/servers/sql/postgresql/patches/empty-pg-config-view-15+.patch
@@ -0,0 +1,54 @@
+Empty the pg_config system information view. This view keeps references to
+several -dev outputs, which we want to avoid to keep closure size down.
+
+The alternative to this patch would be to nuke references across the board,
+but this will also affect the output of the pg_config utility. By emptying
+the view only, we keep the pg_config binary intact. It resides in the -dev
+output, so it's fine to have all those references there.
+
+---
+--- a/src/backend/utils/misc/pg_config.c
++++ b/src/backend/utils/misc/pg_config.c
+@@ -32,20 +32,5 @@ pg_config(PG_FUNCTION_ARGS)
+ 	/* initialize our tuplestore */
+ 	InitMaterializedSRF(fcinfo, 0);
+ 
+-	configdata = get_configdata(my_exec_path, &configdata_len);
+-	for (i = 0; i < configdata_len; i++)
+-	{
+-		Datum		values[2];
+-		bool		nulls[2];
+-
+-		memset(values, 0, sizeof(values));
+-		memset(nulls, 0, sizeof(nulls));
+-
+-		values[0] = CStringGetTextDatum(configdata[i].name);
+-		values[1] = CStringGetTextDatum(configdata[i].setting);
+-
+-		tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
+-	}
+-
+ 	return (Datum) 0;
+ }
+--- a/src/test/regress/expected/sysviews.out
++++ b/src/test/regress/expected/sysviews.out
+@@ -29,7 +29,7 @@ select name, ident, parent, level, total_bytes >= free_bytes
+ (1 row)
+ 
+ -- At introduction, pg_config had 23 entries; it may grow
+-select count(*) > 20 as ok from pg_config;
++select count(*) = 0 as ok from pg_config;
+  ok 
+ ----
+  t
+--- a/src/test/regress/sql/sysviews.sql
++++ b/src/test/regress/sql/sysviews.sql
+@@ -18,7 +18,7 @@ select name, ident, parent, level, total_bytes >= free_bytes
+   from pg_backend_memory_contexts where level = 0;
+ 
+ -- At introduction, pg_config had 23 entries; it may grow
+-select count(*) > 20 as ok from pg_config;
++select count(*) = 0 as ok from pg_config;
+ 
+ -- We expect no cursors in this test; see also portals.sql
+ select count(*) = 0 as ok from pg_cursors;
diff --git a/pkgs/servers/sql/postgresql/patches/empty-pg-config-view.patch b/pkgs/servers/sql/postgresql/patches/empty-pg-config-view.patch
@@ -0,0 +1,50 @@
+Empty the pg_config system information view. This view keeps references to
+several -dev outputs, which we want to avoid to keep closure size down.
+
+The alternative to this patch would be to nuke references across the board,
+but this will also affect the output of the pg_config utility. By emptying
+the view only, we keep the pg_config binary intact. It resides in the -dev
+output, so it's fine to have all those references there.
+
+---
+--- a/src/backend/utils/misc/pg_config.c
++++ b/src/backend/utils/misc/pg_config.c
+@@ -69,16 +69,6 @@ pg_config(PG_FUNCTION_ARGS)
+ 	/* initialize our tuplestore */
+ 	tupstore = tuplestore_begin_heap(true, false, work_mem);
+ 
+-	configdata = get_configdata(my_exec_path, &configdata_len);
+-	for (i = 0; i < configdata_len; i++)
+-	{
+-		values[0] = configdata[i].name;
+-		values[1] = configdata[i].setting;
+-
+-		tuple = BuildTupleFromCStrings(attinmeta, values);
+-		tuplestore_puttuple(tupstore, tuple);
+-	}
+-
+ 	/*
+ 	 * no longer need the tuple descriptor reference created by
+ 	 * TupleDescGetAttInMetadata()
+--- a/src/test/regress/expected/sysviews.out
++++ b/src/test/regress/expected/sysviews.out
+@@ -20,7 +20,7 @@ select count(*) >= 0 as ok from pg_available_extensions;
+ (1 row)
+ 
+ -- At introduction, pg_config had 23 entries; it may grow
+-select count(*) > 20 as ok from pg_config;
++select count(*) = 0 as ok from pg_config;
+  ok 
+ ----
+  t
+--- a/src/test/regress/sql/sysviews.sql
++++ b/src/test/regress/sql/sysviews.sql
+@@ -13,7 +13,7 @@ select count(*) >= 0 as ok from pg_available_extension_versions;
+ select count(*) >= 0 as ok from pg_available_extensions;
+ 
+ -- At introduction, pg_config had 23 entries; it may grow
+-select count(*) > 20 as ok from pg_config;
++select count(*) = 0 as ok from pg_config;
+ 
+ -- We expect no cursors in this test; see also portals.sql
+ select count(*) = 0 as ok from pg_cursors;
diff --git a/pkgs/servers/sql/postgresql/patches/paths-for-split-outputs.patch b/pkgs/servers/sql/postgresql/patches/paths-for-split-outputs.patch
@@ -4,7 +4,7 @@
  	i++;
 
  	configdata[i].name = pstrdup("PGXS");
-+	strlcpy(path, "@out@/lib", sizeof(path));
++	strlcpy(path, "@dev@/lib", sizeof(path));
 -	get_pkglib_path(my_exec_path, path);
  	strlcat(path, "/pgxs/src/makefiles/pgxs.mk", sizeof(path));
  	cleanup_path(path);
