Re: initdb caching during tests
> On 7 Dec 2023, at 15:27, Matthias van de Meent > wrote: > Then that'd be the attached patch, which also includes --auth instead > of -A, for the same reason as -N vs --no-sync Applied to master, thanks! -- Daniel Gustafsson
Re: initdb caching during tests
On Thu, 7 Dec 2023 at 15:06, Daniel Gustafsson wrote: > > > On 7 Dec 2023, at 14:50, Matthias van de Meent > > wrote: > > > Attached a patch that fixes this for both make and meson, by adding > > --no-clean to the initdb template. > > Makes sense. While in there I think we should rename -N to the long optoin > --no-sync to make it easier to grep for and make the buildfiles more > self-documenting. Then that'd be the attached patch, which also includes --auth instead of -A, for the same reason as -N vs --no-sync Kind regards, Matthias van de Meent Neon (https://neon.tech) v2-0001-Don-t-remove-initdb-template-when-initdb-fails.patch Description: Binary data
Re: initdb caching during tests
> On 7 Dec 2023, at 14:50, Matthias van de Meent > wrote: > Attached a patch that fixes this for both make and meson, by adding > --no-clean to the initdb template. Makes sense. While in there I think we should rename -N to the long optoin --no-sync to make it easier to grep for and make the buildfiles more self-documenting. -- Daniel Gustafsson
Re: initdb caching during tests
On Fri, 25 Aug 2023 at 00:16, Andres Freund wrote:
>
> Hi,
>
> On 2023-08-23 10:10:31 +0200, Daniel Gustafsson wrote:
> > > On 23 Aug 2023, at 03:17, Andres Freund wrote:
> > > On 2023-08-22 23:47:24 +0200, Daniel Gustafsson wrote:
> >
> > >> My only small gripe is that I keep thinking about template databases for
> > >> CREATE
> > >> DATABASE when reading the error messages in this patch, which is clearly
> > >> not
> > >> related to what this does.
> > >>
> > >> + note("initializing database system by copying initdb template");
> > >>
> > >> I personally would've used cache instead of template in the user facing
> > >> parts
> > >> to keep concepts separated, but thats personal taste.
> > >
> > > I am going back and forth on that one (as one can notice with $subject).
> > > It
> > > doesn't quite seem like a cache, as it's not "created" on demand and only
> > > usable when the exactly same parameters are used repeatedly. But template
> > > is
> > > overloaded as you say...
> >
> > That's a fair point, cache is not a good word to describe a stored copy of
> > something prefabricated. Let's go with template, we can always refine
> > in-tree
> > if a better wording comes along.
>
> Cool. Pushed that way. Only change I made is to redirect the output of cp
> (and/or robocopy) in pg_regress, similar to how that was done for initdb
> proper.
While working on some things that are prone to breaking initdb, I
noticed that this template isn't generated with --no-clean, while
pg_regress does do that. This meant `make check` didn't have any
meaningful debuggable output when I broke the processes in initdb,
which is undesirable.
Attached a patch that fixes this for both make and meson, by adding
--no-clean to the initdb template.
Kind regards,
Matthias van de Meent
Neon (https://neon.tech)
v1-0001-Don-t-remove-initdb-template-when-initdb-fails.patch
Description: Binary data
Re: initdb caching during tests
On Thu, Aug 24, 2023 at 03:10:00PM -0700, Andres Freund wrote: > Cool. Pushed that way. I just noticed the tests running about 30% faster on my machine due to this. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: initdb caching during tests
Hi, On 2023-08-25 09:00:24 +0200, Daniel Gustafsson wrote: > > On 25 Aug 2023, at 07:50, Thomas Munro wrote: > > > > On Fri, Aug 25, 2023 at 10:10 AM Andres Freund wrote: > >> Let's see what the buildfarm says - it's not inconceivable that it'll show > >> some issues. > > > > Apparently Solaris doesn't like "cp -a", per animal "margay". I think > > "cp -RPp" should be enough everywhere? Thanks for noticing the issue and submitting the patch. > Agreed, AFAICT that should work equally well on all supported platforms. Also agreed. Unsurprisingly, CI didn't find anything on the tested platforms. Pushed. Greetings, Andres Freund
Re: initdb caching during tests
> On 25 Aug 2023, at 07:50, Thomas Munro wrote: > > On Fri, Aug 25, 2023 at 10:10 AM Andres Freund wrote: >> Let's see what the buildfarm says - it's not inconceivable that it'll show >> some issues. > > Apparently Solaris doesn't like "cp -a", per animal "margay". I think > "cp -RPp" should be enough everywhere? Agreed, AFAICT that should work equally well on all supported platforms. -- Daniel Gustafsson
Re: initdb caching during tests
On Fri, Aug 25, 2023 at 10:10 AM Andres Freund wrote:
> Let's see what the buildfarm says - it's not inconceivable that it'll show
> some issues.
Apparently Solaris doesn't like "cp -a", per animal "margay". I think
"cp -RPp" should be enough everywhere?
https://docs.oracle.com/cd/E88353_01/html/E37839/cp-1.html
https://pubs.opengroup.org/onlinepubs/9699919799.2013edition/utilities/cp.html
From fd6c558e6bd43eef40d633ace763d8a2088c2509 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Fri, 25 Aug 2023 17:30:48 +1200
Subject: [PATCH] Avoid non-POSIX cp flags.
Commit 252dcb32 introduced cp -a, but apparently Solaris doesn't like
it. Use cp -RPp instead.
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 426f94ff09..227c34ab4d 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -549,7 +549,7 @@ sub init
}
else
{
- @copycmd = qw(cp -a);
+ @copycmd = qw(cp -RPp);
$expected_exitcode = 0;
}
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 06674141a3..ec67588cf5 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -2355,7 +2355,7 @@ regression_main(int argc, char *argv[],
else
{
#ifndef WIN32
- const char *copycmd = "cp -a \"%s\" \"%s/data\"";
+ const char *copycmd = "cp -RPp \"%s\" \"%s/data\"";
int expected_exitcode = 0;
#else
const char *copycmd = "robocopy /E /NJS /NJH /NFL /NDL /NP \"%s\" \"%s/data\"";
--
2.39.2
Re: initdb caching during tests
Hi,
On 2023-08-23 10:10:31 +0200, Daniel Gustafsson wrote:
> > On 23 Aug 2023, at 03:17, Andres Freund wrote:
> > On 2023-08-22 23:47:24 +0200, Daniel Gustafsson wrote:
>
> >> My only small gripe is that I keep thinking about template databases for
> >> CREATE
> >> DATABASE when reading the error messages in this patch, which is clearly
> >> not
> >> related to what this does.
> >>
> >> + note("initializing database system by copying initdb template");
> >>
> >> I personally would've used cache instead of template in the user facing
> >> parts
> >> to keep concepts separated, but thats personal taste.
> >
> > I am going back and forth on that one (as one can notice with $subject). It
> > doesn't quite seem like a cache, as it's not "created" on demand and only
> > usable when the exactly same parameters are used repeatedly. But template is
> > overloaded as you say...
>
> That's a fair point, cache is not a good word to describe a stored copy of
> something prefabricated. Let's go with template, we can always refine in-tree
> if a better wording comes along.
Cool. Pushed that way. Only change I made is to redirect the output of cp
(and/or robocopy) in pg_regress, similar to how that was done for initdb
proper.
Let's see what the buildfarm says - it's not inconceivable that it'll show
some issues.
Greetings,
Andres Freund
Re: initdb caching during tests
> On 23 Aug 2023, at 03:17, Andres Freund wrote:
> On 2023-08-22 23:47:24 +0200, Daniel Gustafsson wrote:
>> My only small gripe is that I keep thinking about template databases for
>> CREATE
>> DATABASE when reading the error messages in this patch, which is clearly not
>> related to what this does.
>>
>> + note("initializing database system by copying initdb template");
>>
>> I personally would've used cache instead of template in the user facing parts
>> to keep concepts separated, but thats personal taste.
>
> I am going back and forth on that one (as one can notice with $subject). It
> doesn't quite seem like a cache, as it's not "created" on demand and only
> usable when the exactly same parameters are used repeatedly. But template is
> overloaded as you say...
That's a fair point, cache is not a good word to describe a stored copy of
something prefabricated. Let's go with template, we can always refine in-tree
if a better wording comes along.
--
Daniel Gustafsson
Re: initdb caching during tests
Hi,
On 2023-08-22 23:47:24 +0200, Daniel Gustafsson wrote:
> I had a look at this today and have been running a lot of tests with it
> without
> finding anything that breaks.
Thanks!
> The duplicated code is unfortunate, but after playing around with some
> options I agree that it's likely the best option.
Good and bad to hear :)
> While looking I did venture down the rabbithole of making it support extra
> params as well, but I don't think moving the goalposts there is doing us any
> favors, it's clearly chasing diminishing returns.
Agreed. I also went down that rabbithole, but it quickly gets a lot more code
and complexity - and there just aren't that many tests using non-default
options.
> My only small gripe is that I keep thinking about template databases for
> CREATE
> DATABASE when reading the error messages in this patch, which is clearly not
> related to what this does.
>
> + note("initializing database system by copying initdb template");
>
> I personally would've used cache instead of template in the user facing parts
> to keep concepts separated, but thats personal taste.
I am going back and forth on that one (as one can notice with $subject). It
doesn't quite seem like a cache, as it's not "created" on demand and only
usable when the exactly same parameters are used repeatedly. But template is
overloaded as you say...
> All in all, I think this is committable as is.
Cool. Planning to do that tomorrow. We can easily extend / adjust this later,
it just affects testing infrastructure.
Greetings,
Andres Freund
Re: initdb caching during tests
> On 5 Aug 2023, at 21:56, Andres Freund wrote:
> We have some issues with CI on macos and windows being too expensive (more on
> that soon in a separate email), which reminded me of this thread (with
> original title: [1])
>
> I've attached a somewhat cleaned up version of the patch to cache initdb
> across runs. The results are still fairly impressive in my opinion.
>
> One thing I do not like, but don't have a good idea for how to improve, is
> that there's a bunch of duplicated logic in pg_regress.c and Cluster.pm. I've
> tried to move that into initdb.c itself, but that ends up pretty ugly, because
> we need to be a lot more careful about checking whether options are compatible
> etc. I've also thought about just putting this into a separate perl script,
> but right now we still allow basic regression tests without perl being
> available. So I concluded that for now just having the copies is the best
> answer.
I had a look at this today and have been running a lot of tests with it without
finding anything that breaks. The duplicated code is unfortunate, but after
playing around with some options I agree that it's likely the best option.
While looking I did venture down the rabbithole of making it support extra
params as well, but I don't think moving the goalposts there is doing us any
favors, it's clearly chasing diminishing returns.
My only small gripe is that I keep thinking about template databases for CREATE
DATABASE when reading the error messages in this patch, which is clearly not
related to what this does.
+ note("initializing database system by copying initdb template");
I personally would've used cache instead of template in the user facing parts
to keep concepts separated, but thats personal taste.
All in all, I think this is committable as is.
--
Daniel Gustafsson
Re: initdb caching during tests
Hi, On 2023-08-05 16:58:38 -0400, Tom Lane wrote: > Andres Freund writes: > > Times for running all tests under meson, on my workstation (20 cores / 40 > > threads): > > > cassert build -O2: > > > Before: > > real0m44.638s > > user7m58.780s > > sys 2m48.773s > > > After: > > real0m38.938s > > user2m37.615s > > sys 2m0.570s > > Impressive results. Even though your bottom-line time doesn't change that > much Unfortunately we have a few tests that take quite a while - for those the initdb removal doesn't make that much of a difference. Particularly because this machine has enough CPUs to not be fully busy except for the first few seconds... E.g. for a run with the patch applied: 258/265 postgresql:pg_basebackup / pg_basebackup/010_pg_basebackup OK 16.58s 187 subtests passed 259/265 postgresql:subscription / subscription/100_bugs OK6.69s 12 subtests passed 260/265 postgresql:regress / regress/regress OK 24.95s 215 subtests passed 261/265 postgresql:ssl / ssl/001_ssltests OK7.97s 205 subtests passed 262/265 postgresql:pg_dump / pg_dump/002_pg_dump OK 19.65s 11262 subtests passed 263/265 postgresql:recovery / recovery/027_stream_regress OK 29.34s 6 subtests passed 264/265 postgresql:isolation / isolation/isolation OK 33.94s 112 subtests passed 265/265 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade OK 38.22s 18 subtests passed The pg_upgrade test is faster in isolation (29s), but not that much. The overall runtime is reduces due to the reduced "competing" cpu usage, but other than that... Looking at where the time is spent when running the pg_upgrade test on its own: grep -E '^\[' testrun/pg_upgrade/002_pg_upgrade/log/regress_log_002_pg_upgrade |sed -E -e 's/.*\(([0-9.]+)s\)(.*)/\1 \2/g'|sort -n -r cassert: 13.094 ok 5 - regression tests pass 6.147 ok 14 - run of pg_upgrade for new instance 2.340 ok 6 - dump before running pg_upgrade 1.638 ok 17 - dump after running pg_upgrade 1.375 ok 12 - run of pg_upgrade --check for new instance 0.798 ok 1 - check locales in original cluster 0.371 ok 9 - invalid database causes failure status (got 1 vs expected 1) 0.149 ok 7 - run of pg_upgrade --check for new instance with incorrect binary path 0.131 ok 16 - check that locales in new cluster match original cluster optimized: 8.372 ok 5 - regression tests pass 3.641 ok 14 - run of pg_upgrade for new instance 1.371 ok 12 - run of pg_upgrade --check for new instance 1.104 ok 6 - dump before running pg_upgrade 0.636 ok 17 - dump after running pg_upgrade 0.594 ok 1 - check locales in original cluster 0.359 ok 9 - invalid database causes failure status (got 1 vs expected 1) 0.148 ok 7 - run of pg_upgrade --check for new instance with incorrect binary path 0.127 ok 16 - check that locales in new cluster match original cluster The time for "dump before running pg_upgrade" is misleadingly high - there's no output between starting initdb and the dump, so the timing includes initdb and a bunch of other work. But it's still not fast (1.637s) after. A small factor is that the initdb times are not insignificant, because the template initdb can't be used due to a bunch of parameters passed to initdb :) > the big reduction in CPU time should translate to a nice speedup on slower > buildfarm animals. Yea. It's a particularly large win when using valgrind. Under valgrind, a very large portion of the time for many tests is just spent doing initdb... So I am hoping to see some nice gains for skink. Greetings, Andres Freund
Re: initdb caching during tests
Andres Freund writes: > Times for running all tests under meson, on my workstation (20 cores / 40 > threads): > cassert build -O2: > Before: > real 0m44.638s > user 7m58.780s > sys 2m48.773s > After: > real 0m38.938s > user 2m37.615s > sys 2m0.570s Impressive results. Even though your bottom-line time doesn't change that much, the big reduction in CPU time should translate to a nice speedup on slower buildfarm animals. (Disclaimer: I've not read the patch.) regards, tom lane
initdb caching during tests
Hi,
We have some issues with CI on macos and windows being too expensive (more on
that soon in a separate email), which reminded me of this thread (with
original title: [1])
I've attached a somewhat cleaned up version of the patch to cache initdb
across runs. The results are still fairly impressive in my opinion.
One thing I do not like, but don't have a good idea for how to improve, is
that there's a bunch of duplicated logic in pg_regress.c and Cluster.pm. I've
tried to move that into initdb.c itself, but that ends up pretty ugly, because
we need to be a lot more careful about checking whether options are compatible
etc. I've also thought about just putting this into a separate perl script,
but right now we still allow basic regression tests without perl being
available. So I concluded that for now just having the copies is the best
answer.
Times for running all tests under meson, on my workstation (20 cores / 40
threads):
cassert build -O2:
Before:
real0m44.638s
user7m58.780s
sys 2m48.773s
After:
real0m38.938s
user2m37.615s
sys 2m0.570s
cassert build -O0:
Before:
real1m11.290s
user13m9.817s
sys 2m54.946s
After:
real1m2.959s
user3m5.835s
sys 1m59.887s
non-cassert build:
Before:
real0m34.579s
user5m30.418s
sys 2m40.507s
After:
real0m27.710s
user2m20.644s
sys 1m55.770s
On CI this reduces the test times substantially:
Freebsd 8:51 -> 5:35
Debian w/ asan, autoconf 6:43 -> 4:55
Debian w/ alignmentsan, ubsan 4:02 -> 2:33
macos 5:07 -> 4:29
windows 10:21 -> 9:49
This is ignoring a bit of run-to-run variance, but the trend is obvious enough
that it's not worth worrying about that.
Greetings,
Andres Freund
[1] https://postgr.es/m/20220120021859.3zpsfqn4z7ob7afz%40alap3.anarazel.de
>From 0fd431c277f01284a91999a04368de6b59b6691e Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Thu, 2 Feb 2023 21:51:53 -0800
Subject: [PATCH v3 1/2] Use "template" initdb in tests
Discussion: https://postgr.es/m/20220120021859.3zpsfqn4z7ob7...@alap3.anarazel.de
---
meson.build | 30 ++
.cirrus.yml | 3 +-
src/test/perl/PostgreSQL/Test/Cluster.pm | 46 ++-
src/test/regress/pg_regress.c| 74 ++--
src/Makefile.global.in | 52 +
5 files changed, 161 insertions(+), 44 deletions(-)
diff --git a/meson.build b/meson.build
index 04ea3488522..47429a18c3f 100644
--- a/meson.build
+++ b/meson.build
@@ -3056,8 +3056,10 @@ testport = 4
test_env = environment()
temp_install_bindir = test_install_location / get_option('bindir')
+test_initdb_template = meson.build_root() / 'tmp_install' / 'initdb-template'
test_env.set('PG_REGRESS', pg_regress.full_path())
test_env.set('REGRESS_SHLIB', regress_module.full_path())
+test_env.set('INITDB_TEMPLATE', test_initdb_template)
# Test suites that are not safe by default but can be run if selected
# by the user via the whitespace-separated list in variable PG_TEST_EXTRA.
@@ -3072,6 +3074,34 @@ if library_path_var != ''
endif
+# Create (and remove old) initdb template directory. Tests use that, where
+# possible, to make it cheaper to run tests.
+#
+# Use python to remove the old cached initdb, as we cannot rely on a working
+# 'rm' binary on windows.
+test('initdb_cache',
+ python,
+ args: [
+ '-c', '''
+import shutil
+import sys
+import subprocess
+
+shutil.rmtree(sys.argv[1], ignore_errors=True)
+sp = subprocess.run(sys.argv[2:] + [sys.argv[1]])
+sys.exit(sp.returncode)
+''',
+ test_initdb_template,
+ temp_install_bindir / 'initdb',
+ '-A', 'trust', '-N', '--no-instructions'
+ ],
+ priority: setup_tests_priority - 1,
+ timeout: 300,
+ is_parallel: false,
+ env: test_env,
+ suite: ['setup'])
+
+
###
# Test Generation
diff --git a/.cirrus.yml b/.cirrus.yml
index d260f15c4e2..8fce17dff08 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -115,8 +115,9 @@ task:
test_minimal_script: |
su postgres <<-EOF
ulimit -c unlimited
+ meson test $MTEST_ARGS --suite setup
meson test $MTEST_ARGS --num-processes ${TEST_JOBS} \
-tmp_install cube/regress pg_ctl/001_start_stop
+cube/regress pg_ctl/001_start_stop
EOF
on_failure:
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 5e161dbee60..4d449c35de9 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -522,8 +522,50 @@ sub init
mkdir $self->backup_dir;
mkdir $self->archive_dir;
- PostgreSQL::Test::Utils::system_or_bail('initdb', '-D', $pgdata, '-A',
- 'trust', '-N', @{ $params{extra} });
+ # If available and if there aren't any parameters, use a
