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

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 30 Aug 2023 10:57:10 +0200

> On 28 Aug 2023, at 14:32, 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.

The attached v2 fixes a silly mistake which led to a compiler warning.

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Tue, 12 Sep 2023 16:49:01 -0700

Hi,

> The attached v2 fixes a silly mistake which led to a compiler warning.

Still seems like a good idea to me. To see what impact it has, I measured the
time running the pg_regress tests that take less than 6s on my machine - I
excluded the slower ones (like the main regression tests) because they'd hide
any overall difference.

ninja && m test --suite setup --no-rebuild && tests=$(m test --no-rebuild --list|grep -E '/regress'|grep -vE '(regress|postgres_fdw|test_integerset|intarray|amcheck|test_decoding)/regress'|cut -d' ' -f 3) && time m test --no-rebuild $tests

Time for:

master:

cassert:
real	0m5.265s
user	0m8.422s
sys	0m8.381s

optimized:
real	0m4.926s
user	0m6.356s
sys	0m8.263s

my patch (probing every 100ms with psql):

cassert:
real	0m3.465s
user	0m8.827s
sys	0m8.579s

optimized:
real	0m2.932s
user	0m6.596s
sys	0m8.458s

Daniel's (probing every 50ms with PQping()):

cassert:
real	0m3.347s
user	0m8.373s
sys	0m8.354s

optimized:
real	0m2.527s
user	0m6.156s
sys	0m8.315s

My patch increased user/sys time a bit (likely due to a higher number of
futile psql forks), but Daniel's doesn't. And it does show a nice overall wall
clock time saving.

Greetings,

Andres Freund

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 18 Sep 2023 15:12:49 +0200

> On 13 Sep 2023, at 01:49, Andres Freund <[email-masked]> wrote:

> futile psql forks), but Daniel's doesn't. And it does show a nice overall wall

While it does add a lib dependency I think it's worth doing, so I propose we go
ahead with this for master.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 24 Oct 2023 22:25:54 +0200

> futile psql forks), but Daniel's doesn't. And it does show a nice overall wall

I went ahead and applied this on master, thanks for review!  Now to see if
there will be any noticeable difference in resource usage.

--
Daniel Gustafsson

---

From: Tom Lane <[email-masked]>
Date: Tue, 24 Oct 2023 16:34:54 -0400

> there will be any noticeable difference in resource usage.

I think that tools like Coverity are likely to whine about your
use of sprintf instead of snprintf.  Sure, it's perfectly safe,
but that won't stop the no-sprintf-ever crowd from complaining.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 24 Oct 2023 22:45:48 +0200

> On 24 Oct 2023, at 22:34, Tom Lane <[email-masked]> wrote:
> 

Fair point, that's probably quite likely to happen.  I can apply an snprintf()
conversion change like this in the two places introduced by this:

-        sprintf(s, "%d", port);
+        sprintf(s, sizeof(s), "%d", port);

--
Daniel Gustafsson