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

Test on PostgreSQL 16 #2958

Merged
merged 1 commit into from
Sep 21, 2023
Merged

Conversation

laurenceisla
Copy link
Member

@laurenceisla laurenceisla commented Sep 19, 2023

Since it's already added to nixpkgs, we can test on PostgreSQL 16 now: #2865 (comment)

Locally, tests run without issues, added options to test on CI too.

Closes #2865

@laurenceisla laurenceisla marked this pull request as ready for review September 21, 2023 18:21
@steve-chavez steve-chavez merged commit 90e3a5e into PostgREST:main Sep 21, 2023
@steve-chavez
Copy link
Member

steve-chavez commented Sep 25, 2023

The MacOS stuff is now consistently failing with this change:

       > installing
       > install: cannot stat 'safeupdate.so': No such file or directory

Looks like safeupdate is not compatible with pg 16. Will try to fix this.

@laurenceisla An option to make CI green again would be to remove safeupdate from here:

{ name = "postgresql-16"; postgresql = pkgs.postgresql_16.withPackages (p: [ p.postgis p.pg_safeupdate ]); }

And make the safeupdate spec tests run conditionally.

We would need to conditionally do this for MacOS with lib.platforms.darwin

@steve-chavez
Copy link
Member

It's weird that the above only happens on MacOS though ^

@steve-chavez
Copy link
Member

Unfortunately I don't have MacOS to test this.

cc @robx

@steve-chavez
Copy link
Member

Maybe we should revert this for now as it will also give us trouble when doing a new release. It's not that critical as we already tested for pg 16.

@dbaynard
Copy link
Contributor

Shouldn't it be referring to safeupdate.dylib on darwin, as shown by the line before installing in the logs?

cc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -g -O2  -fvisibility=hidden safeupdate.o -L/nix/store/1qipb24pnm55f9l2k6l7d89k0jl8sp7k-postgresql-16.0-lib/lib  -L/nix/store/i6skdxjya738cq5hcmx45cd5kdyq3xii-libxml2-2.11.5/lib -L/nix/store/xi3i64k4l3p7pxazixsmhf4y05s9g5pz-lz4-1.9.4/lib -L/nix/store/aklwi0pvynly8mv7n9ckjvv1lhz87a1y-zstd-1.5.5/lib  -Wl,-dead_strip_dylibs  -fvisibility=hidden -bundle -bundle_loader /nix/store/nm0qqgi36p8jb8v9gm4wdg83sx23v8lg-postgresql-16.0/bin/postgres -o safeupdate.dylib

The source tries to install safeupdate.so, unconditionally: nixpkgs/pkgs/servers/sql/postgresql/ext/pg_safeupdate.nix.

install -D safeupdate.so -t $out/lib

In any case, I tried removing the suggested line (on current nixpkgs) and the test tools do build on my M1 (I'm using the most recent nixos-unstable, though).

@dbaynard dbaynard mentioned this pull request Sep 27, 2023
@steve-chavez
Copy link
Member

steve-chavez commented Sep 27, 2023

Thanks for jumping in to help @dbaynard.

Shouldn't it be referring to safeupdate.dylib on darwin, as shown by the line before installing in the logs?
The source tries to install safeupdate.so, unconditionally: nixpkgs/pkgs/servers/sql/postgresql/ext/pg_safeupdate.nix.
install -D safeupdate.so -t $out/lib

That is surprising. So no other extensions from nixpkgs work on MacOS too? For example pg_cron also tries to copy an .so file:

https://github.com/NixOS/nixpkgs/blob/6500b4580c2a1f3d0f980d32d285739d8e156d92/pkgs/servers/sql/postgresql/ext/pg_cron.nix#L19


Also, why does it fail only on pg16?

@dbaynard
Copy link
Contributor

dbaynard commented Sep 27, 2023

When I force a rebuild of postgresql15Packaes.pg_safeupdate on darwin (i.e. force as it builds), I see the following.

pg-safeupdate> cc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -g -O2  safeupdate.o -L/nix/store/h4163dz3vjpq0qkn5b7hj7f0jz29pcr0-postgresql-15.4-lib/lib  -L/nix/store/5zghyzxf6zvnavmglcjhrab140gfmwig-libxml2-2.11.5/lib -L/nix/store/xal70bxk0rx3c8fsazlixhsc7nady7jn-lz4-1.9.4/lib -L/nix/store/ay59b8c1s2p2wr292ssa353ww0smczc3-zstd-1.5.5/lib  -Wl,-dead_strip_dylibs   -bundle -bundle_loader /nix/store/n4wsbhyakr6mnjafdb05fzb8zavc1zs7-postgresql-15.4/bin/postgres -o safeupdate.so

Note the safeupdate.so at the end.

For postgresql16Packages.pg_safeupdate I see the .dylib output [edit: as in, the cc command has -o safeupdate.dylib].

pg_cron builds from postgresql15Packages but not from postgresql16Packages. Different error.

       > installing
       > cp: missing destination file operand after '/nix/store/jlf7srd7cjw31ccdfcrxghf75m8p1abb-pg_cron-1.6.0/lib'
       > Try 'cp --help' for more information

@dbaynard
Copy link
Contributor

dbaynard commented Sep 27, 2023

It is far from obvious what is going on, from the source — I checked the patches, too.

This is the diff of the output of nix derivation show — again, nothing jumps out other than the upgrade of postgres.

diff --git 1/tmp/zshlvZij2 2/tmp/zsho7fLsn
index a2594dd..9b7843e 100644
--- 1/tmp/zshlvZij2
+++ 2/tmp/zsho7fLsn
@@ -1,5 +1,5 @@
 {
-  "/nix/store/0k7dmnnxwmmmzci4rxf9qngp88gvqs7b-pg-safeupdate-1.4.drv": {
+  "/nix/store/k14pjzw0svhln8b715q92vcdxpi5pw2k-pg-safeupdate-1.4.drv": {
     "args": [
       "-e",
       "/nix/store/6xg259477c90a229xwmb53pdfkn6ig3g-default-builder.sh"
@@ -12,7 +12,7 @@
       "__propagatedSandboxProfile": "",
       "__sandboxProfile": "",
       "__structuredAttrs": "",
-      "buildInputs": "/nix/store/n4wsbhyakr6mnjafdb05fzb8zavc1zs7-postgresql-15.4",
+      "buildInputs": "/nix/store/aag1jq1gyll5sn9agi29jlw1pm607szv-postgresql-16.0",
       "builder": "/nix/store/hm8sxwvz7qbf5kzgprdlggj231l5g2s4-bash-5.2-p15/bin/bash",
       "cmakeFlags": "",
       "configureFlags": "",
@@ -30,7 +30,7 @@
       "mesonFlags": "",
       "name": "pg-safeupdate-1.4",
       "nativeBuildInputs": "",
-      "out": "/nix/store/h335i7701rbkb0zj20fwiknf94apln9q-pg-safeupdate-1.4",
+      "out": "/nix/store/kq7gxbd9shxdr7hy7ia3bx0kjy9g72yj-pg-safeupdate-1.4",
       "outputs": "out",
       "patches": "",
       "pname": "pg-safeupdate",
@@ -49,10 +49,10 @@
       "/nix/store/1dlkpkdl57k8g0bi36fqb806fnj3qfp9-bash-5.2-p15.drv": [
         "out"
       ],
-      "/nix/store/207r2kxpxpkjxwpns0swlja6y7bxvich-postgresql-15.4.drv": [
+      "/nix/store/8z7lnakg257p7anxswwy0cvj6rx2zy5r-stdenv-darwin.drv": [
         "out"
       ],
-      "/nix/store/8z7lnakg257p7anxswwy0cvj6rx2zy5r-stdenv-darwin.drv": [
+      "/nix/store/hgn74l3iws2xfzaljiqfy73bfrbnm4q7-postgresql-16.0.drv": [
         "out"
       ]
     },
@@ -62,7 +62,7 @@
     "name": "pg-safeupdate-1.4",
     "outputs": {
       "out": {
-        "path": "/nix/store/h335i7701rbkb0zj20fwiknf94apln9q-pg-safeupdate-1.4"
+        "path": "/nix/store/kq7gxbd9shxdr7hy7ia3bx0kjy9g72yj-pg-safeupdate-1.4"
       }
     },
     "system": "aarch64-darwin"

@steve-chavez
Copy link
Member

Hm, it could be a breaking change in pg 16 itself but not sure if pg-safeupdate is at fault here. Because postgis seems to work? @dbaynard Can you confirm if postgis works on MacOS?

@dbaynard
Copy link
Contributor

dbaynard commented Sep 27, 2023

Yes, postgis works — search https://gist.github.com/dbaynard/4af11b18b78573fe71eaf3921160548b for postgis

[edit: I'll hold fire on raising an issue in nixpkgs, then, which is why I created the gist — also, I'm going AFK.]

@steve-chavez
Copy link
Member

steve-chavez commented Sep 28, 2023

@dbaynard Not sure if I'm reading this right, but most extensions are failing to build for pg16?

For example plr: plr> install: cannot stat 'plr.so': No such file or directory .

PostGIS succeeding is weird, maybe they don't produce shared libraries.

Edit: we have a possible fix on #2973 (comment)

@dbaynard
Copy link
Contributor

@dbaynard Not sure if I'm reading this right, but most extensions are failing to build for pg16?

Correct. I was in a bit of a rush, so please forgive the hackiness of that gist.

Edit: we have a possible fix on #2973 (comment)

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Test on PostgreSQL 16 beta 2
3 participants