Subject: Re: Test to dump and restore objects left behind by regression

From: Andres Freund <[email-masked]>
Date: Thu, 3 Apr 2025 09:40:49 -0400

Hi,

On 2025-04-03 10:20:09 +0200, Alvaro Herrera wrote:
> On 2025-Apr-03, Ashutosh Bapat wrote:
>
> > Looks like the problem is in the test itself as pointed out by Jeff in
> > [1]. PFA patch fixing the test and enabling statistics back.
>
> Thanks, pushed.

Since then the pg_upgrade tests have been failing on skink/valgrind, due to
exceeding the already substantially increased timeout.

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=skink&dt=2025-04-03%2007%3A06%3A19&stg=pg_upgrade-check
(note that there are other issues in that run)

284/333 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade                               TIMEOUT        10000.66s   killed by signal 15 SIGTERM

[10:38:19.815](16.712s) ok 20 - check that locales in new cluster match original cluster
...
# Running: pg_dumpall --no-sync --dbname port=15114 host=/tmp/bh_AdT5uvQ dbname='postgres' --file /home/bf/bf-build/skink-master/HEAD/pgsql.build/testrun/pg_upgrade/002_pg_upgrade/data/tmp_test_gp2G/dump2.sql
death by signal at /home/bf/bf-build/skink-master/HEAD/pgsql/src/test/perl/PostgreSQL/Test/Cluster.pm line 181.
...
[10:44:11.720](351.905s) # Tests were run but no plan was declared and done_testing() was not seen.

I've increased the timeout even further, but I can't say that I am happy about
the slowest test getting even slower. Adding test time in the serially slowest
test is way worse than adding the same time in a concurrent test.

I suspect that the test will go a bit faster if log_statement weren't forced
on, printing that many log lines, with context, does make valgrind slower,
IME. But Cluster.pm forces it to on, and I suspect that putting a global
log_statement=false into TEMP_CONFIG would have it's own disadvantages.

/me and checks prices for increasing the size of skink's host.

Greetings,

Andres

---

From: Andres Freund <[email-masked]>
Date: Fri, 4 Apr 2025 12:07:08 -0400

Hi,

On 2025-04-04 12:01:16 -0400, Andres Freund wrote:
> FWIW, for me 027 is actually considerably faster. In an cassert -O0 build (my
> normal development env, I find even -Og too problematic for debugging):
> 
> pg_upgrade/002_pg_upgrade    96.61s
> recovery/027_stream_regress  66.04s
> 
> After
>   git revert 8806e4e8deb1e21715e031e17181d904825a410e abe56227b2e213755dd3e194c530f5f06467bd7c 172259afb563d35001410dc6daad78b250924038
> 
> pg_upgrade/002_pg_upgrade    75.09s
> 
> Slower by 29%, far from the 3s increased time I saw mentioned somewhere.
> 
> 
> And this really affects the overall test time:
> 
> All tests before:
> 	real	1m38.173s
> 	user	5m52.500s
> 	sys	4m23.574s
> 
> All tests after:
> 	real	2m0.397s
> 	user	5m53.625s
> 	sys	4m30.518s
> 
> The CPU time increase is rather minimal, but the wall clock time increase is
> 22%.
> 
> 17:
> 	real	1m14.822s
> 	user	4m2.630s
> 	sys	3m22.384s
> 
> We regressed wall clock time *60%* from 17->18. Some test cycle increase is
> reasonable and can largely be compensated with hardware, but this cycle we're
> growing way faster than hardware gets faster.  I don't think that's
> sustainable.

FWIW, with cassert and -O2, it's:

17:
	real	0m53.981s
	user	3m22.837s
	sys	3m24.237s

HEAD:
	real	1m19.749s
	user	4m54.526s
	sys	4m15.657s

so this isn't just due to me using -O0. A 48% increase is better than a 60%
increase, but it's still not sustainable.

Greetings,

Andres Freund

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 5 Aug 2025 16:33:38 +0200

Hello,

> increase, but it's still not sustainable.

I happened to notice that this item was still open in the commitfest,
and rereading the thread I now have second thoughts about having it
enabled by default, giving your complaints about speed.  How about
applying this to 18 and master?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"This is a foot just waiting to be shot"                (Andrew Dunstan)

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 5 Aug 2025 16:41:03 +0200

> On 5 Aug 2025, at 16:33, Alvaro Herrera <[email-masked]> wrote:

> applying this to 18 and master?

Thanks for reviving this.  I am +1 on placing this behind PG_TEST_EXTRA as was
discussed upthread.

--
Daniel Gustafsson

---

From: Tom Lane <[email-masked]>
Date: Tue, 05 Aug 2025 10:59:01 -0400

> Thanks for reviving this.  I am +1 on placing this behind PG_TEST_EXTRA as was

+1 here too.

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 5 Aug 2025 20:11:41 +0200

> > Thanks for reviving this.  I am +1 on placing this behind PG_TEST_EXTRA as was

Cool, thanks, done.  Now we need a volunteer to set up a buildfarm
animal with this flag ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)

---

From: Michael Paquier <[email-masked]>
Date: Wed, 6 Aug 2025 10:22:02 +0900

> animal with this flag ...

Sure.  I have added regress_dump_restore to the configuration of
batta, hachi and gokiburi.
--
Michael

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 6 Aug 2025 20:40:00 +0530

> animal with this flag ...

Your patch didn't contain doc changes. But the commit has it. Thanks.

-- 
Best Wishes,
Ashutosh Bapat