Subject: Re: CI and test improvements

From: Justin Pryzby <[email-masked]>
Date: Mon, 13 Mar 2023 23:56:56 -0500

On Mon, Mar 13, 2023 at 07:39:52AM +0100, Peter Eisentraut wrote:
> On 03.02.23 15:26, Justin Pryzby wrote:
> > rebased, and re-including a patch to show code coverage of changed
> > files.
> 
> This constant flow of patches under one subject doesn't lend itself well to
> the commit fest model of trying to finish things up.
> I can't quite tell which of these patches are ready and agreed upon,
> and which ones are work in progress or experimental.

I'm soliticing feedback on those patches that I've sent recently - I've
elided patches if they have some unresolved issue.

I'm not aware of any loose ends other than what's updated here:

- cirrus: code coverage

I changed this to also run an "initial" coverage report before running
tests.  It's not clear to me what effect that has, though...

Andres seems to think it's a problem that this shows coverage only for
files that were actually changed.  But that's what's intended; it's
sufficient to see if new code is being hit by tests.  It would be slow
and take a lot of extra space to upload a coverage report for every
patch, every day.  It might be nice for cfbot to show how test coverage
changed in the affected files: -15% / +25%.

- cirrus: upload changed html docs as artifacts

Fixed an "only_if" line so cfbot will run the "warnings" task.

Maybe this path is waiting on Andres' patch to "move CompilerWarnings to
meson" ?

> > 7e09035f588 WIP: ci/meson: allow showing only failed tests ..
> 
> I'm not sure I like this one.  I sometimes look up the logs of non-failed
> tests to compare them with failed tests, to get context to could lead to
> failures.  Maybe we can make this behavior adjustable. But I've not been
> bothered by the current behavior.

It's adjustable by un/setting the environment variable.

I'm surprised to hear that anyone using cirrusci (with or without cfbot)
wouldn't prefer the behavior this patch implements.  It's annoying to
search find the logs for the (typically exactly one) failing test in
cirrus' directory of 200some test artifacts.  We're also uploading a lot
of logs for every failure.  (But I suppose this might break cfbot's new
client side parsing of things like build logs...)

-- 
Justin
>From f6174c446b0ee4f69239524ecc1506edaf41b33b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Wed, 25 May 2022 21:53:22 -0500
Subject: [PATCH 1/8] cirrus/windows: add compiler_warnings_script

I'm not sure how to write this test in windows shell; it's also easy to
write something that doesn't work in posix sh, since windows shell is
interpretting && and ||...

https://www.postgresql.org/message-id/20220212212310.f645c6vw3njkgxka%40alap3.anarazel.de

See also:
8a1ce5e54f6d144e4f8e19af7c767b026ee0c956
https://cirrus-ci.com/task/6241060062494720
https://cirrus-ci.com/task/6496366607204352

ci-os-only: windows
---
 .cirrus.yml | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 505c50f3285..60c0efc2e63 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -563,28 +563,37 @@ task:
 
   setup_additional_packages_script: |
     REM choco install -y --no-progress ...
 
   # Use /DEBUG:FASTLINK to avoid high memory usage during linking
   configure_script: |
     vcvarsall x64
     meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" build
 
   build_script: |
     vcvarsall x64
-    ninja -C build
+    ninja -C build |tee build.txt
+    REM Since pipes lose the exit status of the preceding command, rerun the compilation
+    REM without the pipe, exiting now if it fails, to avoid trying to run checks
+    ninja -C build > nul
 
   check_world_script: |
     vcvarsall x64
     meson test %MTEST_ARGS% --num-processes %TEST_JOBS%
 
+  # This should be last, so check_world is run even if there are warnings
+  always:
+    compiler_warnings_script:
+      # this avoids using metachars which would be interpretted by the windows shell
+      - sh -c 'if grep ": warning " build.txt; then exit 1; fi; exit 0'
+
   on_failure:
     <<: *on_failure_meson
     crashlog_artifacts:
       path: "crashlog-*.txt"
       type: text/plain

 task:
   << : *WINDOWS_ENVIRONMENT_BASE
   name: Windows - Server 2019, MinGW64 - Meson
 
-- 
2.34.1

>From 60378805b5b2d634f426850b6ce4d1c64b8aabf4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Fri, 24 Jun 2022 00:09:12 -0500
Subject: [PATCH 2/8] cirrus/freebsd: run with more CPUs+RAM and do not
 repartition

There was some historic problem where tests under freebsd took 8+ minutes (and
before 4a288a37f took 15 minutes).

This reduces test time from 10min to 3min.
4 CPUs 4 tests https://cirrus-ci.com/task/4880240739614720
4 CPUs 6 tests https://cirrus-ci.com/task/4664440120410112 https://cirrus-ci.com/task/4586784884523008
4 CPUs 8 tests https://cirrus-ci.com/task/5001995491737600

6 CPUs https://cirrus-ci.com/task/6678321684545536
8 CPUs https://cirrus-ci.com/task/6264854121021440

See also:
https://www.postgresql.org/message-id/flat/[email-masked]#f36c0b17e33e31e7925e7e5812998686
8 jobs 7min https://cirrus-ci.com/task/6186376667332608

//-os-only: freebsd
---
 .cirrus.yml | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 60c0efc2e63..99276481e57 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -123,27 +123,25 @@ task:
     <<: *on_failure_meson
     cores_script: |
       mkdir -m 770 /tmp/cores
       find / -maxdepth 1 -type f -name 'core*' -exec mv '{}' /tmp/cores/ \;
       src/tools/ci/cores_backtrace.sh linux /tmp/cores

 task:
   name: FreeBSD - 13 - Meson
 
   env:
-    # FreeBSD on GCP is slow when running with larger number of CPUS /
-    # jobs. Using one more job than cpus seems to work best.
-    CPUS: 2
-    BUILD_JOBS: 3
-    TEST_JOBS: 3
+    CPUS: 4
+    BUILD_JOBS: 4
+    TEST_JOBS: 6
 
     CCACHE_DIR: /tmp/ccache_dir
     CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
     CFLAGS: -Og -ggdb
 
   depends_on: SanityCheck
   only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*freebsd.*'
 
   compute_engine_instance:
     image_project: $IMAGE_PROJECT
     image: family/pg-ci-freebsd-13
@@ -152,39 +150,36 @@ task:
     memory: 4G
     disk: 50
 
   sysinfo_script: |
     id
     uname -a
     ulimit -a -H && ulimit -a -S
     export
 
   ccache_cache:
     folder: $CCACHE_DIR
-  # Work around performance issues due to 32KB block size
-  repartition_script: src/tools/ci/gcp_freebsd_repartition.sh
   create_user_script: |
     pw useradd postgres
     chown -R postgres:postgres .
     mkdir -p ${CCACHE_DIR}
     chown -R postgres:postgres ${CCACHE_DIR}
   setup_core_files_script: |
     mkdir -m 770 /tmp/cores
     chown root:postgres /tmp/cores
     sysctl kern.corefile='/tmp/cores/%N.%P.core'
   setup_additional_packages_script: |
     #pkg install -y ...
 
   # NB: Intentionally build without -Dllvm. The freebsd image size is already
-  # large enough to make VM startup slow, and even without llvm freebsd
-  # already takes longer than other platforms except for windows.
+  # large enough to make VM startup slow
   configure_script: |
     su postgres <<-EOF
       meson setup \
         --buildtype=debug \
         -Dcassert=true -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
         -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
         -Dextra_lib_dirs=/usr/local/lib -Dextra_include_dirs=/usr/local/include/ \
         build
     EOF
   build_script: su postgres -c 'ninja -C build -j${BUILD_JOBS}'
   upload_caches: ccache
-- 
2.34.1

>From 5b376acde8f6e7eff2f4a846620d2bb68091c681 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Fri, 9 Dec 2022 15:32:50 -0600
Subject: [PATCH 3/8] cirrus/freebsd: define
 ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

See also: 54100f5c6052404f68de9ce7310ceb61f1c291f8

ci-os-only: freebsd
---
 .cirrus.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 99276481e57..f8238498613 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -128,23 +128,23 @@ task:

 task:
   name: FreeBSD - 13 - Meson
 
   env:
     CPUS: 4
     BUILD_JOBS: 4
     TEST_JOBS: 6
 
     CCACHE_DIR: /tmp/ccache_dir
-    CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
+    CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
     CFLAGS: -Og -ggdb
 
   depends_on: SanityCheck
   only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*freebsd.*'
 
   compute_engine_instance:
     image_project: $IMAGE_PROJECT
     image: family/pg-ci-freebsd-13
     platform: freebsd
     cpu: $CPUS
     memory: 4G
-- 
2.34.1

>From 473991f8246a2e710dc0c3b614b769cfa1efc31d Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Sat, 30 Jul 2022 19:25:51 -0500
Subject: [PATCH 4/8] pg_upgrade: tap test: exercise --link and --clone

This both increases code coverage and accelerates tests.

See also: b059a2409faf5833b3ba7792e247d6466c9e8090

linux,
macos,
//-os-only: freebsd
---
 .cirrus.yml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.cirrus.yml b/.cirrus.yml
index f8238498613..4910a0fceba 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -130,22 +130,23 @@ task:
 task:
   name: FreeBSD - 13 - Meson
 
   env:
     CPUS: 4
     BUILD_JOBS: 4
     TEST_JOBS: 6
 
     CCACHE_DIR: /tmp/ccache_dir
     CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
     CFLAGS: -Og -ggdb
+    PG_TEST_PG_UPGRADE_MODE: --link
 
   depends_on: SanityCheck
   only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*freebsd.*'
 
   compute_engine_instance:
     image_project: $IMAGE_PROJECT
     image: family/pg-ci-freebsd-13
     platform: freebsd
     cpu: $CPUS
     memory: 4G
     disk: 50
@@ -266,22 +267,23 @@ task:
     # print_stacktraces=1,verbosity=2, duh
     # detect_leaks=0: too many uninteresting leak errors in short-lived binaries
     UBSAN_OPTIONS: print_stacktrace=1:disable_coredump=0:abort_on_error=1:verbosity=2
     ASAN_OPTIONS: print_stacktrace=1:disable_coredump=0:abort_on_error=1:detect_leaks=0
 
     # SANITIZER_FLAGS is set in the tasks below
     CFLAGS: -Og -ggdb -fno-sanitize-recover=all $SANITIZER_FLAGS
     CXXFLAGS: $CFLAGS
     LDFLAGS: $SANITIZER_FLAGS
     CC: ccache gcc
     CXX: ccache g++
+    PG_TEST_PG_UPGRADE_MODE: --link
 
     LINUX_CONFIGURE_FEATURES: *LINUX_CONFIGURE_FEATURES
     LINUX_MESON_FEATURES: *LINUX_MESON_FEATURES
 
   depends_on: SanityCheck
   only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*linux.*'
 
   compute_engine_instance:
     image_project: $IMAGE_PROJECT
     image: family/pg-ci-bullseye
     platform: linux
@@ -416,22 +418,23 @@ task:
 
     CIRRUS_WORKING_DIR: ${HOME}/pgsql/
     CCACHE_DIR: ${HOME}/ccache
     HOMEBREW_CACHE: ${HOME}/homebrew-cache
     PERL5LIB: ${HOME}/perl5/lib/perl5
 
     CC: ccache cc
     CXX: ccache c++
     CPPFLAGS: -DRANDOMIZE_ALLOCATED_MEMORY
     CFLAGS: -Og -ggdb
     CXXFLAGS: -Og -ggdb
+    PG_TEST_PG_UPGRADE_MODE: --clone
 
   depends_on: SanityCheck
   only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*(macos|darwin|osx).*'
 
   macos_instance:
     image: ghcr.io/cirruslabs/macos-ventura-base:latest
 
   sysinfo_script: |
     id
     uname -a
     ulimit -a -H && ulimit -a -S
-- 
2.34.1

>From 187302a1932b753fc175fbbfd2d17cd288cf1e5f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Fri, 25 Nov 2022 13:57:17 -0600
Subject: [PATCH 5/8] WIP: ci/meson: allow showing only failed tests ..

It's simpler and seems to make more sense to integrate this with
testwrap, rather than to run it after check-world, but only if it
failed, and finding a way to preserve the exit code.

https://www.postgresql.org/message-id/[email-masked]

> It is wasteful to upload thousdands of logfiles to show a single
> failure.  That would make our cirrus tasks faster - compressing and
> uploading the logs takes over a minute.
>
> It's also a lot friendlier to show fewer than 8 pages of test folders to
> search through to find the one that failed.

macos
ci-os-only: linux-meson freebsd
---
 .cirrus.yml        | 13 +++++++------
 src/tools/testwrap |  6 ++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 4910a0fceba..6cbbea2f1e1 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -15,42 +15,43 @@ env:
   CIRRUS_CLONE_DEPTH: 500
   # Useful to be able to analyse what in a script takes long
   CIRRUS_LOG_TIMESTAMP: true
 
   CCACHE_MAXSIZE: "250M"
 
   # target to test, for all but windows
   CHECK: check-world PROVE_FLAGS=$PROVE_FLAGS
   CHECKFLAGS: -Otarget
   PROVE_FLAGS: --timer
   MTEST_ARGS: --print-errorlogs --no-rebuild -C build
+  PG_FAILED_TESTDIR: ${CIRRUS_WORKING_DIR}/failed.build
   PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl

 # What files to preserve in case tests fail
 on_failure_ac: &on_failure_ac
   log_artifacts:
     paths:
       - "**/*.log"
       - "**/*.diffs"
       - "**/regress_log_*"
     type: text/plain
 
 on_failure_meson: &on_failure_meson
   testrun_artifacts:
     paths:
-      - "build*/testrun/**/*.log"
-      - "build*/testrun/**/*.diffs"
-      - "build*/testrun/**/regress_log_*"
+      - "failed.build*/**/*.log"
+      - "failed.build*/**/*.diffs"
+      - "failed.build*/**/regress_log_*"
     type: text/plain
 
   # In theory it'd be nice to upload the junit files meson generates, so that
   # cirrus will nicely annotate the commit. Unfortunately the files don't
   # contain identifiable file + line numbers right now, so the annotations
   # don't end up useful. We could probably improve on that with a some custom
   # conversion script, but ...
   meson_log_artifacts:
     path: "build*/meson-logs/*.txt"
     type: text/plain
 
@@ -190,26 +191,26 @@ task:
       ulimit -c unlimited
       meson test $MTEST_ARGS --num-processes ${TEST_JOBS}
     EOF
 
   # test runningcheck, freebsd chosen because it's currently fast enough
   test_running_script: |
     su postgres <<-EOF
       set -e
       ulimit -c unlimited
       meson test $MTEST_ARGS --quiet --suite setup
       export LD_LIBRARY_PATH="$(pwd)/build/tmp_install/usr/local/pgsql/lib/:$LD_LIBRARY_PATH"
-      mkdir -p build/testrun
+      mkdir -p build/testrun ${PG_FAILED_TESTDIR}
       build/tmp_install/usr/local/pgsql/bin/initdb -N build/runningcheck --no-instructions -A trust
       echo "include '$(pwd)/src/tools/ci/pg_ci_base.conf'" >> build/runningcheck/postgresql.conf
-      build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off' -D build/runningcheck -l build/testrun/runningcheck.log start
+      build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off' -D build/runningcheck -l ${PG_FAILED_TESTDIR}/runningcheck.log start
       meson test $MTEST_ARGS --num-processes ${TEST_JOBS} --setup running
       build/tmp_install/usr/local/pgsql/bin/pg_ctl -D build/runningcheck stop
     EOF
 
   on_failure:
     # if the server continues running, it often causes cirrus-ci to fail
     # during upload, as it doesn't expect artifacts to change size
     stop_running_script: |
       su postgres <<-EOF
         build/tmp_install/usr/local/pgsql/bin/pg_ctl -D build/runningcheck stop || true
       EOF
@@ -387,23 +388,23 @@ task:
         EOF
         # so that we don't upload 64bit logs if 32bit fails
         rm -rf build/
 
       # There's currently no coverage of icu with LANG=C in the buildfarm. We
       # can easily provide some here by running one of the sets of tests that
       # way. Newer versions of python insist on changing the LC_CTYPE away
       # from C, prevent that with PYTHONCOERCECLOCALE.
       test_world_32_script: |
         su postgres <<-EOF
           ulimit -c unlimited
-          PYTHONCOERCECLOCALE=0 LANG=C meson test $MTEST_ARGS -C build-32 --num-processes ${TEST_JOBS}
+          PYTHONCOERCECLOCALE=0 LANG=C PG_FAILED_TESTDIR=`pwd`/failed.build-32 meson test $MTEST_ARGS -C build-32 --num-processes ${TEST_JOBS}
         EOF
 
       on_failure:
         <<: *on_failure_meson
 
   on_failure:
     cores_script: src/tools/ci/cores_backtrace.sh linux /tmp/cores

 task:
   name: macOS - Ventura - Meson
diff --git a/src/tools/testwrap b/src/tools/testwrap
index 7a64fe76a2d..445ac595afc 100755
--- a/src/tools/testwrap
+++ b/src/tools/testwrap
@@ -36,12 +36,18 @@ env_dict = {**os.environ,
             'TESTDATADIR': os.path.join(testdir, 'data'),
             'TESTLOGDIR': os.path.join(testdir, 'log')}
 
 sp = subprocess.run(args.test_command, env=env_dict)
 
 if sp.returncode == 0:
     print('# test succeeded')
     open(os.path.join(testdir, 'test.success'), 'x')
 else:
     print('# test failed')
     open(os.path.join(testdir, 'test.fail'), 'x')
+    faileddir = os.getenv('PG_FAILED_TESTDIR')
+    if faileddir:
+        parentdir = os.path.dirname(testdir)
+        newdest = os.path.join(faileddir, os.path.basename(parentdir), os.path.basename(testdir))
+        shutil.copytree(testdir, newdest)
+
 sys.exit(sp.returncode)
-- 
2.34.1

>From fe982a4652518c6b346009873284ed0fbd60a205 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 17 Jan 2022 00:54:28 -0600
Subject: [PATCH 6/8] cirrus: code coverage

Coverage is shown only for changed files.  This is useful to see
coverage of newly-added code, but won't show added/lost coverage in
files which this patch doesn't modify.

Some alternatives:
- could build with "--coverage -fprofile-filter-files=", but that means
  that ccache will never work (both because it doesn't support that option
  and also because the arguments will be different for every patch).
- could use ninja coverage-html, but that can't filter only changed
  files, and would take a long time and a lot of space to upload a lot
  of useless files.

https://www.postgresql.org/message-id/202202111821.w3gqblvfp4pr%40alvherre.pgsql
https://www.postgresql.org/message-id/flat/[email-masked]

ci-os-only: freebsd
---
 .cirrus.yml                       | 20 ++++++++++++-
 src/tools/ci/code-coverage-report | 48 +++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletion(-)
 create mode 100755 src/tools/ci/code-coverage-report

diff --git a/.cirrus.yml b/.cirrus.yml
index 6cbbea2f1e1..cea8b6fef2b 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -20,22 +20,30 @@ env:
 
   # target to test, for all but windows
   CHECK: check-world PROVE_FLAGS=$PROVE_FLAGS
   CHECKFLAGS: -Otarget
   PROVE_FLAGS: --timer
   MTEST_ARGS: --print-errorlogs --no-rebuild -C build
   PG_FAILED_TESTDIR: ${CIRRUS_WORKING_DIR}/failed.build
   PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
   TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
   PG_TEST_EXTRA: kerberos ldap ssl
 
+  # The commit that this branch is rebased on.  There's no easy way to find this.
+  # This does the right thing for cfbot, which always squishes all patches into a single commit.
+  # And does the right thing for any 1-patch commits.
+  # Patch series manually submitted to cirrus would benefit from setting this to the
+  # number of patches in the series (or directly to the commit the series was rebased on).
+  #BASE_COMMIT: HEAD~1
+  # For demo purposes:
+  BASE_COMMIT: HEAD~11
 
 # What files to preserve in case tests fail
 on_failure_ac: &on_failure_ac
   log_artifacts:
     paths:
       - "**/*.log"
       - "**/*.diffs"
       - "**/regress_log_*"
     type: text/plain
 
 on_failure_meson: &on_failure_meson
@@ -149,57 +157,67 @@ task:
     image: family/pg-ci-freebsd-13
     platform: freebsd
     cpu: $CPUS
     memory: 4G
     disk: 50
 
   sysinfo_script: |
     id
     uname -a
     ulimit -a -H && ulimit -a -S
     export
+    git diff --name-only "$BASE_COMMIT"
 
   ccache_cache:
     folder: $CCACHE_DIR
   create_user_script: |
     pw useradd postgres
     chown -R postgres:postgres .
     mkdir -p ${CCACHE_DIR}
     chown -R postgres:postgres ${CCACHE_DIR}
   setup_core_files_script: |
     mkdir -m 770 /tmp/cores
     chown root:postgres /tmp/cores
     sysctl kern.corefile='/tmp/cores/%N.%P.core'
   setup_additional_packages_script: |
-    #pkg install -y ...
+    pkg install -y lcov
 
   # NB: Intentionally build without -Dllvm. The freebsd image size is already
   # large enough to make VM startup slow
   configure_script: |
     su postgres <<-EOF
       meson setup \
         --buildtype=debug \
+        -Db_coverage=true \
         -Dcassert=true -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
         -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
         -Dextra_lib_dirs=/usr/local/lib -Dextra_include_dirs=/usr/local/include/ \
         build
     EOF
   build_script: su postgres -c 'ninja -C build -j${BUILD_JOBS}'
   upload_caches: ccache
 
   test_world_script: |
     su postgres <<-EOF
+      set -e
       ulimit -c unlimited
+      # Write initial coverage files before running tests:
+      time ./src/tools/ci/code-coverage-report "$BASE_COMMIT" ./build ./coverage "--initial"
       meson test $MTEST_ARGS --num-processes ${TEST_JOBS}
+      # Create coverage report for files changed since the base commit.
+      time ./src/tools/ci/code-coverage-report "$BASE_COMMIT" ./build ./coverage
     EOF
 
+  coverage_artifacts:
+    path: 'coverage/**'
+
   # test runningcheck, freebsd chosen because it's currently fast enough
   test_running_script: |
     su postgres <<-EOF
       set -e
       ulimit -c unlimited
       meson test $MTEST_ARGS --quiet --suite setup
       export LD_LIBRARY_PATH="$(pwd)/build/tmp_install/usr/local/pgsql/lib/:$LD_LIBRARY_PATH"
       mkdir -p build/testrun ${PG_FAILED_TESTDIR}
       build/tmp_install/usr/local/pgsql/bin/initdb -N build/runningcheck --no-instructions -A trust
       echo "include '$(pwd)/src/tools/ci/pg_ci_base.conf'" >> build/runningcheck/postgresql.conf
       build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off' -D build/runningcheck -l ${PG_FAILED_TESTDIR}/runningcheck.log start
diff --git a/src/tools/ci/code-coverage-report b/src/tools/ci/code-coverage-report
new file mode 100755
index 00000000000..db448e802ba
--- /dev/null
+++ b/src/tools/ci/code-coverage-report
@@ -0,0 +1,48 @@
+#! /bin/sh
+# Called during CI to generate a code coverage report of changed files.
+set -e
+
+base_branch=$1
+build_dir=$2
+outdir=$3
+args=$4
+
+changed=`git diff --name-only "$base_branch" '*.c'`
+[ -z "$changed" ] && exit 0 # Nothing changed
+
+[ -d "$outdir" ] ||
+	mkdir "$outdir"
+
+# This could be used to map from object file back to source file:
+# readelf --debug-dump=info src/backend/postgres_lib.a.p/utils_adt_array_userfuncs.c.o |awk '/DW_AT_name/{print $NF;exit}'
+# gcov ./src/port/libpgport_shlib.a.p/inet_net_ntop.c.gcno --stdout
+
+gcov=$outdir/coverage.gcov
+lcov --quiet --capture --directory "$build_dir" $args >"$gcov.new"
+
+# Filter to include only changed files
+echo "$changed" |sed 's,^,*/,' |
+	xargs -rt lcov --extract "$gcov.new" >"$gcov.filtered"
+rm "$gcov.new"
+
+echo "$args" |grep initial >/dev/null && {
+	mv "$gcov.filtered" "$gcov.init"
+	exit 0
+}
+
+# Exit successfully if no relevant files were changed
+[ -s "$gcov.filtered" ] || {
+	rm "$gcov.init"
+	exit 0
+}
+
+# Otherwise combine with the init file:
+lcov -a "$gcov.init" -a "$gcov.filtered" >"$gcov"
+
+genhtml "$gcov" --show-details --legend --quiet --num-spaces=4 --output-directory "$outdir" \
+	--title="Coverage report of files changed since: $base_branch"
+cp "$outdir"/index.html "$outdir"/00-index.html
+
+gzip "$gcov" "$gcov.init" "$gcov.filtered"
+ls -l "$outdir"
+du -sh "$outdir"
-- 
2.34.1

>From df877bcb5a4e9cc73d9d278b2ccd3e7b8ad0bdee Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Sat, 26 Feb 2022 19:39:10 -0600
Subject: [PATCH 7/8] cirrus: upload changed html docs as artifacts

This could be done on the client side (cfbot).  One advantage of doing
it here is that fewer docs are uploaded - many patches won't upload docs
at all.

https://www.postgresql.org/message-id/flat/[email-masked]
https://www.postgresql.org/message-id/CAB8KJ=[email-masked]

https://cirrus-ci.com/task/5396696388599808

ci-os-only: html
---
 .cirrus.yml                    | 16 +++++++++++++++-
 src/tools/ci/copy-changed-docs | 29 +++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100755 src/tools/ci/copy-changed-docs

diff --git a/.cirrus.yml b/.cirrus.yml
index cea8b6fef2b..a0929f7eb5a 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -700,22 +700,23 @@ task:
     image: $CONTAINER_REPO/linux_debian_bullseye_ci:latest
     cpu: $CPUS
 
   sysinfo_script: |
     id
     uname -a
     cat /proc/cmdline
     ulimit -a -H && ulimit -a -S
     gcc -v
     clang -v
     export
+    git diff --name-only "$BASE_COMMIT"
 
   ccache_cache:
     folder: $CCACHE_DIR
 
   setup_additional_packages_script: |
     #apt-get update
     #DEBIAN_FRONTEND=noninteractive apt-get -y install ...
 
   ###
   # Test that code can be built with gcc/clang without warnings
   ###
@@ -774,34 +775,47 @@ task:
   always:
     mingw_cross_warning_script: |
       time ./configure \
         --host=x86_64-w64-mingw32 \
         --enable-cassert \
         CC="ccache x86_64-w64-mingw32-gcc" \
         CXX="ccache x86_64-w64-mingw32-g++"
       make -s -j${BUILD_JOBS} clean
       time make -s -j${BUILD_JOBS} world-bin
 
   ###
-  # Verify docs can be built
+  # Verify docs can be built, and upload changed docs as artifacts
   ###
   # XXX: Only do this if there have been changes in doc/ since last build
   always:
     docs_build_script: |
       time ./configure \
         --cache gcc.cache \
         CC="ccache gcc" \
         CXX="ccache g++" \
         CLANG="ccache clang"
       make -s -j${BUILD_JOBS} clean
       time make -s -j${BUILD_JOBS} -C doc
+      cp -r doc new-docs
+
+      # Re-build HTML docs from the base commit.
+      git checkout "$BASE_COMMIT" -- doc
+      make -s -C doc clean
+      time make -s -C doc html
+      cp -r doc old-docs
+
+    copy_changed_docs_script:
+      - src/tools/ci/copy-changed-docs "old-docs" "new-docs" "html_docs"
+
+    html_docs_artifacts:
+      paths: ['html_docs/*.html', 'html_docs/*.png', 'html_docs/*.css']
 
   ###
   # Verify headerscheck / cpluspluscheck succeed
   #
   # - Don't use ccache, the files are uncacheable, polluting ccache's
   #   cache
   # - Use -fmax-errors, as particularly cpluspluscheck can be very verbose
   # - XXX have to disable ICU to avoid errors:
   #   https://postgr.es/m/20220323002024.f2g6tivduzrktgfa%40alap3.anarazel.de
   ###
   always:
diff --git a/src/tools/ci/copy-changed-docs b/src/tools/ci/copy-changed-docs
new file mode 100755
index 00000000000..1c921a8df6f
--- /dev/null
+++ b/src/tools/ci/copy-changed-docs
@@ -0,0 +1,29 @@
+#! /bin/sh
+# Copy HTML which differ between $old and $new into $outdir
+set -e
+
+old=$1
+new=$2
+outdir=$3
+
+# The index is large and changes often
+skippages="bookindex.html"
+
+mkdir "$outdir"
+cp "$new"/src/sgml/html/*.css "$new"/src/sgml/html/*.svg "$outdir"
+
+changed=`git diff --no-index --name-only "$old"/src/sgml/html "$new"/src/sgml/html` ||
+	[ $? -eq 1 ]
+
+for f in $changed
+do
+	# Avoid removed files
+	[ -f "$f" ] || continue
+
+	echo "$f" |grep -Ew "$skippages" >/dev/null &&
+		continue
+
+	cp -v "$f" "$outdir"
+done
+
+exit 0
-- 
2.34.1

>From 6ff86c991dabad62dbe8e69c53db37e081529e6a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 28 Feb 2022 23:18:19 -0600
Subject: [PATCH 8/8] +html index file

This allows linking to the artifacts from the last successful build.

//freebsd
ci-os-only: warnings
---
 src/tools/ci/copy-changed-docs | 42 ++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/src/tools/ci/copy-changed-docs b/src/tools/ci/copy-changed-docs
index 1c921a8df6f..57dd2823a50 100755
--- a/src/tools/ci/copy-changed-docs
+++ b/src/tools/ci/copy-changed-docs
@@ -1,29 +1,67 @@
 #! /bin/sh
 # Copy HTML which differ between $old and $new into $outdir
 set -e
 
 old=$1
 new=$2
 outdir=$3
+branch=$CIRRUS_BRANCH
+
+# The index is large and changes often
+skippages="bookindex.html"
 
 # The index is large and changes often
 skippages="bookindex.html"
 
 mkdir "$outdir"
 cp "$new"/src/sgml/html/*.css "$new"/src/sgml/html/*.svg "$outdir"
 
+# The index is useful to allow a static link (not specific to a cirrus run) to the artifacts for the most-recent, successful CI run for a branch
+branchurl=https://api.cirrus-ci.com/v1/artifact/github/$CIRRUS_REPO_FULL_NAME/$CIRRUS_TASK_NAME/html_docs/html_docs/00-index.html?branch=$branch
+
+index="$outdir/00-index.html"
+cat >"$index" <<EOF
+<html>
+<head><title>Index of docs changed since: $BASE_COMMIT</title></head>
+<body>
+<!-- A link to documentation for the most recent successful build: $branchurl -->
+<h1>Index of docs changed since: $BASE_COMMIT</h1>
+<ul>
+EOF
+
 changed=`git diff --no-index --name-only "$old"/src/sgml/html "$new"/src/sgml/html` ||
 	[ $? -eq 1 ]
 
 for f in $changed
 do
 	# Avoid removed files
 	[ -f "$f" ] || continue
 
 	echo "$f" |grep -Ew "$skippages" >/dev/null &&
 		continue
 
-	cp -v "$f" "$outdir"
-done
+	cp -v "$f" "$outdir" >&2
+	fn=${f##*/}
+	# ?branch=... is needed when accessing the artifacts for the static link for the branch
+	# It's not needed and ignored if accessing artifacts for *this* CI run
+	echo "<li><a href='$fn?branch=$branch'>$fn</a>"
+done >>"$index"
+
+github=https://github.com/$CIRRUS_REPO_FULL_NAME/commit
+cirrus=https://cirrus-ci.com/build
+
+cat >>"$index" <<EOF
+</ul>
+<hr>
+<code>
+<br>This file was written on: `date --rfc-822 --utc`
+<br>CIRRUS_CHANGE_TITLE: $CIRRUS_CHANGE_TITLE
+<br>CIRRUS_CHANGE_IN_REPO: <a href="$github/$CIRRUS_CHANGE_IN_REPO">$CIRRUS_CHANGE_IN_REPO</a>
+<br>CIRRUS_BUILD_ID: <a href="$cirrus/$CIRRUS_BUILD_ID">$CIRRUS_BUILD_ID</a>
+<br>CIRRUS_LAST_GREEN_CHANGE: <a href="$github/$CIRRUS_LAST_GREEN_CHANGE">$CIRRUS_LAST_GREEN_CHANGE</a>
+<br>CIRRUS_LAST_GREEN_BUILD_ID: <a href="$cirrus/$CIRRUS_LAST_GREEN_BUILD_ID">$CIRRUS_LAST_GREEN_BUILD_ID</a>
+</code>
+</body></html>
+EOF
 
 exit 0
-- 
2.34.1

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 15 Mar 2023 10:58:41 +0100

> [PATCH 1/8] cirrus/windows: add compiler_warnings_script

Needs a better description of what it actually does.  (And fewer "I'm 
not sure how to write this ..." comments ;-) )  It looks like it would 
fail the build if there is a compiler warning in the Windows VS task? 
Shouldn't that be done in the CompilerWarnings task?

Also, I see a bunch of warnings in the current output from that task. 
These should be cleaned up in any case before we can let a thing like 
this loose.

(The warnings are all like

C:\python\Include\pyconfig.h(117): warning C4005: 'MS_WIN64': macro 
redefinition

so possibly a single fix can address them all.)

 > [PATCH 2/8] cirrus/freebsd: run with more CPUs+RAM and do not repartition

I don't know enough about this.  Maybe Andres or Thomas want to take 
this.  No concerns if it's safe.

 > [PATCH 3/8] cirrus/freebsd: define 

Looks sensible.

 > [PATCH 4/8] pg_upgrade: tap test: exercise --link and --clone

I haven't been able to get any changes to the test run times outside of 
noise from this.  But some more coverage is sensible in any case.

I'm concerned that with this change, the only platform that tests --copy 
is Windows, but Windows has a separate code path for copy.  So we should 
leave one Unix platform to test --copy.  Maybe have FreeBSD test --link 
and macOS test --clone and leave the others with --copy?

 > [PATCH 5/8] WIP: ci/meson: allow showing only failed tests ..

> client side parsing of things like build logs...)

One thing that actually annoys me that is that a successful run does not 
upload any test artifacts at all.  So, I guess I'm just of a different 
opinion here.

 > [PATCH 6/8] cirrus: code coverage

This adds -Db_coverage=true to the FreeBSD task.  This has a significant 
impact on the build time.  (+50% at least, it appears.)

I'm not sure the approach here makes sense.  For example, if you add a 
new test, the set of changed files is just that test.  So you won't get 
any report on what coverage change the test has caused.

Also, I don't think I trust the numbers from the meson coverage stuff 
yet.  See for example 
<https://www.postgresql.org/message-id/Y/3AI+/MqKcjLk/[email-masked]>.

 > [PATCH 7/8] cirrus: upload changed html docs as artifacts
 > [PATCH 8/8] +html index file

This builds the docs twice and then analyzes the differences between the 
two builds.  This also affects the build times quite significantly.

How useful is this actually?  People who want to look at the docs can 
build them locally.  There are no platform dependencies or anything like 
that where having them built elsewhere is of advantage.

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 15 Mar 2023 09:56:12 -0500

On Wed, Mar 15, 2023 at 10:58:41AM +0100, Peter Eisentraut wrote:
> Needs a better description of what it actually does.  (And fewer "I'm not
> sure how to write this ..." comments ;-) )  It looks like it would fail the
> build if there is a compiler warning in the Windows VS task? Shouldn't that
> be done in the CompilerWarnings task?

The goal is to fail due to warnings only after running tests.

https://www.postgresql.org/message-id/20220212212310.f645c6vw3njkgxka%40alap3.anarazel.de
"Probably worth scripting something to make the windows task error out
if there had been warnings, but only after running the tests."

CompilerWarnings runs in a linux environment running with -Werror.  
This patch scrapes warnings out of MSVC, since (at least historically)
it's too slow to run a separate windows VM to compile with -Werror.

> Also, I see a bunch of warnings in the current output from that task. These
> should be cleaned up in any case before we can let a thing like this loose.

Yeah (and I mentioned those myself).  As it stands, my patch also
"breaks" everytime someone's else's patch introduces warnings.  I
included links demonstrating its failures.

I agree that it's not okay to merge the patch when it's currently
failing, but I cannot dig into that other issue right now.

> impact on the build time.  (+50% at least, it appears.)

Yes - but with the CPUs added by the prior patch, the freebsd task is
faster than it is currently.  And its 8min runtime would match the other
tasks well.

> I'm not sure the approach here makes sense.  For example, if you add a new
> test, the set of changed files is just that test.  So you won't get any
> report on what coverage change the test has caused.

The coverage report that I proposed clearly doesn't handle that case -
it's not intended to.

Showing a full coverage report is somewhat slow to generate, probably
unreasonable to upload for every patch, every day, and not very
interesting since it's at least 99% duplicative.  The goal is to show a
coverage report for new code for every patch.  What fraction of the time
do you think the patch author, reviewer or committer have looked at a
coverage report?  It's not a question of whether it's possible to do so
locally, but of whether it's actually done.

> Also, I don't think I trust the numbers from the meson coverage stuff yet.
> See for example

I'm not using the meson coverage target.  I could instead add
CFLAGS=--coverage.  Anyway, getting a scalar value like "83%" might be
interesting to show in cfbot, but it's not the main goal here.

> This builds the docs twice and then analyzes the differences between the two
> builds.  This also affects the build times quite significantly.

The main goal is to upload the changed docs.

> People who want to look at the docs can build them locally.

This makes the docs for every patch available for reviewers, without
needing a build environment.  An easy goal would be if documentation for
every patch was reviewed by a native english speaker.  Right now that's
not consistently true.

> How useful is this actually?

I'm surprised if there's any question about the merits of making
documentation easily available for review.  Several people have agreed;
one person mailed me privately specifically to ask how to show HTML docs
on cirrusci.

Anyway, all this stuff is best addressed either before or after the CF.
I'll kick the patch forward.  Thanks for looking.

-- 
Justin