Subject: Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc?

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 23 Aug 2023 14:48:26 +0200

> On 23 Aug 2023, at 08:58, Andres Freund <[email-masked]> wrote:

> I'm hoping to push this fairly soon, as I'll be on vacation the last week of
> August. I'll be online intermittently though, if there are issues, I can react
> (very limited connectivity for middday Aug 29th - midday Aug 31th though). I'd
> appreciate a quick review or two.

I've been reading over these and the thread, and while not within my area of
expertise, nothing really sticks out.

I'll do another pass, but below are a few small comments so far.

I don't know Windows to know the implications, but should the below file have
some sort of warning about not doing that for production/shared systems, only
for dedicated test instances?

+++ b/src/tools/ci/windows_write_cache.ps1
@@ -0,0 +1,20 @@
+# Define the write cache to be power protected. This reduces the rate of cache
+# flushes, which seems to help metadata heavy workloads on NTFS. We're just
+# testing here anyway, so ...
+#
+# Let's do so for all disks, this could be useful beyond cirrus-ci.

One thing in 0010 caught my eye, and while not introduced in this patchset it
might be of interest here.  In the below hunks we loop X ticks around
system(psql), with the loop assuming the server can come up really quickly and
sleeping if it doesn't.  On my systems I always reach the pg_usleep after
failing the check, but if I reverse the check such it first sleeps and then
checks I only need to check once instead of twice.

@@ -2499,7 +2502,7 @@ regression_main(int argc, char *argv[],
 		else
 			wait_seconds = 60;
 
-		for (i = 0; i < wait_seconds; i++)
+		for (i = 0; i < wait_seconds * WAITS_PER_SEC; i++)
 		{
 			/* Done if psql succeeds */
 			fflush(NULL);
@@ -2519,7 +2522,7 @@ regression_main(int argc, char *argv[],
 					 outputdir);
 			}
 
-			pg_usleep(1000000L);
+			pg_usleep(1000000L / WAITS_PER_SEC);
 		}
 		if (i >= wait_seconds)
 		{

It's a micro-optimization, but if we're changing things here to chase cycles it
might perhaps be worth doing?

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Wed, 23 Aug 2023 12:22:39 -0700

Hi,

> expertise, nothing really sticks out.

Thanks!

> for dedicated test instances?

Ah, I should have explained that: I'm not planning to apply
- regress: Check for postgres startup completion more often
- ci: windows: Disabling write cache flushing during test
right now. Compared to the other patches the wins are much smaller and/or more
work is needed to make them good.

I think it might be worth going for
- ci: switch tasks to debugoptimized build
because that provides a fair bit of gain. But it might be more hurtful than
helpful due to costing more when ccache doesn't work...

> +++ b/src/tools/ci/windows_write_cache.ps1

I think there's more effective ways to make this cheaper. The basic thing
would be to use libpq instead of forking of psql to make a connection
check. Medium term, I think we should invent a way for pg_ctl and other
tooling (including pg_regress) to wait for the service to come up. E.g. having
a named pipe that postmaster opens once the server is up, which should allow
multiple clients to use select/epoll/... to wait for it without looping.

ISTM making pg_regress use libpq w/ PQping() should be a pretty simple patch?
The non-polling approach obviously is even better, but also requires more
thought (and documentation and ...).

> might perhaps be worth doing?

I wouldn't quite call not waiting for 1s for the server to start, when it does
so within a few ms, chasing cycles ;). For short tests it's a substantial
fraction of the overall runtime...

Greetings,

Andres Freund

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 23 Aug 2023 22:11:09 +0200

> On 23 Aug 2023, at 21:22, Andres Freund <[email-masked]> wrote:

> helpful due to costing more when ccache doesn't work...

Gotcha.

> check.

I had it in my head that not using libpq in pg_regress was a deliberate choice,
but I fail to find a reference to it in the archives.

> fraction of the overall runtime...

Absolutely, I was referring to shifting the sleep before the test to avoid the
extra test, not the reduction of the pg_usleep.  Reducing the sleep is a clear
win.

--
Daniel Gustafsson

---

From: Tom Lane <[email-masked]>
Date: Wed, 23 Aug 2023 17:02:51 -0400

> but I fail to find a reference to it in the archives.

I have a vague feeling that you are right about that.  Perhaps the
concern was that under "make installcheck", pg_regress might be
using a build-tree copy of libpq rather than the one from the
system under test.  As long as we're just trying to ping the server,
that shouldn't matter too much I think ... unless we hit problems
with, say, a different default port number or socket path compiled into
one copy vs. the other?  That seems like it's probably a "so don't
do that" case, though.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 23 Aug 2023 23:12:50 +0200

> On 23 Aug 2023, at 23:02, Tom Lane <[email-masked]> wrote:
> 

Ah yes, that does ring a familiar bell.  I agree that using it for pinging the
server should be safe either way, but we should document the use-with-caution
in pg_regress.c if/when we go down that path.  I'll take a stab at changing the
psql retry loop for pinging tomorrow to see what it would look like.

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Wed, 23 Aug 2023 14:43:41 -0700

Hi,

On 2023-08-23 17:02:51 -0400, Tom Lane wrote:
> that shouldn't matter too much I think

Or perhaps the opposite? That an installcheck pg_regress run might use the
system libpq, which doesn't have the symbols, or such?

Either way, with a function like PQping(), which existing in well beyond the
supported branches, that shouldn't be an issue?

> ... unless we hit problems with, say, a different default port number or
> socket path compiled into one copy vs. the other?  That seems like it's
> probably a "so don't do that" case, though.

If we were to find such a case, it seems we could just add whatever missing
parameter to the connection string? I think we would likely already hit such
problems though, the psql started by an installcheck pg_regress might use the
system libpq, I think?

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Wed, 23 Aug 2023 17:55:53 -0400

> problems though, the psql started by an installcheck pg_regress might use the

The trouble with that approach is that in "make installcheck", we
don't really want to assume we know what the installed libpq's default
connection parameters are.  So we don't explicitly know where that
libpq will connect.

As I said, we might be able to start treating installed-libpq-not-
compatible-with-build as a "don't do it" case.  Another idea is to try
to ensure that pg_regress uses the same libpq that the psql-under-test
does; but I'm not sure how to implement that.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 23 Aug 2023 15:06:12 -0700

Hi,

> connection parameters are.  So we don't explicitly know where that

Stepping back: I don't think installcheck matters for the concrete use of
libpq we're discussing - the only time we wait for server startup is the
non-installcheck case.

There are other potential uses for libpq in pg_regress though - I'd e.g. like
to have a "monitoring" session open, which we could use to detect that the
server crashed (by waiting for the FD to be become invalid). Where the
connection default issue could matter more?

I was wondering if we could create an unambiguous connection info, but that
seems like it'd be hard to do, without creating cross version hazards.

> does; but I'm not sure how to implement that.

I don't think that's likely to work, psql could use a libpq with a different
soversion. We could dlopen() libpq, etc, but that seems way too complicated.

What's the reason we don't force psql to come from the same build as
pg_regress?

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Wed, 23 Aug 2023 18:32:26 -0400

> libpq we're discussing - the only time we wait for server startup is the

Oh, that's an excellent point.  So for the immediately proposed use-case,
there's no issue.  (We don't have a mode where we try to start a server
using already-installed executables.)

> connection default issue could matter more?

Meh.  I don't find that idea compelling enough to justify adding
restrictions on what test scenarios will work.  It's seldom hard to
tell from the test output whether the server crashed.

> seems like it'd be hard to do, without creating cross version hazards.

Hmm, we don't expect the regression test suite to work against other
server versions, so maybe that could be made to work --- that is, we
could run the psql under test and get a full set of connection
parameters out of it?  But I'm still not finding this worth the
trouble.

> What's the reason we don't force psql to come from the same build as

Because the point of installcheck is to check the installed binaries
--- including the installed psql and libpq.

(Thinks for a bit...)  Maybe we should add pg_regress to the installed
fileset, and use that copy not the in-tree copy for installcheck?
Then we could assume it's using the same libpq as psql.  IIRC there
have already been suggestions to do that for the benefit of PGXS
testing.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 23 Aug 2023 15:56:30 -0700

Hi,

> tell from the test output whether the server crashed.

I find it pretty painful to wade through a several-megabyte regression.diffs
to find the cause of a crash. I think we ought to use
restart_after_crash=false, since after a crash there's no hope for the tests
to succeed, but even in that case, we end up with a lot of pointless contents
in regression.diffs. If we instead realized that we shouldn't start further
tests, we'd limit that by a fair bit.

Greetings,

Andres Freund

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 24 Aug 2023 08:10:14 +0200

> tests, we'd limit that by a fair bit.

I once coded it up so that if the server crashes during a test, it would 
wait until it recovers before running the next test.  I found that 
useful.  I agree the current behavior is not useful in any case.

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 28 Aug 2023 14:32:24 +0200

> On 23 Aug 2023, at 23:12, Daniel Gustafsson <[email-masked]> wrote:
> 

Attached is a patch with a quick PoC for using PQPing instead of using psql for
connection checks in pg_regress.  In order to see performance it also includes
a diag output for "Time to first test" which contains all setup costs.  This
might not make it into a commit but it was quite helpful in hacking so I left
it in for now.

The patch incorporates Andres' idea for finer granularity of checks by checking
TICKS times per second rather than once per second, it also shifts the
pg_usleep around to require just one ping in most cases compard to two today.

On my relatively tired laptop this speeds up pg_regress setup with 100+ms with
much bigger wins on Windows in the CI.  While it does add a dependency on
libpq, I think it's a fairly decent price to pay for running tests faster.

--
Daniel Gustafsson