Adding CI to our tree

On Fri, Oct 1, 2021 at 3:28 PM Andres Freund <andres@anarazel.de> wrote:

For several development efforts I found it to be incredibly valuable to push
changes to a personal repository and see a while later whether tests succeed
on a number of different platforms. This is especially useful for platforms
that are quite different from ones own platform, like e.g. windows in my case.

An obvious criticism of the effort to put CI runner infrastructure into core
is that they are effectively all proprietary technology, and that we should be
hesistant to depend too much on one of them. I think that's a valid
concern. However, once one CI integration is done, a good chunk (but not all!)
the work is transferrable to another CI solution, which I do think reduces the
dependency sufficiently.

I agree with everything you've said, including the nuanced parts about
the possible downsides.

We already know what happens when one of these CI providers stops
providing open source projects with free resources, because that's
exactly what happened with Travis CI: projects that use their
infrastructure are mildly inconvenienced. I can't see any real notable
downside, as long as we just use the resources that they make
available for development work. Clearly these services could never
replace the buildfarm.

--
Peter Geoghegan

Hi,

On 2021-10-02 01:49:39 +0200, 0010203112132233 wrote:

On Sat, 2 Oct 2021 at 00:28, Andres Freund <andres@anarazel.de> wrote:

New contributors (and quite a bit of older ones too) IMO expect to be able to
see whether their changes work as-is, without sending a patch to the list.

Have they checked 'make installcheck-world'? I believe that is one of
the first action items on the 'So, you want to become a developer?'
wiki page, after downloading the sources. Of course, that is limited
to only the environment of the user, but that's what they're generally
developing for, right?

If you want to get a change into postgres, it almost always needs to actually
work on all operating systems, and always needs to at least not cause build
failures on all platforms.

Furthermore, after looking it through, I think that Cirrus is an
unfortunate choice as a CI platform of preference, as you cannot use
it without access to Github (which is problematic for people located
in certain localities due to USA regulations).

I agree that it's not optimal that cirrus isn't available on all git hosting
platforms. Hence saying that I think it's likely we'd end up adding a few more
platforms over time. If we factor the meat of the work into an helper script,
so that the CI specific bit is just a couple invocation of that script, it's
not a lot of overhead to have 2-3 CI platforms.

If we're going to include CI configuration for private use, I'd prefer if it
were a CI that can be enjoyed in private without pushing code to a 3rd
party.

FWIW, you can use cirrus locally on your machine:
https://github.com/cirruslabs/cirrus-cli

It'll not be able to run all kinds of tasks though (e.g. no windows docker on
a linux host, dealing with the license costs for that presumably would be
nontrivial).

Lastly, I consider CI configuration similar to IDE configuration: each
developer has their own preferred tools which they use, but we don't
favour one over the other. We don't include IDE-specific configuration
files either, or at least, the policy is against that.

So, I greatly appreciate the effort, but I don't think this is
something that should be committed into core. Maybe as a dedicated
wiki page detailing configurations for CI, similar to the Buildfarm
page?

That doesn't scale - I've actually added CI to all my substantial development
work in my private branches, and it's pretty annoying to need to do so every
time. And there's many developers who won't go through the effort most of the
time.

It's not like this forces you to use cirrus or anything. For people that don't
want to use CI, It'll make cfbot a bit more effective (because people can
adjust what it tests as appropriate for $patch), but that's it.

Greetings,

Andres Freund

On Sat, Oct 2, 2021 at 1:10 PM Andres Freund <andres@anarazel.de> wrote:

On 2021-10-02 01:49:39 +0200, 0010203112132233 wrote:

Furthermore, after looking it through, I think that Cirrus is an
unfortunate choice as a CI platform of preference, as you cannot use
it without access to Github (which is problematic for people located
in certain localities due to USA regulations).

I agree that it's not optimal that cirrus isn't available on all git hosting
platforms. Hence saying that I think it's likely we'd end up adding a few more
platforms over time. If we factor the meat of the work into an helper script,
so that the CI specific bit is just a couple invocation of that script, it's
not a lot of overhead to have 2-3 CI platforms.

BTW I think they might be considering supporting other code hosting
platforms (at least they ask for feedback on this at
https://cirrus-ci.org/guide/quick-start/ ).

Lastly, I consider CI configuration similar to IDE configuration: each
developer has their own preferred tools which they use, but we don't
favour one over the other. We don't include IDE-specific configuration
files either, or at least, the policy is against that.

We have some files in the tree to help users of Emacs, vim, and even
make github format text the way we like.

Personally, I think that if someone is willing to develop and maintain
high quality CI control files that work for any public
free-for-open-source CI system, then we should accept them too. It
costs very little to have a few .something.yml files at top level. If
at any point the file for a given provider is showing signs of being
unmaintained, we can remove it. Personally, I'm willing and able to
help maintain Cirrus control files, not least because it means that
cfbot will become simpler and will match exactly what you can get in
your own github account.

I really like Cirrus because our project has more portability concerns
than most, and most other CIs are like "we got both kinds, country and
western!". I wanted to add FreeBSD to cfbot, which is something they
advertise as a feature, but it looks like at least 3 other OSes we
target would probably work just as well given a suitable image.

Andres Freund <andres@anarazel.de> writes:

It's not like this forces you to use cirrus or anything. For people that don't
want to use CI, It'll make cfbot a bit more effective (because people can
adjust what it tests as appropriate for $patch), but that's it.

Yeah. I cannot see any reason to object to Andres' 0002 patch: you can
just ignore those files if you don't want to use cirrus. It does set a
precedent that we'd also accept infrastructure for other CI systems,
but as long as they're similarly noninvasive, why not? (Maybe there
needs to be one more directory level though, ie ci/cirrus/whatever.
I don't want to end up with one toplevel directory per CI platform.)

I don't know enough about Windows to evaluate 0001, but I'm a little
worried about it because it looks like it's changing our *production*
error handling on that platform.

As for 0003, wasn't that committed already?

regards, tom lane

On 2 Oct 2021, at 00:27, Andres Freund <andres@anarazel.de> wrote:

For several development efforts I found it to be incredibly valuable to push
changes to a personal repository and see a while later whether tests succeed
on a number of different platforms. This is especially useful for platforms
that are quite different from ones own platform, like e.g. windows in my case.

Same, and for my case I run several CI jobs to compile/test against different
OpenSSL versions etc.

Of course everybody can set this up for themselves. However, doing so well is
a significant effort, particularly if windows is to be supported well. And
doubly so if useful things like getting backtraces for crashes is desirable
([1])

+1 on adding these, rather than having everyone duplicate the effort. Those
who don't want to use them can disregard them.

Right now the patch attached
- runs check-world on FreeBSD, Linux, macOS - all using gcc
- freebsd, linux use a custom generated image
- macOS installs missing dependencies at runtime, with some caching
- all use ccache to make subsequent compilation faster
- runs all the tests I could find on windows, via vcregress.pl
- checks for compiler warnings on linux, with both clang and gcc

Why not compiling with OpenSSL on FreeBSD and macOS? On FreeBSD all you need
is --with-ssl=openssl while on macOS you need to point to the headers and libs
like:

--with-includes=/usr/local/include:/usr/local/opt/openssl/include --with-libs=/usr/local/libs:/usr/local/opt/openssl/lib

One thing to note for Cirrus on macOS (I've never seen it anywhere else) is
that it intermittently will fail on a too long socketpath:

Unix-domain socket path "/private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/cirrus-ci-build/src/bin/pg_upgrade/.s.PGSQL.51696" is too long (maximum 103 bytes)

Exporting PGSOCKETDIR can avoid that annoyance.

+  tests_script:
+    - su postgres -c 'ulimit -c unlimited ; ${TIMEOUT_CMD} make -s ${CHECK} ${CHECKFLAGS} -j8'
Don't you need PG_TEST_EXTRA=ssl here to ensure the src/test/ssl tests are run?

--
Daniel Gustafsson https://vmware.com/

Hi,

On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote:

On 2 Oct 2021, at 00:27, Andres Freund <andres@anarazel.de> wrote:
Right now the patch attached
- runs check-world on FreeBSD, Linux, macOS - all using gcc
- freebsd, linux use a custom generated image
- macOS installs missing dependencies at runtime, with some caching
- all use ccache to make subsequent compilation faster
- runs all the tests I could find on windows, via vcregress.pl
- checks for compiler warnings on linux, with both clang and gcc

Why not compiling with OpenSSL on FreeBSD and macOS? On FreeBSD all you need
is --with-ssl=openssl while on macOS you need to point to the headers and libs
like:

--with-includes=/usr/local/include:/usr/local/opt/openssl/include --with-libs=/usr/local/libs:/usr/local/opt/openssl/lib

Yea, there's several things like that, that should be added. The CI files
originated from development where breakage around SSL wasn't likely (AIO,
shared memory stats, procarray scalability etc), so I didn't focussed on that
angle.

Needing to get all that stuff right on multiple platforms is one of the
reasons why I think having this thing in-tree would be good. No need for
everyone to discover the magic incantations themselves. Even if you e.g. might
want to extend them to test multiple SSL versions or such, it's a lot easier
to do that if the basics are there.

One thing to note for Cirrus on macOS (I've never seen it anywhere else) is
that it intermittently will fail on a too long socketpath:

I've seen it somewhere else before. It wasn't even intermittent - it always
failed. I worked around that by setting CIRRUS_WORKING_DIR: ${HOME}/pgsql/ -
also made output including filenames easier to read ;)

+  tests_script:
+    - su postgres -c 'ulimit -c unlimited ; ${TIMEOUT_CMD} make -s ${CHECK} ${CHECKFLAGS} -j8'
Don't you need PG_TEST_EXTRA=ssl here to ensure the src/test/ssl tests are run?

Probably. I quickly added that stuff, we'll see how many mistakes I made:
https://cirrus-ci.com/build/5846034501861376

Greetings,

Andres Freund

Hi,

On 2021-10-02 12:41:07 -0700, Andres Freund wrote:

On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote:

+  tests_script:
+    - su postgres -c 'ulimit -c unlimited ; ${TIMEOUT_CMD} make -s ${CHECK} ${CHECKFLAGS} -j8'
Don't you need PG_TEST_EXTRA=ssl here to ensure the src/test/ssl tests are run?

Probably. I quickly added that stuff, we'll see how many mistakes I made:
https://cirrus-ci.com/build/5846034501861376

I wonder if we shouldn't stop skipping the ssl / kerberos / ldap (and perhaps
others) tests in the makefile, and instead do so in the tap tests
themselves. Then one can see them included as the skipped in the tap result
output, which seems like it'd make it easier to discover them?

Greetings,

Andres Freund

On 2 Oct 2021, at 21:45, Andres Freund <andres@anarazel.de> wrote:

I wonder if we shouldn't stop skipping the ssl / kerberos / ldap (and perhaps
others) tests in the makefile, and instead do so in the tap tests
themselves. Then one can see them included as the skipped in the tap result
output, which seems like it'd make it easier to discover them?

I am definitely in favor of doing that, better to see them skipped rather than
having to remember to opt in. We even do so already to some extent already,
like for example the SSL tests:

if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}

--
Daniel Gustafsson https://vmware.com/

On 2 Oct 2021, at 21:41, Andres Freund <andres@anarazel.de> wrote:

One thing to note for Cirrus on macOS (I've never seen it anywhere else) is
that it intermittently will fail on a too long socketpath:

I've seen it somewhere else before. It wasn't even intermittent - it always
failed. I worked around that by setting CIRRUS_WORKING_DIR: ${HOME}/pgsql/ -
also made output including filenames easier to read ;)

Aha, nice trick! Didn't know about that one but that's easier than setting
specific dirs via PG* environment vars.

--
Daniel Gustafsson https://vmware.com/

Hi,

On 2021-10-02 11:05:20 -0400, Tom Lane wrote:

Yeah. I cannot see any reason to object to Andres' 0002 patch: you can
just ignore those files if you don't want to use cirrus. It does set a
precedent that we'd also accept infrastructure for other CI systems,
but as long as they're similarly noninvasive, why not?

Exactly.

(Maybe there needs to be one more directory level though, ie
ci/cirrus/whatever. I don't want to end up with one toplevel directory per
CI platform.)

Good question - it definitely shouldn't be one toplevel directory per CI
platform (although some will require their own hidden toplevel directories,
like .github/workflows etc). I'd hope to share a bunch of the infrastructure
between them over time, so perhaps we don't need a deeper hierarchy.

I don't know enough about Windows to evaluate 0001, but I'm a little
worried about it because it looks like it's changing our *production*
error handling on that platform.

Yea. It's clearly not ready as-is - it's the piece that I was planning to
write a separate email about.

It's hard to understand what *precisely* SEM_NOGPFAULTERRORBOX etc do.

What I do know is that without the _set_abort_behavior() stuff abort() doesn't
trigger windows' "crash" paths in at least debugging builds, and that the
SetErrorMode() and _CrtSetReportMode() changes are necessary to get segfaults
to reach the crash paths.

The in-tree behaviour turns out to make debugging on windows a major pain, at
least when compiling with msvc. Crashes never trigger core dumps or "just in
time" debugging (their term for invoking a debugger upon crash), so one has to
attach to processes before they crash, to have any chance of debugging.

As far as I can tell this also means that at least for debugging builds,
pgwin32_install_crashdump_handler() is pretty much dead weight -
crashDumpHandler() never gets invoked. I think it may get invoked for abort()s
in production builds, but probably not for segfaults.

And despite SEM_NOGPFAULTERRORBOX we display those annoying "popup" boxes
telling us about the crash and giving the option to retry, ignore, something
something. It's all a bit baffling.

As for 0003, wasn't that committed already?

Not at the time I was writing the email, but now it is, yes.

Greetings,

Andres Freund

Hi,

On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote:

Same, and for my case I run several CI jobs to compile/test against different
OpenSSL versions etc.

On that note: Did you do this for windows? If so, I'd rather not figure that
out myself...

Greetings,

Andres Freund

On 2 Oct 2021, at 22:01, Andres Freund <andres@anarazel.de> wrote:
On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote:

Same, and for my case I run several CI jobs to compile/test against different
OpenSSL versions etc.

On that note: Did you do this for windows? If so, I'd rather not figure that
out myself...

Not with Cirrus, I've been using Appveyor for Windows and they provide 1.0.2 -
3.0.0 which can easily set in config.pl with for example:

$config->{openssl} = 'C:\OpenSSL-v111-Win64';

--
Daniel Gustafsson https://vmware.com/

On 10/2/21 11:05 AM, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

It's not like this forces you to use cirrus or anything. For people that don't
want to use CI, It'll make cfbot a bit more effective (because people can
adjust what it tests as appropriate for $patch), but that's it.

Yeah. I cannot see any reason to object to Andres' 0002 patch: you can
just ignore those files if you don't want to use cirrus.

Yeah. I enable cirrus selectively on my github repos, which makes it 
close to impossible to get an unwanted effect.

One of the things I like about this is that it institutionalizes some
knowledge that has hitherto been mostly private. I have a lot of this in
a setup I use for spinning up test instances, but this makes a lot of
that sort of knowledge more broadly available.

I hope it will also encourage people to test more widely, given how easy
it will make it.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

I hope it will also encourage people to test more widely, given how easy
it will make it.

If you'd like that, there would need to be some (ahem) documentation
of how to use it.

regards, tom lane

Hi,

On 2021-10-02 16:44:44 -0400, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

I hope it will also encourage people to test more widely, given how easy
it will make it.

If you'd like that, there would need to be some (ahem) documentation
of how to use it.

Yea, definitely necessary. Where would we want it to be? ci/README.md? That'd
be viewable on the various git hosting platforms. I guess there's an argument
for it to be in the sgml docs, but that doesn't seem all that useful in this
case.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2021-10-02 16:44:44 -0400, Tom Lane wrote:

If you'd like that, there would need to be some (ahem) documentation
of how to use it.

Yea, definitely necessary. Where would we want it to be? ci/README.md? That'd
be viewable on the various git hosting platforms. I guess there's an argument
for it to be in the sgml docs, but that doesn't seem all that useful in this
case.

A README seems plenty good enough to me. Maybe -0.1 for making
it .md rather than plain text ... plain text is our habit everywhere
else AFAIR.

regards, tom lane

Hi,

On October 2, 2021 1:18:38 PM PDT, Daniel Gustafsson <daniel@yesql.se> wrote:

On 2 Oct 2021, at 22:01, Andres Freund <andres@anarazel.de> wrote:
On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote:

Same, and for my case I run several CI jobs to compile/test against different
OpenSSL versions etc.

On that note: Did you do this for windows? If so, I'd rather not figure that
out myself...

Not with Cirrus, I've been using Appveyor for Windows and they provide 1.0.2 -
3.0.0 which can easily set in config.pl with for example:

$config->{openssl} = 'C:\OpenSSL-v111-Win64';

Got the build part working (although the state of msvc compatible openssl distribution on windows seems a bit scary). However the ssl tests don't fully succeed:

https://cirrus-ci.com/task/6264790323560448?logs=ssl#L655

I didn't see code in the bf client code running the test so perhaps that's not too surprising :/

Did you run those tests on windows?

Regards,

Andres

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Hi,

On 2021-10-02 21:05:17 -0700, Andres Freund wrote:

Got the build part working (although the state of msvc compatible openssl
distribution on windows seems a bit scary). However the ssl tests don't
fully succeed:

https://cirrus-ci.com/task/6264790323560448?logs=ssl#L655

I didn't see code in the bf client code running the test so perhaps that's
not too surprising :/

Did you run those tests on windows?

As you can see in the test output, every mismatch prints the whole file,
despite only intending to show the tail. Which appears to be because the
windows portion of 3c5b0685b921 doesn't actually work. The reason for that in
turn is that afaict the setFilePointer doesn't change the file position in a
way that affects perl.

Consequently, if I force the !win32 path, the tests pass.

At first I assumed the cause of this is that while the setFilePointer() modifies the
state of the underlying handle, it doesn't actually let perl know about
that. Due to buffering etc perl likely has its own bookeeping about the
position in the file. There's some pretty clear hints in
https://perldoc.perl.org/functions/seek

But the problem turns out to be that it's bogus to pass $fh to
setFilePointer(). That's a perl handle, not an win32 handle. Fixing that seems
to make the tests pass.

Why did 3c5b0685b921 choose to use setFilePointer() in the first place? At
this point it's a perl filehandle, so we should just use perl seek?

Leaving the concrete breakage aside, I'm somewhat unhappy that there's not a
single comment explaining why TestLib.pm is trying to use native windows
APIs.

Isn't the code as-is also "leaking" an open IO::Handle? There's a
CloseHandle($fHandle), but nothing is done to $fh. But perhaps there's some
perl magic cleaning things up? Even if so, loks like just closing $fh will
close the handle as well...

Greetings,

Andres Freund

Hi,

On 2021-10-03 10:18:31 -0700, Andres Freund wrote:

As you can see in the test output, every mismatch prints the whole file,
despite only intending to show the tail. Which appears to be because the
windows portion of 3c5b0685b921 doesn't actually work. The reason for that in
turn is that afaict the setFilePointer doesn't change the file position in a
way that affects perl.

Consequently, if I force the !win32 path, the tests pass.

At first I assumed the cause of this is that while the setFilePointer() modifies the
state of the underlying handle, it doesn't actually let perl know about
that. Due to buffering etc perl likely has its own bookeeping about the
position in the file. There's some pretty clear hints in
https://perldoc.perl.org/functions/seek

But the problem turns out to be that it's bogus to pass $fh to
setFilePointer(). That's a perl handle, not an win32 handle. Fixing that seems
to make the tests pass.

It does (I only let it run to the ssl test, then pushed a newer revision):
https://cirrus-ci.com/task/5345293928497152?logs=ssl#L5

Why did 3c5b0685b921 choose to use setFilePointer() in the first place? At
this point it's a perl filehandle, so we should just use perl seek?

Leaving the concrete breakage aside, I'm somewhat unhappy that there's not a
single comment explaining why TestLib.pm is trying to use native windows
APIs.

Isn't the code as-is also "leaking" an open IO::Handle? There's a
CloseHandle($fHandle), but nothing is done to $fh. But perhaps there's some
perl magic cleaning things up? Even if so, loks like just closing $fh will
close the handle as well...

I think something roughly like the attached might be a good idea. Runs locally
on linux, and hopefully still on windows

https://cirrus-ci.com/build/4857291573821440

Greetings,

Andres Freund