Subject: CI and test improvements

From: Justin Pryzby <[email-masked]>
Date: Sat, 28 May 2022 10:37:41 -0500

I'm "joining" a bunch of unresolved threads hoping to present them better since
they're related and I'm maintaining them together anyway.

https://www.postgresql.org/message-id/flat/20220219234148.GC9008%40telsasoft.com
 - set TESTDIR from perl rather than Makefile
https://www.postgresql.org/message-id/flat/20220522232606.GZ19626%40telsasoft.com
 - ccache, MSVC, and meson
https://www.postgresql.org/message-id/20220416144454.GX26620%40telsasoft.com
 - Re: convert libpq uri-regress tests to tap test
https://www.postgresql.org/message-id/CA%2BhUKGLneD%2Bq%2BE7upHGwn41KGvbxhsKbJ%2BM-y9nvv7_Xjv8Qog%40mail.gmail.com
 - Re: A test for replay of regression tests
https://www.postgresql.org/message-id/flat/[email-masked]
 - cfbot requests

See the commit messages for more thread references.

I'm anticipating the need to further re-arrange the patch set - it's not clear
which patches should go first.  Maybe some patches should be dropped in favour
of the meson project.  I guess some patches will have to be re-implemented with
meson (msvc warnings).

I think there was some confusion about the vcregress "alltaptests" target.
I said that it's okay to add it and make cirrus use it (and that the buildfarm
could use it too).  Andrew responded that the buildfarm wants to run different
tests separately.  But Andres seems to have interpretted that as an objection
to the addition of an "alltaptests" target, which I think isn't what's
intended - it's fine if the buildfarm prefers not to use it.

Maybe the improvements to vcregress should go into v15 ?  CI should run all the
tests (which also serves to document *how* to run all the tests, even if there
isn't a literal check-world target).

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 23 Jun 2022 14:31:26 -0500

> they're related and I'm maintaining them together anyway.

This resolves an error with libpq tests in an intermediate commit, probably
caused by rebasing (and maybe hidden by the fact that the tests weren't being
run).

And updates ccache to avoid CCACHE_COMPILER.

Should any of the test changes go into v15 ?

> Subject: [PATCH 02/19] cirrus/vcregress: test modules/contrib with NO_INSTALLCHECK=1
> Subject: [PATCH 08/19] vcregress: add alltaptests
> Subject: [PATCH 14/19] Move libpq_pipeline test into src/interfaces/libpq.
> Subject: [PATCH 15/19] msvc: do not install libpq test tools by default

I also added: cirrus/ccache: disable compression and show stats

Since v4.0, ccache enables zstd compression by default, saving roughly 2x-3x.
But, cirrus caches are compressed as tar.gz, so we could disable ccache
compression, allowing cirrus to gzip the uncompressed data (better than
ccache's default of zstd-1).

https://cirrus-ci.com/build/5194596123672576
debian/bullseye has ccache 4.2; cirrus says 92MB cache after a single compilation; cache_size_kibibyte 102000
macos: has 4.5.1: 46MB cache; cache_size_kibibyte        51252
freebsd: has 3.7.12: 41MB cache; cache_size_kibibyte     130112

For some reason, mac's ccache uses 2x less space than either freesbsd or linux.
Linux is ~30% larger.
Freebsd uploads an artifact 3x smaller than the size ccache reports, because
its ccache is old so doesn't enable compression by default.

I've also sent some patches to Thomas for cfbot to help progress some of these
patches (code coverage and documentation upload as artifacts).
https://github.com/justinpryzby/cfbot/commits/master

-- 
Justin

---

From: Thomas Munro <[email-masked]>
Date: Fri, 24 Jun 2022 08:38:50 +1200

[Resending -- sorry if you receive this twice.  Jacob's mail server
has apparently offended the list management software so emails bounce
if he's in CC.]

> its ccache is old so doesn't enable compression by default.

That port/package got stuck on 3.x because of some dependency problems
when using in bootstrapping FreeBSD itself (or other ports?),
apparently.  I didn't follow the details but the recent messages here
sound hopeful and I'm keeping my eye on it to see if 4.x lands as a
separate package we'd need to install, or something.  Fingers crossed!

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=234971

> https://github.com/justinpryzby/cfbot/commits/master

Thanks, sorry for lack of action, will get to these soon.

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 7 Jul 2022 19:22:32 -0500

rebased on b6a5158f9 and 054325c5e

Also, cirrus/freebsd task can run 3x faster with more CPUs.
Subject: [PATCH 21/21] cirrus: run freebsd with more CPUs+RAM                                                                                                                                                      
https://cirrus-ci.com/task/4664440120410112
https://cirrus-ci.com/task/5100411110555648

In the past, I gather there was some undiagnosed issue when using more CPUs
(cirrus now enforces >5GB RAM when using 6 CPUs - maybe you tried to use too
little RAM, or maybe hit bad performance involving NUMA?)
https://www.postgresql.org/message-id/flat/[email-masked]#f36c0b17e33e31e7925e7e5812998686

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 28 Aug 2022 09:44:47 -0500

Resending with a problematic address removed...

> Also, cirrus/freebsd task can run 3x faster with more CPUs.

Checking if there's interest in any/none of these patches ?
I have added several more.

Do you have an idea when the meson branch might be merged ?

Will vcregress remain for a while, or will it go away for v16 ?

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Sun, 28 Aug 2022 09:07:52 -0700

Hi,

> Do you have an idea when the meson branch might be merged ?

I hope to do that fairly soon, but it's of course dependant on review etc. The
plan was to merge it early and mature it in tree to some degree. There's only
so much we can do "from the outside"...

> Will vcregress remain for a while, or will it go away for v16 ?

The plan was for the windows stuff to go away fairly quickly.

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

Looks good.

> -    MSBFLAGS: -m -verbosity:minimal "-consoleLoggerParameters:Summary;ForceNoAlign" /p:TrackFileAccess=false -nologo
> +    # -fileLoggerParameters1: write warnings to msbuild.warn.log.
> +    MSBFLAGS: -m -verbosity:minimal "-consoleLoggerParameters:Summary;ForceNoAlign" /p:TrackFileAccess=false -nologo -fileLoggerParameters1:warningsonly;logfile=msbuild.warn.log

Except, I think it'd be good to split this line. What do you think about using
something like
MSBFLAGS: >-
  -nologo
  -m -verbosity:minimal
  /p:TrackFileAccess=false
  "-consoleLoggerParameters:Summary;ForceNoAlign"
  -fileLoggerParameters1:warningsonly;logfile=msbuild.warn.log

I think that should work?

>      # If tests hang forever, cirrus eventually times out. In that case log
>      # output etc is not uploaded, making the problem hard to debug. Of course
> @@ -450,6 +451,11 @@ task:
>      cd src/tools/msvc
>      %T_C% perl vcregress.pl ecpgcheck
>
> +  # These should be last, so all the important checks are always run
> +  always:
> +    compiler_warnings_script:
> +      - sh src\tools\ci\windows-compiler-warnings msbuild.warn.log
> +
>    on_failure:
>      <<: *on_failure
>      crashlog_artifacts:
> diff --git a/src/tools/ci/windows-compiler-warnings b/src/tools/ci/windows-compiler-warnings
> new file mode 100755
> index 00000000000..d6f9a1fc569
> --- /dev/null
> +++ b/src/tools/ci/windows-compiler-warnings
> @@ -0,0 +1,16 @@
> +#! /bin/sh
> +# Success if the given file doesn't exist or is empty, else fail
> +# This is a separate file only to avoid dealing with windows shell quoting and escaping.
> +set -e
> +
> +fn=$1
> +
> +if [ -s "$fn" ]
> +then
> +	# Display the file's content, then exit indicating failure
> +	cat "$fn"
> +	exit 1
> +else
> +	# Success
> +	exit 0
> +fi
> --
> 2.17.1

Wouldn't that be doable as something like
sh -c 'if test -s file; then cat file;exit 1; fi"
inside .cirrus.yml?

> From 1064a0794e85e06b3a0eca4ed3765f078795cb36 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <[email-masked]>
> Date: Sun, 3 Apr 2022 00:10:20 -0500
> Subject: [PATCH 03/25] cirrus/ccache: disable compression and show stats
>
> ccache since 4.0 enables zstd compression by default.
>
> With default compression enabled (https://cirrus-ci.com/task/6692342840164352):
> linux has 4.2; 99MB cirrus cache; cache_size_kibibyte	109616
> macos has 4.5.1: 47MB cirrus cache; cache_size_kibibyte	52500
> freebsd has 3.7.12: 42MB cirrus cache; cache_size_kibibyte	134064
> windows has 4.6.1; 180MB cirrus cache; cache_size_kibibyte	51179
> todo: compiler warnings
>
> With compression disabled (https://cirrus-ci.com/task/4614182514458624):
> linux: 91MB cirrus cache; cache_size_kibibyte	316136
> macos: 41MB cirrus cache; cache_size_kibibyte	118068
> windows: 42MB cirrus cache; cache_size_kibibyte	134064
> freebsd is the same
>
> The stats should either be shown or logged (or maybe run with CCACHE_NOSTATS,
> to avoid re-uploading cache tarball in a 100% cached build, due only to
> updating ./**/stats).
>
> Note that ccache 4.4 supports CCACHE_STATSLOG, which seems ideal.

I stared at this commit message for a while, trying to make sense of it, and
couldn't really. I assume you're saying that the cirrus compression is better
with ccache compression disabled, but it's extremely hard to parse out of it.

This does too much at once. Show stats, change cache sizes, disable
compression.

> From 01e9abd386a4e6cc0125b97617fb42e695898cbf Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <[email-masked]>
> Date: Tue, 26 Jul 2022 20:30:02 -0500
> Subject: [PATCH 04/25] cirrus/ccache: add explicit cache keys..
>
> Since otherwise, building with ci-os-only will probably fail to use the normal
> cache, since the cache key is computed using both the task name and its *index*
> in the list of caches (internal/executor/cache.go:184).

Hm, perhaps worth confirming and/or reporting to cirrus rather?

> From 8de5c977686270b0a4e666a924ebe820a245913a Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <[email-masked]>
> Date: Sun, 24 Jul 2022 23:09:12 -0500
> Subject: [PATCH 05/25] silence make distprep and generated-headers
>
> this saves vertical screen space.
>
> https://www.postgresql.org/message-id/[email-masked]

I don't feel this should go in as a part of CI changes. Or rather, I feel
uncomfortable committing it when just discussed under this subject.

> From eaf263ccaa8310c5d9834b97e93ad8434d63296e Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <[email-masked]>
> Date: Sun, 24 Jul 2022 22:44:53 -0500
> Subject: [PATCH 06/25] pg_regress: run more quietly
>
> The number of lines of output should be closer to 1 per test, rather than 25 +
> 1 per test.
>
> https://www.postgresql.org/message-id/flat/20220221173109.yl6dqqu3ud52ripd%40alap3.anarazel.de

See above. There's also a dedicated thread about revising the output.

> From 6a6a97fc869fd1fd8b7ab5da5147f145581634f9 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <[email-masked]>
> Date: Fri, 24 Jun 2022 00:09:12 -0500
> Subject: [PATCH 08/25] cirrus/freebsd: run with more CPUs+RAM and do not
>  repartitiion
>
> 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:
> 8 jobs 7min https://cirrus-ci.com/task/6186376667332608
>
> xi-os-only: freebsd

Typo.

> @@ -71,8 +69,6 @@ task:
>      fingerprint_key: ccache/freebsd
>      reupload_on_changes: true
>
> -  # Workaround 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 .
> --

What's the story there - at some point that was important for performance
because of the native block size triggering significant read-modify-write
cycles with postres' writes. You didn't comment on it in the commit message.

> From fd1c36a0bd8fa608ccdff5be3735dac5e3e48bf3 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <[email-masked]>
> Date: Wed, 27 Jul 2022 16:54:47 -0500
> Subject: [PATCH 09/25] cirrus/freebsd: run build+check in a make vpath

> From 7052a32a21752b59632225684fc9426bb94e46e0 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <[email-masked]>
> Date: Sun, 13 Feb 2022 17:56:40 -0600
> Subject: [PATCH 10/25] cirrus/windows: increase timeout to 25min

No explanation?

> From 602983b2cf37fc43465c62330b2e15e9d6d2035d Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <[email-masked]>
> Date: Fri, 26 Aug 2022 12:00:10 -0500
> Subject: [PATCH 15/25] f!and chdir

I don't see the point of pointing fixup commits to the list.

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 28 Aug 2022 12:10:29 -0500

> sh -c 'if test -s file; then cat file;exit 1; fi"

I had written it inline in a couple ways, like
- sh -exc 'f=msbuild.warn.log; if [ -s "$f" ]; then cat "$f"; exit 1; else exit 0; fi'

but then separated it out as you suggested in
[email-masked]

after I complained about cmd.exe requiring escaping for && and ||
That makes writing any shell script a bit perilous and a separate script
seems better.

> with ccache compression disabled, but it's extremely hard to parse out of it.

Yes, because ccache uses zstd-1, and cirrus uses gzip, which it's going
to use no matter what ccache does, and gzip's default -6 is better than
ccache's zstd-1.

> This does too much at once. Show stats, change cache sizes, disable

The cache size change is related to the compression level change; ccache
prunes based on the local size, which was compressed with zstd-1 and,
with this patch, not compressed (so ~2x larger).  Also, it's more
interesting to control the size uploaded to cirrus (after compression
ith gzip-6).

> Hm, perhaps worth confirming and/or reporting to cirrus rather?

I know because of reading their source.  Unfortunately, there's no
commit history indicating the intent or rationale.
https://github.com/cirruslabs/cirrus-ci-agent/blob/master/internal/executor/cache.go#L183

> > 8 jobs 7min https://cirrus-ci.com/task/6186376667332608

No - it's deliberate so I can switch to and from "everything" to "this
only".

> cycles with postres' writes. You didn't comment on it in the commit message.

Well, I don't know the history, but it seems to be unneeded now.

Is there a good description of the original problem ?  Originally,
freebsd check-world took ~15min to run tests, and when we changed to use
-Og it took 10min.  Since then, seems to have improved on its own, and
currently takes ~6min.  This patch adds CPUs to make it run in ~4min,
and takes the opportuity to drop the historic repartition stuff.

> > Subject: [PATCH 10/25] cirrus/windows: increase timeout to 25min

Because of the immediately following commit which makes it run all the
tests.

> I don't see the point of pointing fixup commits to the list.

It's a separate commit to make it easy to see the changes, separately,
since I imagine maybe the "chdir" part won't be desirable, or maybe the
PATH part won't.  But I'm not sure, so I'm here soliciting feedback.

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Sun, 28 Aug 2022 14:28:02 -0700

Hi,

> That makes writing any shell script a bit perilous and a separate script

I remember that I suggested it - but note that the way I wrote above doesn't
have anything needing escaping. Anyway, what do you think of the multiline
split I suggested?

> interesting to control the size uploaded to cirrus (after compression

That's what should have been in the commit message.

> No - it's deliberate so I can switch to and from "everything" to "this

I don't see the point in posting patches to be applied if they contain lots of
such things that a potential committer would need to catch and include a lot
of of fixup patches.

> Well, I don't know the history, but it seems to be unneeded now.

It's possible it was mainly needed for testing with aio + dio. But also
possible that an upgrade improved the situation since.

> Because of the immediately following commit which makes it run all the

Mention that in the commit message then. Especially when dealing with 25
commits I don't think you can expect others to infer such things.

> PATH part won't.  But I'm not sure, so I'm here soliciting feedback.

Shrug, I doubt you'll get much if asked that way.

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 10 Sep 2022 15:05:42 -0500

On Sun, Aug 28, 2022 at 02:28:02PM -0700, Andres Freund wrote:
> have anything needing escaping. 

It doesn't require it, but that still gives the impression that it's
normally possible to write one-liner shell scripts there, which is
misleading/wrong, and the reason why I took your suggestion to use a
separate script file.

> Anyway, what do you think of the multiline split I suggested?

Done, and sorted.

> That's what should have been in the commit message.

Sure.  I copied into the commit message the explanation that I had
written in June's email.

> such things that a potential committer would need to catch and include a lot

I get that you disliked that I disabled the effect of a CI tag by
munging "c" to "x".  I've amended the message to avoid confusion.  But,
lots of what such things ?  "ci-os-only" would be removed before being
pushed anyway.

"catching things" is the first part of the review process, which (as I
understand) is intended to help patch authors to improve their patches.
If you found lots of problems in my patches, I'd need to know about
them; but most of what I heard seem like quibbles about the presentation
of the patches.  It's true that some parts are dirty/unclear, and that
seems reasonable for patches most of which haven't yet received review,
for which I asked whether to pursue the patch at all, and how best to
present them.  This is (or could be) an opportunity to make
improvements.

I renamed the two, related patches to Cluser.pm which said "f!", which
are deliberately separate but looked like "fixup" patches.  Are you
interested in any combination of those three, related changes to move
logic from Makefile to perl ?  If not, we don't need to debate the
merits of spliting the patch.

What about the three, related changes for ccache compression ?

Should these be dropped in favour of meson ?
 - cirrus/vcregress: test modules/contrib with NO_INSTALLCHECK=1
 - vcregress: add alltaptests

I added: "WIP: skip building if only docs have changed"

changesInclude() didn't seem to work right when I first tried to use it.
Eventually, I realized that it seems to use something like "git log",
and not "git diff" (as I'd thought).  It seems to work fine now that I
know what to expect.

git commit --amend --no-edit
git diff --stat @{1}..@{0} # this outputs nothing
git log  --stat @{1}..@{0} # this lists the files changed by the tip commit

It'd be nice to be have cfbot inject this patch into each commitfest
patch for awhile, to make sure everything works as expected.  Same for
the code coverage patch and the doc artifacts patch.  (These patches
currently assume that the base commit is HEAD~1, which is correct for
cfbot, and that would also provide code coverage and docs until such
time as cfbot is updated to apply and preserve the original series of
patches).

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 22 Sep 2022 16:07:02 -0500

> possible that an upgrade improved the situation since.

Maybe freebsd got faster as a result of the TAU CPUs?
https://mobile.twitter.com/cirrus_labs/status/1534982111568052240

I noticed because it's been *slower* the last ~24h since cirrusci
disabled TAU, as Thomas commit mentioned.
https://twitter.com/cirrus_labs/status/1572657320093712384

For example this CF entry:

https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3736
https://cirrus-ci.com/task/4670794365140992 5m36s - 4days ago
https://cirrus-ci.com/task/4974926233862144 5m25s - 3days ago
https://cirrus-ci.com/task/5561409034518528 5m29s - 2days ago
https://cirrus-ci.com/task/6432442008469504 9m19s - yesterday

CF_BOT's latest tasks seem to be fast again, since 1-2h ago.
https://cirrus-ci.com/build/5178906041909248 9m1s
https://cirrus-ci.com/build/4593160281128960 5m8s
https://cirrus-ci.com/build/4539845644124160 5m22s

The logs for July show when freebsd started "being fast":
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/38/3708
https://cirrus-ci.com/task/6316073015312384 10m25s Jul 13
https://cirrus-ci.com/task/5662878987452416 5m48s Jul 15

Maybe that changed in July rather than June because the TAU CPUs were
still not available in every region/zone (?)
https://cloud.google.com/compute/docs/regions-zones/

I have no idea if the TAU CPUs eliminate/mitigate the original
performance issue you had with AIO.  But they have such a large effect
on freebsd that it could now be the fastest task, if given more than 2
CPUs.

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Thu, 22 Sep 2022 14:53:23 -0700

Hi,

> https://twitter.com/cirrus_labs/status/1572657320093712384

Yea, I noticed that as well. It's entirely possible that something in the
"hardware" stack improved sufficiently to avoid problems.

> on freebsd that it could now be the fastest task, if given more than 2

I'm planning to rebase early next week and try that out.

Greetings,

Andres Freund

---

From: Thomas Munro <[email-masked]>
Date: Fri, 23 Sep 2022 15:32:48 +1200

> > possible that an upgrade improved the situation since.

Yeah, it is very important for direct I/O (patches soon...), because
every 8KB random write becomes a read-32KB/wait/write-32KB without it.
It's slightly less important for buffered I/O, because the kernel
caches hide that, but it still triggers I/O bandwidth amplification,
and we definitely saw positive effects earlier on the CI system back
on the previous generation.  FWIW I am planning to see about getting
the FreeBSD installer to create the root file system the way we want,
instead of this ugliness.

> [data]

Very interesting.  Would be good to find the exact instance types +
storage types to see if there has been a massive IOPS boost, perhaps
via local SSD.  The newer times are getting closer to a local
developer machine.

---

From: Andres Freund <[email-masked]>
Date: Sat, 1 Oct 2022 17:45:01 -0700

Hi,

On 2022-09-10 15:05:42 -0500, Justin Pryzby wrote:
> From 4ed5eb427de4508a4c3422e60891b45c8512814a Mon Sep 17 00:00:00 2001
> Subject: [PATCH 03/23] cirrus/ccache: disable compression and show stats
> 
> Since v4.0, ccache enables zstd compression by default, saving roughly
> 2x-3x.  But, cirrus caches are compressed as tar.gz, so we could disable
> ccache compression, allowing cirrus to gzip the uncompressed data
> (better than ccache's default of zstd-1).

I wonder whether we could instead change CCACHE_COMPRESSLEVEL (maybe 3, zstd's
default IIRC). It'd be good if we could increase cache utilization.

> From 0bd5f51b8c143ed87a867987309d66b8554b1fd6 Mon Sep 17 00:00:00 2001
> Date: Thu, 14 Apr 2022 06:27:07 -0500
> Subject: [PATCH 05/23] cirrus: enable various runtime checks on macos and
>  freebsd
> 
> windows is slower than freebsd and mac, so it's okay to enable options which
> will slow them down some.  Also, the cirrusci mac instances always have lot of
> cores available.

> See:
> https://www.postgresql.org/message-id/[email-masked]
> https://www.postgresql.org/message-id/20211213211223.vkgg3wwiss2tragj%40alap3.anarazel.de
> https://www.postgresql.org/message-id/CAH2-WzmevBhKNEtqX3N-Tkb0gVBHH62C0KfeTxXzqYES_PiFiA%40mail.gmail.com
> https://www.postgresql.org/message-id/[email-masked]
> 
> ci-os-only: freebsd, macos
> ---
>  .cirrus.yml | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 183e8746ce6..4ad20892eeb 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -113,7 +113,9 @@ task:
>          \
>          CC="ccache cc" \
>          CXX="ccache c++" \
> -        CFLAGS="-Og -ggdb"
> +        CPPFLAGS="-DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST" \
> +        CXXFLAGS="-Og -ggdb -march=native -mtune=native" \
> +        CFLAGS="-Og -ggdb -march=native -mtune=native"

What's reason for -march=native -mtune=native here?

>      EOF
>    build_script: |
>      su postgres -c "ccache --zero-stats"
> @@ -336,8 +338,8 @@ task:
>        CC="ccache cc" \
>        CXX="ccache c++" \
>        CLANG="ccache ${brewpath}/llvm/bin/ccache" \
> -      CFLAGS="-Og -ggdb" \
> -      CXXFLAGS="-Og -ggdb" \
> +      CFLAGS="-Og -ggdb -DRANDOMIZE_ALLOCATED_MEMORY" \
> +      CXXFLAGS="-Og -ggdb -DRANDOMIZE_ALLOCATED_MEMORY" \
>        \
>        LLVM_CONFIG=${brewpath}/llvm/bin/llvm-config \
>        PYTHON=python3

I'd also use CPPFLAGS here, given you'd used it above...

I'm planning to commit an updated version of this change soon, without the
-march=native -mtune=native bit, unless somebody protests...

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 1 Oct 2022 19:58:01 -0500

> default IIRC). It'd be good if we could increase cache utilization.

I considered that (and I think that's what I wrote initially).

I figured that if cirrus is going to use gzip-6 (tar.gz) in any case, we
might as well disable compression.  Then, all the tasks are also doing
the same thing (half the tasks have ccache before 4.0).

> What's reason for -march=native -mtune=native here?

No particular reason, and my initial patch didn't have it.
I suppose I added it to test its effect and never got rid of it.

> -march=native -mtune=native bit, unless somebody protests...

One other thing is that your -m32 changes caused the linux/meson task to
take an additional 3+ minutes (total ~8).  That's no issue, except that
the Warnings task depends on the linux/mason task, and itself can take
up to 15 minutes.

So those two potentially take as long as the windows task.
I suggested that CompileWarnings could instead "Depend on: Freebsd",
which currently takes 6-7min (and could take 4-5min if given more CPUs).

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Sat, 1 Oct 2022 18:36:41 -0700

Hi,

> which currently takes 6-7min (and could take 4-5min if given more CPUs).

I am wondering if we should instead introduce a new "quickcheck" task that
just compiles and runs maybe one test and have *all* other tests depend on
that.  Wasting a precious available windows instance to just fail to build or
immediately fail during tests doesn't really make sense.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Sun, 2 Oct 2022 13:52:01 -0700

Hi,

> immediately fail during tests doesn't really make sense.

Attached is an implementation of that idea.

I fairly randomly chose two quick tests to execute as part of the sanity
check, cube/regress pg_ctl/001_start_stop. I wanted to have coverage for
initdb, a pg_regress style test, a tap test, some other client binary.

With a primed cache this takes ~32s, not too bad imo. 12s of that is cloning
the repo.

What do you think?

We could bake a bare repo into the images to make the clone step in faster,
but that'd be for later anyway.

set -e
rm -rf /tmp/pg-clone-better
mkdir /tmp/pg-clone-better
cd /tmp/pg-clone-better
git init --bare
git remote add origin https://github.com/postgres/postgres.git --no-tags -t 'REL_*' -t master
git fetch -v
git repack -ad -f
du -sh

results in a 227MB repo.

git clone https://github.com/anarazel/postgres.git -v --depth 1000 -b ci-sanitycheck --reference /tmp/pg-clone-better /tmp/pg-clone-better-clone

clones an example branch in ~1.35s.

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 2 Oct 2022 16:35:06 -0500

> With a primed cache this takes ~32s, not too bad imo. 12s of that is
> cloning the repo.

Maybe - that would avoid waiting 4 minutes for a windows instance to
start in the (hopefully atypical) case of a patch that fails in 1-2
minutes under linux/freebsd.

If the patch were completely broken, the windows task would take ~4min
to start, plus up to ~4min before failing to compile or failing an early
test.  6-8 minutes isn't nothing, but doesn't seem worth the added
complexity.

Also, this would mean that in the common case, the slowest task would be
delayed until after the SanityCheck task instance starts, compiles, and
runs some test :( Your best case is 32sec, but I doubt that's going to
be typical.

I was thinking about the idea of cfbot handling "tasks" separately,
similar to what it used to do with travis/appveyor.  The logic for
"windows tasks are only run if linux passes tests" could live there.
That could also be useful if there's ever the possibility of running an
additional OS on another CI provider, or if another provider can run
windows tasks faster, or if we need to reduce our load/dependency on
cirrus.  I realized that goes backwards in some ways to the direction
we've gone with cirrus, and I'm not sure how exactly it would do that (I
suppose it might add ci-os-only tags to its commit message).

> +    # no options enabled, should be small
> +    CCACHE_MAXSIZE: "150M"

Actually, tasks can share caches if the "cache key" is set.

If there was a separate "Sanity" task, I think it should use whatever
flags linux (or freebsd) use to avoid doing two compilations (with lots
of cache misses for patches which modify *.h files, which would then
happen twice, in serial).

> +  # use always: to continue after failures. Task that did not run count as a
> +  # success, so we need to recheck SanityChecks's condition here ...

> -  # task that did not run, count as a success, so we need to recheck Linux'
> -  # condition here ...

Another/better justification/description is that "cirrus warns if the
depending task has different only_if conditions than the dependant task".

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Sun, 2 Oct 2022 14:51:23 -0700

Hi,

> test.  6-8 minutes isn't nothing, but doesn't seem worth the added

Avoiding 6-8mins of wasted windows time would, I think, allow us to crank
cfbot's concurrency up a notch or two.

> runs some test :( Your best case is 32sec, but I doubt that's going to

Even the worst case isn't that bad, the uncached minimal build is 67s.

> "windows tasks are only run if linux passes tests" could live there.

I don't really see the advantage of doing that over just increasing
concurrency by a bit.

> happen twice, in serial).

I think the price of using exactly the same flags is higher than the gain. And
it'll rarely work if we use the container task for the sanity check, as the
timestamps of the compiler, system headers etc will be different.

> depending task has different only_if conditions than the dependant task".

That doesn't really seem easier to understand to me.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Sun, 2 Oct 2022 14:54:21 -0700

Hi,

> test.  6-8 minutes isn't nothing, but doesn't seem worth the added

Btw, the motivation to work on this just now was that I'd like to enable more
sanitizers (undefined,alignment for linux-meson, address for
linux-autoconf). Yes, we could make the dependency on freebsd instead, but I'd
like to try to enable the clang-only memory sanitizer there (if it works on
freebsd)...

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 2 Oct 2022 21:15:41 -0500

On Sun, Oct 02, 2022 at 02:54:21PM -0700, Andres Freund wrote:
> the clang-only memory sanitizer there (if it works on freebsd)...

Have you looked at this much ?  I think it'll require a bunch of
exclusions, right ?

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 4 Nov 2022 18:54:12 -0500

> > Anyway, what do you think of the multiline split I suggested?

Rewrote this and rebased some of the other stuff on top of the meson
commit, for which I also include some new patches.

---

From: Andres Freund <[email-masked]>
Date: Fri, 4 Nov 2022 18:59:46 -0700

Hi,

On 2022-11-04 18:54:12 -0500, Justin Pryzby wrote:
> Subject: [PATCH 1/8] meson: PROVE is not required
> Subject: [PATCH 3/8] meson: rename 'main' tasks to 'regress' and 'isolation'

Pushed, thanks for the patches.

> Subject: [PATCH 2/8] meson: other fixes for cygwin
>
> XXX: what about HAVE_BUGGY_STRTOF ?

What are you wondering about here? Shouldn't that continue to be set via
src/include/port/cygwin.h?

> diff --git a/src/test/regress/meson.build b/src/test/regress/meson.build
> index 3dcfc11278f..6ec3c77af53 100644
> --- a/src/test/regress/meson.build
> +++ b/src/test/regress/meson.build
> @@ -10,7 +10,7 @@ regress_sources = pg_regress_c + files(
>  # patterns like ".*-.*-mingw.*". We probably can do better, but for now just
>  # replace 'gcc' with 'mingw' on windows.
>  host_tuple_cc = cc.get_id()
> -if host_system == 'windows' and host_tuple_cc == 'gcc'
> +if host_system in ['windows', 'cygwin'] and host_tuple_cc == 'gcc'
>    host_tuple_cc = 'mingw'
>  endif

This doesn't quite seem right - shouldn't it say cywin? Not that it makes a
difference right now, given the contents of resultmap:
float4:out:.*-.*-cygwin.*=float4-misrounded-input.out
float4:out:.*-.*-mingw.*=float4-misrounded-input.out

> From 0acbbd2fdd97bbafc5c4552e26f92d52c597eea9 Mon Sep 17 00:00:00 2001
> Subject: [PATCH 4/8] cirrus/windows: add compiler_warnings_script
>
> I'm not sure how to write this test in windows shell; it's also not easy to
> write it in posix sh, since windows shell is somehow 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                            | 10 +++++++++-
>  src/tools/ci/windows-compiler-warnings | 24 ++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
>  create mode 100755 src/tools/ci/windows-compiler-warnings
>
> index 9f2282471a9..99ac09dc679 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -451,12 +451,20 @@ task:
>
>    build_script: |
>      vcvarsall x64
> -    ninja -C build
> +    ninja -C build |tee build/meson-logs/build.txt
> +    REM Since pipes lose exit status of the preceding command, rerun compilation,
> +    REM without the pipe exiting now if it fails, rather than trying to run checks
> +    ninja -C build > nul

This seems mighty grotty :(. but I guess it's quick enough not worry about,
and I can't come up with a better plan.

It doesn't seem quite right to redirect into meson-logs/ to me, my
interpretation is that that's "meson's namespace".  Why not just store it in
build/?

> From e85fe83fd8a4b4c79a96d2bf66cd6a5e1bdfcd1e Mon Sep 17 00:00:00 2001
> Date: Sat, 26 Feb 2022 19:34:35 -0600
> Subject: [PATCH 5/8] cirrus: build docs as a separate task..
>
> This will run the doc build if any docs have changed, even if Linux
> fails, to allow catch doc build failures.
>
> This'll automatically show up as a separate "column" on cfbot.
>
> Also, in the future, this will hopefully upload each patch's changed HTML docs
> as an artifact, for easy review.
>
> Note that this is currently building docs with both autoconf and meson.
>
> ci-os-only: html
> ---
>  .cirrus.yml | 62 +++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 44 insertions(+), 18 deletions(-)
>
> index 99ac09dc679..37fd79e5b77 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -472,6 +472,9 @@ task:
>        type: text/plain
>
>
> +###
> +# Test that code can be built with gcc/clang without warnings
> +###
>  task:
>    name: CompilerWarnings
>
> @@ -515,10 +518,6 @@ task:
>      #apt-get update
>      #DEBIAN_FRONTEND=noninteractive apt-get -y install ...
>
> -  ###
> -  # Test that code can be built with gcc/clang without warnings
> -  ###
> -

Why remove this?

>    setup_script: echo "COPT=-Werror" > src/Makefile.custom
>
>    # Trace probes have a history of getting accidentally broken. Use the
> @@ -580,20 +579,6 @@ task:
>        make -s -j${BUILD_JOBS} clean
>        time make -s -j${BUILD_JOBS} world-bin
>
> -  ###
> -  # Verify docs can be built
> -  ###
> -  # 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
> -
>    ###
>    # Verify headerscheck / cpluspluscheck succeed
>    #
> @@ -617,3 +602,44 @@ task:
>
>    always:
>      upload_caches: ccache
> +
> +
> +###
> +# Verify docs can be built
> +# changesInclude() will skip this task if none of the commits since
> +# CIRRUS_LAST_GREEN_CHANGE touched any relevant files. The comparison appears
> +# to be like "git log a..b -- ./file", not "git diff a..b -- ./file"
> +###
> +
> +task:
> +  name: Documentation
> +
> +  env:
> +    CPUS: 1
> +    BUILD_JOBS: 1
> +
> +  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*(docs|html).*'
> +  skip: "!changesInclude('.cirrus.yml', 'doc/**')"

Perhaps we should introduce something other than ci-os-only if we want that to
include things like "docs and html".  At least this should update
src/tools/ci/README.

> +  sysinfo_script: |
> +    id
> +    uname -a
> +    cat /proc/cmdline
> +    ulimit -a -H && ulimit -a -S
> +    export

I think we can skip this here.

> +  # Exercise HTML and other docs:
> +  ninja_docs_build_script: |
> +    mkdir build.ninja
> +    cd build.ninja

Perhaps build-ninja instead? build.ninja is the filename for ninja's build
instructions, so it seems a bit confusing.

> From adebe93a4409990e929f2775d45c6613134a4243 Mon Sep 17 00:00:00 2001
> Subject: [PATCH 6/8] cirrus/ccache: add explicit cache keys..
>
> Since otherwise, building with ci-os-only will probably fail to use the
> normal cache, since the cache key is computed using both the task name
> and its *index* in the list of caches (internal/executor/cache.go:184).

Seems like this would potentially better addressed by reporting a bug to the
cirrus folks?

>    ccache_cache:
>      folder: ${CCACHE_DIR}
> +    fingerprint_key: ccache/linux
> +    reupload_on_changes: true

There's enough copies of this to make it worth deduplicating. If we use
something like
   fingerprint_script: echo ccache/$CIRRUS_BRANCH/$CIRRUS_OS
we can use a yaml ref?

I think you experimented with creating a 'base' ccache dir (e.g. on the master
branch) and then using branch specific secondar caches? How did that turn out?
I think cfbot's caches constantly get removed due to overrunning the global
space.

> From f16739bc5d2087847129baf663aa25fa9edb8449 Mon Sep 17 00:00:00 2001
> Subject: [PATCH 7/8] cirrus/ccache: disable compression and show stats

> linux/debian/bullseye has 4.2; 99MB cirrus cache; cache_size_kibibyte	109616
> XXX windows has 4.7.2; 180MB cirrus cache; cache_size_kibibyte	51179
> todo: compiler warnings
>

I'm still somewhat doubtful this is a good idea. The mingw cache is huge, for
example, and all that additional IO and memory usage is bound to show up.

> The stats should be shown and/or logged.
> ccache --show-stats shows the *cumulative* stats (including prior
> compilations)
> ccache --zero-stats clears out not only the global stats, but the
> per-file cache stats (from which the global stats are derived) - which
> obviously makes the cache work poorly.
>
> The log should be written *outside* the ccache folder - it shouldn't be
> preserved across cirrusci task invocations.

I assume we don't have a new enough ccache everywhere yet?

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 13 Nov 2022 17:53:04 -0600

> Pushed, thanks for the patches.

Thanks.

> interpretation is that that's "meson's namespace".  Why not just store it in

I put it there so it'd be included with the build artifacts.
Maybe it's worth adding a separate line to artifacts for stuff like
this, and ccache log ?

> instructions, so it seems a bit confusing.

Sure.

Do you think building docs with both autoconf and meson is what's
desirable here ?

I'm not sure if this ought to be combined with/before/after your "move
compilerwarnings task to meson" patch?  (Regarding that patch: I
mentioned that it shouldn't use ccache -C, and it should use
meson_log_artifacts.)

> Seems like this would potentially better addressed by reporting a bug to the

You said that before, but I don't think so - since they wrote code to do
that, it's odd to file a bug that says that the behavior is wrong.  I am
curious why, but it seems delibrate.

https://www.postgresql.org/message-id/20220828171029.GO2342%40telsasoft.com

> There's enough copies of this to make it worth deduplicating. If we
> use something like fingerprint_script: echo
> ccache/$CIRRUS_BRANCH/$CIRRUS_OS we can use a yaml ref?                                                                                                                                                                                                        
I'll look into it...

> branch) and then using branch specific secondar caches?

I have to revisit that sometime.

That's a new feaure in ccache 4.4, which is currently only in macos.
This is another thing that it'd be easier to test by having cfbot
clobber the cirrus.yml rather than committing to postgres repo.
(Technically, it should probably only do use the in-testing cirrus.yml
if the patch it's testing doesn't itself modify .cirrus.yml)

> How did that turn out?  I think cfbot's caches constantly get removed
> due to overrunning the global space.

For cfbot, I don't know if there's much hope that any patch-specific
build artifacts will be cached from the previous run, typically ~24h
prior.

One idea I have, for the "Warnings" task (and maybe linux too), is to
defer pruning until after all the compilations.  To avoid LRU pruning
during early tasks causing bad hit ratios of later tasks.

Another thing that probably happens is that task1 starts compiling
patch1, and then another instance of task1 starts compiling patch2.  A
bit later, the first instance will upload its ccache result for patch1,
which will be summarily overwritten by the second instance's compilation
result, which doesn't include anything from the first instance.

Also, whenever ccache hits its MAXSIZE threshold, it prunes the cache
down to 80% of the configured size, which probably wipes away everything
from all but the most recent ~20 builds.

I also thought about having separate caches for each compilation in the
warnings task - but that requires too much repeated yaml just for that..

> example, and all that additional IO and memory usage is bound to show up.

I think you're referring to the proposed mingw task which runs under
windows, and not the existing cross-compilation ?

And you're right - I remember this now (I think it's due to PCH?)

In my local copy I'd "unset CCACHE_NOCOMPRESS".  But I view that as an
oddity of windows headers, rather than an argument against disabling
compression elsewhere.  BTW, freebsd ccache is too old to use
compression.

What about using CCACHE_HARDLINK (which implies no compression) ?

> I assume we don't have a new enough ccache everywhere yet?

No - see above.

I've added patches to update macos.

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Wed, 16 Nov 2022 19:48:14 -0800

Hi,

> like to try to enable the clang-only memory sanitizer there (if it works on

I've used this a bunch on personal branches, and I think it's the way to
go. It doesn't take long, saves a lot of cycles when one pushes something
broken. Starts to runs the CompilerWarnings task after a minimal amount of
sanity checking, instead of having to wait for a task running all tests,
without the waste of running it immediately and failing all the different
configurations, which takes forever.

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 16 Nov 2022 21:58:39 -0600

> configurations, which takes forever.

Well, I don't hate it.

But I don't think you should call "ccache -z":

On Tue, Oct 18, 2022 at 12:09:30PM -0500, Justin Pryzby wrote:
> I realized that ccache -z clears out not only the global stats, but the

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Wed, 16 Nov 2022 20:08:32 -0800

Hi,

> But I don't think you should call "ccache -z":

Agreed - that was really just for "development" of the task.

I also don't like my "cores_script". Not quite sure yet how to do that
more cleanly.

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 16 Nov 2022 22:16:31 -0600

> I also don't like my "cores_script". Not quite sure yet how to do that

I don't know which is cleaner:

ls /core* && mv /tmp/core* /tmp/cores

find / -maxdepth 1 -type f -name 'core*' -print0 |
	xargs -r0 mv -vt /tmp/cores

for a in /core*; do [ ! -e "$a" ] || mv "$a" /tmp/cores; done

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 19 Nov 2022 14:22:20 -0600

On Sun, Oct 02, 2022 at 01:52:01PM -0700, Andres Freund wrote:
>  
> +# To avoid unnecessarily spinning up a lot of VMs / containers for entirely
> +# broken commits, have a very minimal test that all others depend on.
> +task:
> +  name: SanityCheck

Maybe this should be named 00-SanityCheck, so it sorts first in cfbot ?

Also, if CompilerWarnings doesn't depend on Linux, that means those two
tasks will normally start and run simultaneously, which means a single
branch will use all 8 of the linux CPUs available from cirrus.  Is that
intentional?

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Sat, 19 Nov 2022 13:18:54 -0800

Hi,

> Maybe this should be named 00-SanityCheck, so it sorts first in cfbot ?

Hm. Perhaps cfbot could just use the sorting from cirrus? I don't really like
the idea of making the names more confusing with numbered prefixes,
particularly when only used for some but not other tasks.

> branch will use all 8 of the linux CPUs available from cirrus.  Is that

I don't think that'd really make anything worse. But perhaps we could just
reduce the CPU count for linux autoconf by 1? I suspect that even with asan
enabled it'd still be roughly even with the rest.

I'll try to repost a version of the ubsan/asan patch together with the
sanitycheck patch and see how that looks.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Sat, 19 Nov 2022 13:35:17 -0800

Hi,

> enabled it'd still be roughly even with the rest.

Hm, that doesn't suffice, because we allow 4 cores for the warnings task. The
limit for cirrus is 16 linux CPUs though, not 8. We'll temporarily go up to 12
due to CompilerWarnings after the change. But I think that's fine, because
we'd previously use the same amount of CPUs, just some of it
sequentially.

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 19 Nov 2022 15:45:06 -0600

On Sat, Nov 19, 2022 at 01:18:54PM -0800, Andres Freund wrote:
> reduce the CPU count for linux autoconf by 1?

I didn't understand the goal of "reducing by one" ?

Up to now, most tasks are using half of the available CPUs, which seemed
deliberate.  Like maybe to allow running two branches simultaneously
(that doesn't necessarily work well with ccache, though).

On Sat, Nov 19, 2022 at 01:35:17PM -0800, Andres Freund wrote:
> The limit for cirrus is 16 linux CPUs though, not 8.

Oh.  Then I don't see any issue.

> We'll temporarily go up to 12 due to CompilerWarnings after the change.

What do you mean "temporarily" ?  I think you're implying that the
Warnings task is fast but (at least right now) it is not.

Note that the most recent "code coverage" task is built into the
linux-autoconf task, and slows it down some more.  That's because it's
the only remaining in-tree build, and I aimed to only show coverage for
changed files (I know you questioned whether that was okay, but to me it
still seems to be valuable, even though it obviously doesn't show
changes outside of those files).  And I couldn't see how to map from
"object filename to source file" with meson, although I guess it's
possible with instrospection.  I haven't re-sent that patch because it's
waiting on cfbot changes.

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Sat, 19 Nov 2022 14:14:17 -0800

Hi,

> Warnings task is fast but (at least right now) it is not.

In the sense that we don't need all CPUs until the whole commit has finished
testing (none of the tasks are the slowest task, even after ubsan/asan). As
soon as one of the linux tests has finished for one commit, another task in a
concurrently tested commit can start.  Whereas that's not the case for macos,
due to the VM limit.

(cfbot has double the limits, because it has a 10$/mo account)

> changes outside of those files).

I think we shouldn't add further tests using autoconf, that'll just mean we'll
have to do the work changing that test at some later point.

> And I couldn't see how to map from

The object files should have that in their metadata, fwiw.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Mon, 21 Nov 2022 14:09:03 -0800

Hi,

> sanitycheck patch and see how that looks.

I just pushed the prerequisite patch making UBSAN_OPTIONS work. Attached
is 1) addition of SanityCheck 2) use of asan and ubsan+alignment san to
the linux tasks.

I went with a variation of the find command for SanityCheck's
cores_script, but used -exec to invoke mv, as that results in a nicer
looking commandline imo.

Previously the SanityCheck patch did trigger warnings about only_if not
matching, despite SanityCheck not having an only_if, but I reported that
as a bug to cirrus-ci, and they fixed that.

Pretty happy with this.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Mon, 21 Nov 2022 14:45:42 -0800

Hi,

> I put it there so it'd be included with the build artifacts.

Wouldn't just naming it build-warnings.log suffice? I don't think we
want to actually upload build.txt - it already is captured.

> Do you think building docs with both autoconf and meson is what's

Not sure.

> mentioned that it shouldn't use ccache -C, and it should use

TBH, I'm not quite sure a separate docs task does really still make
sense after the SanityCheck task. It's worth building the docs even if
some flappy test fails, but I don't think we should try to build the
docs if the code doesn't even compile, in all likelihood a lot more is
wrong in that case.

> https://www.postgresql.org/message-id/20220828171029.GO2342%40telsasoft.com

I suspect this is just about dealing with unnamed tasks and could be
handled by just mixing in CI_NODE_INDEX if the task name isn't set.

I pushed a version of 0007-cirrus-clean-up-windows-task.patch. I didn't
rename the task as I would like to add a msbuild version of the task at
some point (it's pretty easy to break msbuild but not ninja
unfortunately). In additional I also removed NO_TEMP_INSTALL.

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 22 Nov 2022 16:57:44 -0600

> want to actually upload build.txt - it already is captured.

Originally, I wanted the input and the output to be available as files
and not just in cirrus' web GUI, but maybe that's not important anymore.
I rewrote it again.

> docs if the code doesn't even compile, in all likelihood a lot more is

It'd be okay either way.  I had split it out to 1) isolate the changes
in the "upload changed docs as artifacts" patch; and, 2) so the docs
artifacts are visible in a cfbot link called "Documentation"; and, 3) so
the docs task runs without a dependency on "Linux", since (as you said)
docs/errors are worth showing/reviewing/reporting/addressing separately
from test errors (perhaps similar to compiler warnings...).

I shuffled my branch around and sending now the current "docs" patches,
but I suppose this is waiting on the "convert CompilerWarnings task to
meson" patch.

-- 
Justin

---

From: Thomas Munro <[email-masked]>
Date: Fri, 30 Dec 2022 16:59:03 +1300

On Wed, Nov 23, 2022 at 11:57 AM Justin Pryzby <[email-masked]> wrote:
> [PATCH 02/10] cirrus/macos: switch to "macos_instance" / M1..

Duelling patches.

Bilal's patch[1] uses the matrix feature to run the tests on both
Intel and ARM, which made sense when he proposed it, but according to
Cirrus CI warnings, the Intel instances are about to go away.  So I
think we just need your smaller change to switch the instance type.

As for the pathname change, there is another place that knows where
Homebrew lives, in ldap/001_auth.  Fixed in the attached.  That test
just SKIPs if it can't find the binary, making it harder to notice.
Standardising our approach here might make sense for a later patch.
As for the kerberos test, Bilal's patch may well be a better idea (it
adds MacPorts for one thing), and so I'll suggest rebasing that, but
here I just wanted the minimum mechanical fix to avoid breaking on the
1st of Jan.

I plan to push this soon if there are no objections.  Then discussion
of Bilal's patch can continue.

> [PATCH 03/10] cirrus/macos: update to macos ventura

I don't know any reason not to push this one too, but it's not time critical.

[1] https://www.postgresql.org/message-id/flat/CAN55FZ2R%2BXufuVgJ8ew_yDBk48PgXEBvyKNvnNdTTVyczbQj0g%40mail.gmail.com
From a355638fd9c1c16e54418d8374a374967081e0a0 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Wed, 28 Dec 2022 16:58:09 +1300
Subject: [PATCH] ci: Change macOS builds from Intel to ARM.

Cirrus is about to shut down its macOS-on-Intel support, so it's time to
move our CI testing over to ARM instances.  The Homebrew package manager
changed its default installation prefix for the new architecture, so a
couple of tests need tweaks to find binaries.

Author: Justin Pryzby <[email-masked]>
Discussion: https://postgr.es/m/20221122225744.GF11463%40telsasoft.com
---
 .cirrus.yml                     | 8 ++++----
 src/test/kerberos/t/001_auth.pl | 9 ++++++++-
 src/test/ldap/t/001_auth.pl     | 8 +++++++-
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 993af88865..354102613d 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -407,7 +407,7 @@ task:
   name: macOS - Monterey - Meson
 
   env:
-    CPUS: 12 # always get that much for cirrusci macOS instances
+    CPUS: 4 # always get that much for cirrusci macOS instances
     BUILD_JOBS: $CPUS
     # Test performance regresses noticably when using all cores. 8 seems to
     # work OK. See
@@ -428,8 +428,8 @@ task:
   depends_on: SanityCheck
   only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*(macos|darwin|osx).*'
 
-  osx_instance:
-    image: monterey-base
+  macos_instance:
+    image: ghcr.io/cirruslabs/macos-monterey-base:latest
 
   sysinfo_script: |
     id
@@ -475,7 +475,7 @@ task:
   ccache_cache:
     folder: $CCACHE_DIR
   configure_script: |
-    brewpath="/usr/local"
+    brewpath="/opt/homebrew"
     PKG_CONFIG_PATH="${brewpath}/lib/pkgconfig:${PKG_CONFIG_PATH}"
 
     for pkg in icu4c krb5 openldap openssl zstd ; do
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index a2bc8a5351..298dc0c62b 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -32,8 +32,15 @@ elsif ($ENV{PG_TEST_EXTRA} !~ /\bkerberos\b/)
 
 my ($krb5_bin_dir, $krb5_sbin_dir);
 
-if ($^O eq 'darwin')
+if ($^O eq 'darwin' && -d "/opt/homebrew" )
 {
+	# typical paths for Homebrew on ARM
+	$krb5_bin_dir  = '/opt/homebrew/opt/krb5/bin';
+	$krb5_sbin_dir = '/opt/homebrew/opt/krb5/sbin';
+}
+elsif ($^O eq 'darwin')
+{
+	# typical paths for Homebrew on Intel
 	$krb5_bin_dir  = '/usr/local/opt/krb5/bin';
 	$krb5_sbin_dir = '/usr/local/opt/krb5/sbin';
 }
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 0ea274c383..39736e5116 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -21,9 +21,15 @@ elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
 {
 	plan skip_all => 'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
 }
+elsif ($^O eq 'darwin' && -d '/opt/homebrew/opt/openldap')
+{
+	# typical paths for Homebrew on ARM
+	$slapd           = '/opt/homebrew/opt/openldap/libexec/slapd';
+	$ldap_schema_dir = '/opt/homebrew/etc/openldap/schema';
+}
 elsif ($^O eq 'darwin' && -d '/usr/local/opt/openldap')
 {
-	# typical paths for Homebrew
+	# typical paths for Homebrew on Intel
 	$slapd           = '/usr/local/opt/openldap/libexec/slapd';
 	$ldap_schema_dir = '/usr/local/etc/openldap/schema';
 }
-- 
2.35.1

---

From: vignesh C <[email-masked]>
Date: Wed, 17 Jan 2024 17:34:00 +0530

On Wed, 12 Jul 2023 at 11:38, Michael Paquier <[email-masked]> wrote:
>
> On Wed, Jul 12, 2023 at 12:56:17AM -0500, Justin Pryzby wrote:
> > And, since 681d9e462:
> >
> > security is missing from contrib/seg/meson.build
>
> Ugh.  Good catch!

Are we planning to do anything more in this thread, the thread has
been idle for more than 7 months. If nothing more is planned for this,
I'm planning to close this commitfest entry in this commitfest.

Regards,
Vignesh

---

From: Michael Paquier <[email-masked]>
Date: Thu, 18 Jan 2024 10:16:00 +0900

> I'm planning to close this commitfest entry in this commitfest.

Oops, this went through the cracks.  security was still missing in
seg's meson.build, so I've just applied a patch to take care of it.
I am not spotting any other holes..
--
Michael

---

From: vignesh C <[email-masked]>
Date: Wed, 31 Jan 2024 15:10:13 +0530

> I am not spotting any other holes..

Are we planning to do anything more on this? I was not sure if we
should move this to next commitfest or return it.

Regards,
Vignesh

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 31 Jan 2024 11:59:21 +0100

> should move this to next commitfest or return it.

Well, the patches don't apply anymore since .cirrus.tasks.yml has been
created.  However, I'm sure we still want [some of] the improvements
to the tests in [1].  I can volunteer to rebase the patches in time for the
March commitfest, if Justin is not available to do so.  If you can
please move it forward to the March cf and set it WoA, I'd appreciate
that.

Thanks

[1] https://postgr.es/m/ZA/[email-masked]

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)