Re: Adding NetBSD and OpenBSD to Postgres CI

Hi,

Thanks for the review!

On Tue, 17 Dec 2024 at 19:21, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2024-11-12 11:38:11 +0300, Nazir Bilal Yavuz wrote:
> > On Fri, 1 Nov 2024 at 21:44, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > > + CCACHE_DIR: /tmp/ccache_dir
> > > > +
> > > > + PATH: /usr/sbin:$PATH
> > > > +
> > > > + # Postgres interprets LANG as a 'en_US.UTF-8' but it is 'C', then
> > >
> > > What does "Postgres interprets LANG as a 'en_US.UTF-8'" mean?
> >
> > It was because initdb was failing on NetBSD when the LANG and LC_ALL
> > is not set to C. I rephrased the comment and moved this under NetBSD
> > task.
>
> Do you happen to have a reference to the failure? The environment variables +
> the exact error message would be good. Kinda feels like that shouldn't
> happen with a default netbsd install.

This was already discussed upthread. Would you like more information?

> > > > + matrix:
> > > > + - name: NetBSD - 10 - Meson
> > > > + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*netbsd.*'
> > > > + env:
> > > > + IMAGE_FAMILY: pg-ci-netbsd-postgres
> > > > + INCLUDE_DIRS: -Dextra_lib_dirs=/usr/pkg/lib -Dextra_include_dirs=/usr/pkg/include
> > > > + <<: *netbsd_task_template
> > > > +
> > > > + - name: OpenBSD - 7 - Meson
> > > > + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*openbsd.*'
> > > > + env:
> > > > + IMAGE_FAMILY: pg-ci-openbsd-postgres
> > > > + INCLUDE_DIRS: -Dextra_include_dirs=/usr/local/include -Dextra_lib_dirs=/usr/local/lib
> > > > + UUID: -Duuid=e2fs
> > >
> > > Shouldn't something be added to PKG_CONFIG_PATH / --pkg-config-path?
> >
> > I don't think so. Both OSes are able to find pkgconfig at
> > '/usr/pkg/bin/pkg-config'. Am I missing something?
>
> --pkg-config-path is about the the path to pkg-config files, not the path to
> the pkg-config binary. If set we shouldn't need the
> extra_lib_dirs/extra_include_dirs, I think.

Yes, my bad. Done.

> > > Right now you don't seem to be collecting core files - but you're still
> > > enabling them via ulimit -c unlimited. At least we shouldn't use ulimit -c
> > > unlimited without collecting core files, but it'd probably be better to add
> > > support for collecting core files. Shouldn't be too hard.
> >
> > Done. I separated this patch to make review easier.
>
> +1
>
>
>
> > From cbea598b11e85b5c7090ca8e9cc05c35f0359f54 Mon Sep 17 00:00:00 2001
> > From: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
> > Date: Thu, 7 Nov 2024 15:48:31 +0300
> > Subject: [PATCH v2 1/3] Fix meson could not find bsd_auth.h
> >
> > bsd_auth.h file needs to be compiled together with the 'sys/types.h' as
> > it has missing type definitions.
> >
> > See synopsis at https://man.openbsd.org/authenticate.3
> > ---
> > meson.build | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 5b0510cef78..84107955d5d 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -551,7 +551,8 @@ test_c_args = cppflags + cflags
> > bsd_authopt = get_option('bsd_auth')
> > bsd_auth = not_found_dep
> > if cc.check_header('bsd_auth.h', required: bsd_authopt,
> > - args: test_c_args, include_directories: postgres_inc)
> > + args: test_c_args, prefix: '#include <sys/types.h>',
> > + include_directories: postgres_inc)
> > cdata.set('USE_BSD_AUTH', 1)
> > bsd_auth = declare_dependency()
> > endif
> > --
>
> Was about to apply that, but then started to wonder if we don't need the same
> for configure? And it sure looks like that has the same problem?
>
> Which also confuses me some, at some point this presumably worked?

This too was discussed upthread. Please let me know if you need more
information.

> > From cd5bb66a55e5226b543f5b7db9128cfa48e338e3 Mon Sep 17 00:00:00 2001
> > From: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
> > Date: Mon, 11 Nov 2024 13:23:22 +0300
> > Subject: [PATCH v2 2/3] Add NetBSD and OpenBSD tasks to the Postgres CI
> >
> > NetBSD and OpenBSD Postgres CI images are generated [1] but their tasks
> > are not added to the upstream Postgres yet. This patch adds them.
> >
> > Note: These tasks will be triggered manually to save CI credits but a
> > related line is commented out for now to trigger CFBot.
> >
> > Author: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
> >
> > [1] https://github.com/anarazel/pg-vm-images
> > ---
> > .cirrus.tasks.yml | 84 +++++++++++++++++++++++++++++++++++++++++++++++
> > .cirrus.yml | 10 ++++++
> > 2 files changed, 94 insertions(+)
> >
> > diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
> > index fc413eb11ef..f338af902aa 100644
> > --- a/.cirrus.tasks.yml
> > +++ b/.cirrus.tasks.yml
> > @@ -213,6 +213,90 @@ task:
> > cores_script: src/tools/ci/cores_backtrace.sh freebsd /tmp/cores
> >
> >
> > +task:
> > + depends_on: SanityCheck
> > + # trigger_type: manual
> > +
> > + env:
> > + # Below are experimentally derived to be a decent choice.
> > + CPUS: 2
>
> For cfbot it turns out to be easier to just use 4 cpus for everything. What
> time difference do you get from 2 vs 4 CPUs?

I shared the timings at the first email of the thread [1]. Copying the
timings from there:

╔══════════════════════╦════════╦═════════╗
║ CI Run Tim ║ ║ ║
║ (Only Test Step) ║ NetBSD ║ OpenBSD ║
║ (in minutes:seconds) ║ ║ ║
╠══════════════════════╬════════╬═════════╣
║ CPU: 2, TJ: 4 ║ 13:18 ║ 17:07 ║
╠══════════════════════╬════════╬═════════╣
║ CPU: 2, TJ: 6 ║ 11:01 ║ 16:23 ║
╠══════════════════════╬════════╬═════════╣
║ CPU: 2, TJ: 8 ║ 10:14 ║ 15:41 ║
╠══════════════════════╬════════╬═════════╣
║ CPU: 4, TJ: 4 ║ 11:46 ║ 16:03 ║
╠══════════════════════╬════════╬═════════╣
║ CPU: 4, TJ: 6 ║ 09:56 ║ 14:59 ║
╠══════════════════════╬════════╬═════════╣
║ CPU: 4, TJ: 8 ║ 10:02 ║ 15:09 ║
╚══════════════════════╩════════╩═════════╝

> > + BUILD_JOBS: 8
> > + TEST_JOBS: 8
>
> 8 build/test jobs for 2 cpus sounds unlikely to be close to optimal. A bit
> higher makes sense, but 4x?

I updated them with CPUS: 4 and JOBS: 8.

> > + CIRRUS_WORKING_DIR: /home/postgres/postgres
>
> I'd add a comment explaining why we're setting this.

Done.

> > + CCACHE_DIR: /tmp/ccache_dir
>
> And is it ok to put the ccache dir here, given the limited size of /tmp?

Yes, you are right; moved it to /home/postgres/cache.

> > + PATH: /usr/sbin:$PATH
> > +
> > + matrix:
> > + - name: NetBSD - 10 - Meson
>
> Given the image name doesn't include the version it seems we shouldn't include
> it here either...

Done.

> > + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*netbsd.*'
> > + env:
> > + IMAGE_FAMILY: pg-ci-netbsd-postgres
> > + INCLUDE_DIRS: -Dextra_include_dirs=/usr/pkg/include -Dextra_lib_dirs=/usr/pkg/lib
> > + # initdb fails with: 'invalid locale settings' error on NetBSD.
> > + # Force 'LANG' and 'LC_*' variables to be 'C'.
> > + LANG: "C"
> > + LC_ALL: "C"
> > + setup_additional_packages_script: |
> > + #pkgin -y install ...
> > + <<: *netbsd_task_template
> > +
> > + - name: OpenBSD - 7 - Meson
> > + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*openbsd.*'
> > + env:
> > + IMAGE_FAMILY: pg-ci-openbsd-postgres
> > + INCLUDE_DIRS: -Dextra_include_dirs=/usr/local/include -Dextra_lib_dirs=/usr/local/lib
> > + UUID: -Duuid=e2fs
>
> So for netbsd we're not using any uuid support? Ah, I see, it's documented
> further down. Maybe reference that, or move the comment around?

Done.

> > + # -Duuid=bsd is not set since 'bsd' uuid option
> > + # is not working on NetBSD & OpenBSD. See
> > + # https://www.postgresql.org/message-id/17358-89806e7420797025@postgresql.org
> > + # And other uuid options are not available on NetBSD.
> > + configure_script: |
> > + su postgres <<-EOF
> > + meson setup \
> > + --buildtype=debugoptimized \
> > + -Dcassert=true -Dinjection_points=true \
> > + -Dssl=openssl ${UUID} \
> > + -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
> > + ${INCLUDE_DIRS} \
> > + build
> > + EOF
>
> https://cirrus-ci.com/task/4879081273032704?logs=configure#L320
>
> [14:48:50.729] Run-time dependency krb5-gssapi found: NO (tried pkgconfig and cmake)
> [14:48:50.729] Library gssapi_krb5 found: NO
>
> https://cirrus-ci.com/task/6286456156585984?logs=configure#L109
>
> [14:49:28.049] Run-time dependency krb5-gssapi found: NO (tried pkgconfig and cmake)
> [14:49:28.049] Library gssapi_krb5 found: NO
>
> Is gss really not available / installed on either?

NetBSD is fixed [2] but we require MIT Kerberos [3] which requires a
gssapi_ext.h file. OpenBSD does not have MIT Kerberos nor gssapi_ext.h
file [4].

> https://cirrus-ci.com/task/4879081273032704?logs=configure#L49-L51
> [14:48:50.729] Run-time dependency tcl found: NO (tried pkgconfig and cmake)
> [14:48:50.729] Library tcl found: NO
> [14:48:50.729] Has header "tcl.h" with dependency -ltcl: NO
>
> Is TCL not available on openbsd?

Version option is added, it is working now.

> > + build_script: su postgres -c 'ninja -C build -j${BUILD_JOBS}'
> > + upload_caches: ccache
> > +
> > + test_world_script: |
> > + su postgres <<-EOF
> > + # Otherwise tests will fail on OpenBSD, due to the lack of enough processes.
> > + ulimit -p 256
>
> Should we also increase the number of semaphores?
>
> https://postgr.es/m/db2773a2-aca0-43d0-99c1-060efcd9954e%40gmail.com

It is already increased while creating the bsd images [5] [6].

> Perhaps it'd be better to update the system config earlier in the openbsd
> specific portion of the test?

'ulimit -p' needs to be run as a postgres user, but postgres user is
created after the OpenBSD specific portion. I can put an if statement
to make it only work for OpenBSD. Does that sound good?

> > build/tmp_install/usr/local/pgsql/bin/pg_ctl -D build/runningcheck stop || true
> > EOF
> > <<: *on_failure_meson
> > - cores_script: src/tools/ci/cores_backtrace.sh freebsd /tmp/cores
> > + cores_script: src/tools/ci/cores_backtrace.sh bsd /tmp/cores
>
> Hm, what's the deal with this change?

Removed, forgot to revert it before sending the patch.

> > on_failure:
> > <<: *on_failure_meson
> > + cores_script: |
> > + # Although OSes are forced to core dump inside ${CORE_DUMP_DIR}, they may
> > + # not obey this. So, move core files to the ${CORE_DUMP_DIR} directory.
> > + find build/ -maxdepth 1 -type f -name '*.core' -exec mv '{}' ${CORE_DUMP_DIR} \;
> > + src/tools/ci/cores_backtrace.sh ${OS_NAME} ${CORE_DUMP_DIR}
>
> s/OSs are forced/we try to configure the OS/
>
> Is -maxdepth 1 really sufficient? The tests run in subdirectories, don't they?

You are right, maxdepth is removed now.

> > + matrix:
> > + - name: NetBSD - 10 - Meson
> > + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*netbsd.*'
> ...
> > + - name: OpenBSD - 7 - Meson
> > + only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*openbsd.*'
>
> Think these probably should be added to src/tools/ci/README

Done.

[1] postgr.es/m/CAN55FZ0GXrojT2yUTrST5McJk8UWmYxUX8b696XjL01B1pKsxg%40mail.gmail.com
[2] https://github.com/anarazel/pg-vm-images/pull/108
[3] f7431bca8b
[4] postgr.es/m/3598083.1680976022%40sss.pgh.pa.us
[5] https://github.com/anarazel/pg-vm-images/blob/af8757bd5ed3f4055809bffde28334a8547dfced/scripts/bsd/netbsd-prep-postgres.sh#L30C1-L32C56
[6] https://github.com/anarazel/pg-vm-images/blob/af8757bd5ed3f4055809bffde28334a8547dfced/scripts/bsd/openbsd-prep-postgres.sh#L50C1-L53C63

--
Regards,
Nazir Bilal Yavuz
Microsoft