Horton

Thread

Commits

  1. Don't clean initdb files on template creation failure

  2. Avoid non-POSIX cp flags

  3. Use "template" data directory in tests

  4. tests: Consistently use pg_basebackup -cfast --no-sync to accelerate tests.

  1. slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2021-12-31T19:25:28Z

    Hi,

cfbot now runs most tests on windows, the windows task is by far the slowest,
and the task limitted most in concurrency [2]. Running tap tests is the
biggest part of that. This is a bigger issue on windows because we don't have
infrastructure (yet) to run tests in parallel.

There's a few tests which stand out in their slowness, which seem worth
addressing even if we tackle test parallelism on windows at some point. I
often find them to be the slowest tests on linux too.

Picking a random successful cfbot run [1] I see the following tap tests taking
more than 20 seconds:

67188 ms pg_basebackup t/010_pg_basebackup.pl
59710 ms recovery t/001_stream_rep.pl
57542 ms pg_rewind t/001_basic.pl
56179 ms subscription t/001_rep_changes.pl
42146 ms pgbench t/001_pgbench_with_server.pl
38264 ms recovery t/018_wal_optimize.pl
33642 ms subscription t/013_partition.pl
29129 ms pg_dump t/002_pg_dump.pl
25751 ms pg_verifybackup t/002_algorithm.pl
20628 ms subscription t/011_generated.pl

It would be good if we could make those tests faster, or if not easily
possible, at least split those tests into smaller tap tests.

Splitting a longer test into smaller ones is preferrable even if they take the
same time, because we can use prove level concurrency on windows to gain some
test parallelism. As a nice side-effect it makes it also quicker to run a
split test isolated during development.

Greetings,

Andres Freund

[1] https://cirrus-ci.com/task/5207126145499136
[2] https://cirrus-ci.org/faq/#are-there-any-limits

  2. Re: slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2022-01-17T18:41:44Z

    Hi,

On 2021-12-31 11:25:28 -0800, Andres Freund wrote:
> cfbot now runs most tests on windows, the windows task is by far the slowest,
> and the task limitted most in concurrency [2]. Running tap tests is the
> biggest part of that. This is a bigger issue on windows because we don't have
> infrastructure (yet) to run tests in parallel.
> 
> There's a few tests which stand out in their slowness, which seem worth
> addressing even if we tackle test parallelism on windows at some point. I
> often find them to be the slowest tests on linux too.
> 
> Picking a random successful cfbot run [1] I see the following tap tests taking
> more than 20 seconds:
> 
> 67188 ms pg_basebackup t/010_pg_basebackup.pl
> 25751 ms pg_verifybackup t/002_algorithm.pl

The reason these in particular are slow is that they do a lot of
pg_basebackups without either / one-of -cfast / --no-sync. The lack of -cfast
in particularly is responsible for a significant proportion of the test
time. The only reason this didn't cause the tests to take many minutes is that
spread checkpoints only throttle when writing out a buffer and there aren't
that many dirty buffers...

Attached is a patch changing the parameters in all the instances I
found. Testing on a local instance it about halves the runtime of
t/010_pg_basebackup.pl on linux and windows (but there's still a 2x time
difference between the two), it's less when running the tests concurrently CI.

It might be worth having one explicit use of -cspread. Perhaps combined with
an explicit checkpoint beforehand?

Greetings,

Andres Freund

  3. Re: slowest tap tests - split or accelerate?

    Robert Haas <robertmhaas@gmail.com> — 2022-01-17T19:05:17Z

    On Mon, Jan 17, 2022 at 1:41 PM Andres Freund <andres@anarazel.de> wrote:
> The reason these in particular are slow is that they do a lot of
> pg_basebackups without either / one-of -cfast / --no-sync. The lack of -cfast
> in particularly is responsible for a significant proportion of the test
> time. The only reason this didn't cause the tests to take many minutes is that
> spread checkpoints only throttle when writing out a buffer and there aren't
> that many dirty buffers...

Adding -cfast to 002_algorithm.pl seems totally reasonable. I'm not
sure what else can realistically be done to speed it up without losing
the point of the test. And it's basically just a single loop, so
splitting it up doesn't seem to make a lot of sense either.

pg_basebackup's 010_pg_basebackup.pl looks like it could be split up,
though. That one, at least to me, looks like people have just kept
adding semi-related things into the same test file.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  4. Re: slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2022-01-17T19:57:11Z

    Hi,

On 2022-01-17 14:05:17 -0500, Robert Haas wrote:
> On Mon, Jan 17, 2022 at 1:41 PM Andres Freund <andres@anarazel.de> wrote:
> > The reason these in particular are slow is that they do a lot of
> > pg_basebackups without either / one-of -cfast / --no-sync. The lack of -cfast
> > in particularly is responsible for a significant proportion of the test
> > time. The only reason this didn't cause the tests to take many minutes is that
> > spread checkpoints only throttle when writing out a buffer and there aren't
> > that many dirty buffers...
>
> Adding -cfast to 002_algorithm.pl seems totally reasonable. I'm not
> sure what else can realistically be done to speed it up without losing
> the point of the test. And it's basically just a single loop, so
> splitting it up doesn't seem to make a lot of sense either.

It's also not that slow compared other tests after the -cfast addition.

However, I'm a bit surprised at how long the individual pg_verifybackup calls
take on windows - about as long as the pg_basebackup call itself.

 Running: pg_basebackup -D C:/dev/postgres/.\src\bin\pg_verifybackup\/tmp_check/t_002_algorithm_primary_data/backup/sha224 --manifest-checksums sha224 --no-sync -cfast
# timing: [4.798 + 0.704s]: complete
# Running: pg_verifybackup -e C:/dev/postgres/.\src\bin\pg_verifybackup\/tmp_check/t_002_algorithm_primary_data/backup/sha224
backup successfully verified
# timing: [5.507 + 0.697s]: completed


Interestingly, with crc32c, this is not so:

# Running: pg_basebackup -D C:/dev/postgres/.\src\bin\pg_verifybackup\/tmp_check/t_002_algorithm_primary_data/backup/crc32c --manifest-checksums crc32c --no-sync -cfast
# timing: [3.500 + 0.688s]: completed
ok 5 - backup ok with algorithm "crc32c"
ok 6 - crc32c is mentioned many times in the manifest
# Running: pg_verifybackup -e C:/dev/postgres/.\src\bin\pg_verifybackup\/tmp_check/t_002_algorithm_primary_data/backup/crc32c
backup successfully verified
# timing: [4.194 + 0.197s]: completed


I wonder if there's something explaining why pg_verifybackup is greatly slowed
down by sha224 but not crc32c, but the server's runtime only differs by ~20ms?
It seems incongruous that pg_basebackup, with all the complexity of needing to
communicate with the server, transferring the backup over network, and *also*
computing checksums, takes as long as the pg_verifybackup invocation?


> pg_basebackup's 010_pg_basebackup.pl looks like it could be split up,
> though. That one, at least to me, looks like people have just kept
> adding semi-related things into the same test file.


Yea.


It's generally interesting how much time initdb takes in these tests. It's
about 1.1s on my linux workstation, and 2.1s on windows.

I've occasionally pondered caching initdb results and reusing them across
tests - just the locking around it seems a bit nasty, but perhaps that could
be done as part of the tmp_install step. Of course, it'd need to deal with
different options etc...

Greetings,

Andres Freund

  5. Re: slowest tap tests - split or accelerate?

    Robert Haas <robertmhaas@gmail.com> — 2022-01-17T20:13:57Z

    On Mon, Jan 17, 2022 at 2:57 PM Andres Freund <andres@anarazel.de> wrote:
> I wonder if there's something explaining why pg_verifybackup is greatly slowed
> down by sha224 but not crc32c, but the server's runtime only differs by ~20ms?
> It seems incongruous that pg_basebackup, with all the complexity of needing to
> communicate with the server, transferring the backup over network, and *also*
> computing checksums, takes as long as the pg_verifybackup invocation?

I guess there must be something explaining it, but I don't know what
it could be. The client and the server are each running the checksum
algorithm over the same data. If that's not the same speed then .... I
don't get it. Unless, somehow, they're using different implementations
of it?

> I've occasionally pondered caching initdb results and reusing them across
> tests - just the locking around it seems a bit nasty, but perhaps that could
> be done as part of the tmp_install step. Of course, it'd need to deal with
> different options etc...

It's a thought, but it does seem like a bit of a pain to implement.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  6. Re: slowest tap tests - split or accelerate?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-17T20:48:54Z

    Andres Freund <andres@anarazel.de> writes:
> I've occasionally pondered caching initdb results and reusing them across
> tests - just the locking around it seems a bit nasty, but perhaps that could
> be done as part of the tmp_install step. Of course, it'd need to deal with
> different options etc...

I'd actually built a prototype to do that, based on making a reference
cluster and then "cp -a"'ing it instead of re-running initdb.  I gave
up when I found than on slower, disk-bound machines it was hardly
any faster.  Thinking about it now, I wonder why not just re-use one
cluster for many tests, only dropping and re-creating the database
in which the testing happens.

			regards, tom lane

  7. Re: slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2022-01-17T21:03:26Z

    Hi,

On 2022-01-17 15:13:57 -0500, Robert Haas wrote:
> I guess there must be something explaining it, but I don't know what
> it could be. The client and the server are each running the checksum
> algorithm over the same data. If that's not the same speed then .... I
> don't get it. Unless, somehow, they're using different implementations
> of it?

I think that actually might be the issue. On linux a test a pg_verifybackup
was much faster than on windows (as in 10x). But if I disable openssl, it's
only 2x.

On the windows instance I *do* have openssl enabled. But I suspect something
is off and the windows buildsystem ends up with our hand-rolled implementation
on the client side, but not the server side. Which'd explain the times I'm
seeing: We have a fast CRC implementation, but the rest is pretty darn
unoptimized.

Greetings,

Andres Freund

  8. Re: slowest tap tests - split or accelerate?

    Robert Haas <robertmhaas@gmail.com> — 2022-01-18T17:49:16Z

    On Mon, Jan 17, 2022 at 2:57 PM Andres Freund <andres@anarazel.de> wrote:
> > pg_basebackup's 010_pg_basebackup.pl looks like it could be split up,
> > though. That one, at least to me, looks like people have just kept
> > adding semi-related things into the same test file.
>
> Yea.

Here's a patch that splits up that file. Essentially the first half of
the file is concerned with testing that a backup ends up in the state
it expects, while the second half is concerned with checking that
various options to pg_basebackup work. So I split it that way, plus I
moved some of the really basic stuff to a completely separate file
with a very brief runtime. The test results are interesting.

Unpatched:

[12:33:33] t/010_pg_basebackup.pl ... ok    16161 ms ( 0.02 usr  0.00
sys +  2.07 cusr  7.80 csys =  9.89 CPU)
[12:33:49] t/020_pg_receivewal.pl ... ok     4115 ms ( 0.00 usr  0.00
sys +  0.89 cusr  1.73 csys =  2.62 CPU)
[12:33:53] t/030_pg_recvlogical.pl .. ok     1857 ms ( 0.01 usr  0.01
sys +  0.63 cusr  0.73 csys =  1.38 CPU)
[12:33:55]
All tests successful.
Files=3, Tests=177, 22 wallclock secs ( 0.04 usr  0.02 sys +  3.59
cusr 10.26 csys = 13.91 CPU)

Pached:

[12:32:03] t/010_pg_basebackup_basic.pl ...... ok      192 ms ( 0.01
usr  0.00 sys +  0.10 cusr  0.05 csys =  0.16 CPU)
[12:32:03] t/011_pg_basebackup_integrity.pl .. ok     5530 ms ( 0.00
usr  0.00 sys +  0.87 cusr  2.51 csys =  3.38 CPU)
[12:32:09] t/012_pg_basebackup_options.pl .... ok    13117 ms ( 0.00
usr  0.00 sys +  1.87 cusr  6.31 csys =  8.18 CPU)
[12:32:22] t/020_pg_receivewal.pl ............ ok     4314 ms ( 0.01
usr  0.00 sys +  0.97 cusr  1.77 csys =  2.75 CPU)
[12:32:26] t/030_pg_recvlogical.pl ........... ok     1908 ms ( 0.00
usr  0.00 sys +  0.64 cusr  0.77 csys =  1.41 CPU)
[12:32:28]
All tests successful.
Files=5, Tests=177, 25 wallclock secs ( 0.04 usr  0.02 sys +  4.45
cusr 11.41 csys = 15.92 CPU)

Sadly, we've gained about 2.5 seconds of runtime as the price of
splitting the test. Arguably the options part could be split up a lot
more finely than this, but that would drive up the runtime even more,
basically because we'd need more initdbs. So I don't know whether it's
better to leave things as they are, split them this much, or split
them more. I think this amount of splitting might be justified simply
in the interests of clarity, but I'm reluctant to go further unless we
get some nifty initdb-caching system.

Thoughts?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  9. Re: slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2022-01-18T21:40:40Z

    Hi,

On 2022-01-18 12:49:16 -0500, Robert Haas wrote:
> Here's a patch that splits up that file.

Ah, nice! The split seems sensible to me.


> Sadly, we've gained about 2.5 seconds of runtime as the price of
> splitting the test. Arguably the options part could be split up a lot
> more finely than this, but that would drive up the runtime even more,
> basically because we'd need more initdbs. So I don't know whether it's
> better to leave things as they are, split them this much, or split
> them more. I think this amount of splitting might be justified simply
> in the interests of clarity, but I'm reluctant to go further unless we
> get some nifty initdb-caching system.

Hm. From the buildfarm / CF perspective it might still be a win, because the
different pieces can run concurrently. But it's not great :(.

Maybe we really should do at least the most simplistic caching for initdbs, by
doing one initdb as part of the creation of temp_install. Then Cluster::init
would need logic to only use that if $params{extra} is empty.

Greetings,

Andres Freund

  10. Re: slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2022-01-19T01:00:34Z

    Hi,

On 2022-01-18 13:40:40 -0800, Andres Freund wrote:
> Maybe we really should do at least the most simplistic caching for initdbs, by
> doing one initdb as part of the creation of temp_install. Then Cluster::init
> would need logic to only use that if $params{extra} is empty.

I hacked this together. And the wins are bigger than I thought. On my
workstation, with plenty cpu and storage bandwidth, according to
  /usr/bin/time check-world NO_TEMP_INSTALL=1
things go from

  321.56user 74.00system 2:26.22elapsed 270%CPU (0avgtext+0avgdata 93768maxresident)k
  24inputs+32781336outputs (2254major+8717121minor)pagefaults 0swaps

to

  86.62user 57.10system 1:57.83elapsed 121%CPU (0avgtext+0avgdata 93752maxresident)k
  8inputs+32683408outputs (1360major+6672618minor)pagefaults 0swaps

The difference in elapsed and system time is pretty good, but the user time
difference is quite staggering.


This doesn't yet actually address the case of the basebackup tests, because
that specifies a "non-default" option, preventing the use of the template
initdb. But the effects are already big enough that I thought it's worth
sharing.

On CI for windows this reduces the time for the subscription tests from 03:24,
to 2:39. There's some run-to-run variation, but it's a pretty clear signal...

Greetings,

Andres Freund

  11. Re: slowest tap tests - split or accelerate?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-19T16:54:01Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-01-18 13:40:40 -0800, Andres Freund wrote:
>> Maybe we really should do at least the most simplistic caching for initdbs, by
>> doing one initdb as part of the creation of temp_install. Then Cluster::init
>> would need logic to only use that if $params{extra} is empty.

> I hacked this together. And the wins are bigger than I thought.

Me too ;-).  As I remarked earlier, I'd tried this once before and
gave up because it didn't seem to be winning much.  But that was
before we had so many initdb's triggered by TAP tests, I think.

I tried this patch on florican's host, which seems mostly disk-bound
when doing check-world.  It barely gets any win from parallelism:

$ time make -s check-world -j1 >/dev/null
     3809.60 real       584.44 user       282.23 sys
$ time make -s check-world -j2 >/dev/null
     3789.90 real       610.60 user       289.60 sys

Adding v2-0001-hack-use-template-initdb-in-tap-tests.patch:

$ time make -s check-world -j1 >/dev/null
     3193.46 real       221.32 user       226.11 sys
$ time make -s check-world -j2 >/dev/null
     3211.19 real       224.31 user       230.07 sys

(Note that all four runs have the "fsync = on" removed from
008_fsm_truncation.pl.)

So this looks like it'll be a nice win for low-end hardware, too.

			regards, tom lane

  12. Re: slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2022-01-19T17:03:30Z

    Hi,

On 2022-01-19 11:54:01 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2022-01-18 13:40:40 -0800, Andres Freund wrote:
> >> Maybe we really should do at least the most simplistic caching for initdbs, by
> >> doing one initdb as part of the creation of temp_install. Then Cluster::init
> >> would need logic to only use that if $params{extra} is empty.
>
> > I hacked this together. And the wins are bigger than I thought.
>
> Me too ;-).  As I remarked earlier, I'd tried this once before and
> gave up because it didn't seem to be winning much.  But that was
> before we had so many initdb's triggered by TAP tests, I think.

What approach did you use? Do you have a better idea than generating
tmp_install/initdb_template?

I for a bit wondered whether initdb should do this internally instead. But it
seemed more work than I wanted to tackle.

The bit in the patch about generating initdb_template in Install.pm definitely
needs to be made conditional, but I don't precisely know on what. The
buildfarm just calls it as
  perl install.pl "$installdir


> So this looks like it'll be a nice win for low-end hardware, too.

Nice!


> (Note that all four runs have the "fsync = on" removed from
> 008_fsm_truncation.pl.)

I assume you're planning on comitting that?

Greetings,

Andres Freund

  13. Re: slowest tap tests - split or accelerate?

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-19T17:14:21Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-01-19 11:54:01 -0500, Tom Lane wrote:
>> Me too ;-).  As I remarked earlier, I'd tried this once before and
>> gave up because it didn't seem to be winning much.  But that was
>> before we had so many initdb's triggered by TAP tests, I think.

> What approach did you use? Do you have a better idea than generating
> tmp_install/initdb_template?

No, it was largely the same as what you have here, I think.  I dug
up my WIP patch and attach it below, just in case there's any ideas
worth borrowing.

>> (Note that all four runs have the "fsync = on" removed from
>> 008_fsm_truncation.pl.)

> I assume you're planning on comitting that?

Yeah, will do that shortly.

			regards, tom lane

  14. Re: slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2022-01-19T17:42:31Z

    Hi,

On 2022-01-19 12:14:21 -0500, Tom Lane wrote:
> No, it was largely the same as what you have here, I think.  I dug
> up my WIP patch and attach it below, just in case there's any ideas
> worth borrowing.

Heh, it does look quite similar.


> +					 "cp -a \"%s\" \"%s/data\" > \"%s/log/initdb.log\" 2>&1",
> +					 pg_proto_datadir,
> +					 temp_instance,
> +					 outputdir);
> +			if (system(buf))
> +			{
> +				fprintf(stderr, _("\n%s: cp failed\nExamine %s/log/initdb.log for the reason.\nCommand was: %s\n"), progname, outputdir, buf);
> +				exit(2);
> +			}

Both ours have this. Unfortunately on windows cp doesn't natively
exist. Although git does provide it.  I tried a few things that appear to be
natively available (time is best of three executions):

 gnu cp from git, cp -a tmp_install\initdb_template t\
 670ms

 xcopy.exe /E /Q tmp_install\initdb_template t\
 638ms

 robocopy /e /NFL /NDL tmp_install\initdb_template t\
 575ms

So I guess we could use robocopy? That's shipped as part of windows starting in
windows 10... xcopy has been there for longer, so I might just default to that.

Greetings,

Andres Freund

  15. Re: slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2022-01-20T02:18:59Z

    Hi,

On 2022-01-19 09:42:31 -0800, Andres Freund wrote:
> Both ours have this. Unfortunately on windows cp doesn't natively
> exist. Although git does provide it.  I tried a few things that appear to be
> natively available (time is best of three executions):
> 
>  gnu cp from git, cp -a tmp_install\initdb_template t\
>  670ms
> 
>  xcopy.exe /E /Q tmp_install\initdb_template t\
>  638ms

This errors out if there's any forward slashes in paths, thinking it's a
flag. Seems out.


>  robocopy /e /NFL /NDL tmp_install\initdb_template t\
>  575ms
> 
> So I guess we could use robocopy? That's shipped as part of windows starting in
> windows 10... xcopy has been there for longer, so I might just default to that.

It's part of of the OS back to at least windows 2016. I've found some random
links on the webs saying that it's included "This command is available in
Vista and Windows 7 by default. For Windows XP and Server 2003 this tool can
be downloaded as part of Server 2003 Windows Resource Kit tools. ".

Given that our oldest supported msvc version only runs on Windows 7 upwards
[2], I think we should be good?


Alternatively we could lift copydir() to src/common? But that seems like a bit
more work than I want to put in.


For a second I was thinking that using something like copy --reflink=auto
could make a lot of sense for machines like florican, removing most of the IO
from a "templated initdb". But it looks like freebsd doesn't have that, and
it'd be a pain to figure out whether cp has --reflink.

Greetings,

Andres Freund

[1] https://www.windows-commandline.com/download-robocopy/
[2] https://docs.microsoft.com/en-us/visualstudio/releases/2013/vs2013-sysrequirements-vs

  16. Re: slowest tap tests - split or accelerate?

    Andrew Dunstan <andrew@dunslane.net> — 2022-01-20T21:54:59Z

    On 1/19/22 21:18, Andres Freund wrote:
> Hi,
>
> On 2022-01-19 09:42:31 -0800, Andres Freund wrote:
>> Both ours have this. Unfortunately on windows cp doesn't natively
>> exist. Although git does provide it.  I tried a few things that appear to be
>> natively available (time is best of three executions):
>>
>>  gnu cp from git, cp -a tmp_install\initdb_template t\
>>  670ms
>>
>>  xcopy.exe /E /Q tmp_install\initdb_template t\
>>  638ms
> This errors out if there's any forward slashes in paths, thinking it's a
> flag. Seems out.
>
>
>>  robocopy /e /NFL /NDL tmp_install\initdb_template t\
>>  575ms
>>
>> So I guess we could use robocopy? That's shipped as part of windows starting in
>> windows 10... xcopy has been there for longer, so I might just default to that.
> It's part of of the OS back to at least windows 2016. I've found some random
> links on the webs saying that it's included "This command is available in
> Vista and Windows 7 by default. For Windows XP and Server 2003 this tool can
> be downloaded as part of Server 2003 Windows Resource Kit tools. ".
>
> Given that our oldest supported msvc version only runs on Windows 7 upwards
> [2], I think we should be good?
>
>
> Alternatively we could lift copydir() to src/common? But that seems like a bit
> more work than I want to put in.
>
>
> For a second I was thinking that using something like copy --reflink=auto
> could make a lot of sense for machines like florican, removing most of the IO
> from a "templated initdb". But it looks like freebsd doesn't have that, and
> it'd be a pain to figure out whether cp has --reflink.



FYI, the buildfarm code has this. It doesn't need backslashed paths, you
just need to quote the paths, which you should probably do anyway:


    sub copydir
    {
        my ($from, $to, $logfile) = @_;
        my ($cp, $rd);
        if ($PGBuild::conf{using_msvc})
        {
            $cp = "robocopy /nfl /ndl /np /e /sec ";
            $rd = qq{/LOG+:"$logfile" >nul};
        }
        else
        {
            $cp = "cp -r";
            $rd = qq{> "$logfile"};
        }
        system(qq{$cp "$from" "$to" $rd 2>&1});
        ## no critic (RequireLocalizedPunctuationVars)
        $? = 0 if ($cp =~ /robocopy/ && $? >> 8 == 1);
        return;
    }

cheers


andrew


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

  17. Re: slowest tap tests - split or accelerate?

    Andres Freund <andres@anarazel.de> — 2022-01-26T17:54:34Z

    Hi,

On 2022-01-19 18:18:59 -0800, Andres Freund wrote:
> >  robocopy /e /NFL /NDL tmp_install\initdb_template t\
> >  575ms
> > 
> > So I guess we could use robocopy? That's shipped as part of windows starting in
> > windows 10... xcopy has been there for longer, so I might just default to that.
> 
> It's part of of the OS back to at least windows 2016. I've found some random
> links on the webs saying that it's included "This command is available in
> Vista and Windows 7 by default. For Windows XP and Server 2003 this tool can
> be downloaded as part of Server 2003 Windows Resource Kit tools. ".
> 
> Given that our oldest supported msvc version only runs on Windows 7 upwards
> [2], I think we should be good?

One thing I'm not sure about is where to perform the creation of the
"template" for the msvc scripts. The prototype upthread created it
unconditionally in Install.pm, but that's clearly not right.

The buildfarm currently creates the temporary installation using a generic
perl install.pl "$installdir" and then uses NO_TEMP_INSTALL.

I don't really have a better idea than to introduce a dedicated vcregress.pl
command to create the temporary installation? :(

Greetings,

Andres Freund

  18. initdb caching during tests

    Andres Freund <andres@anarazel.de> — 2023-08-05T19:56:56Z

    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:
real	0m44.638s
user	7m58.780s
sys	2m48.773s

After:
real	0m38.938s
user	2m37.615s
sys	2m0.570s


cassert build -O0:

Before:
real	1m11.290s
user	13m9.817s
sys	2m54.946s

After:
real	1m2.959s
user	3m5.835s
sys	1m59.887s


non-cassert build:

Before:
real	0m34.579s
user	5m30.418s
sys	2m40.507s

After:
real	0m27.710s
user	2m20.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

  19. Re: initdb caching during tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-08-05T20:58:38Z

    Andres Freund <andres@anarazel.de> 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

  20. Re: initdb caching during tests

    Andres Freund <andres@anarazel.de> — 2023-08-05T22:26:39Z

    Hi,

On 2023-08-05 16:58:38 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> 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

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                                    OK                6.69s   12 subtests passed
260/265 postgresql:regress / regress/regress                                               OK               24.95s   215 subtests passed
261/265 postgresql:ssl / ssl/001_ssltests                                                  OK                7.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

  21. Re: initdb caching during tests

    Daniel Gustafsson <daniel@yesql.se> — 2023-08-22T21:47:24Z

    > On 5 Aug 2023, at 21:56, Andres Freund <andres@anarazel.de> 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

  22. Re: initdb caching during tests

    Andres Freund <andres@anarazel.de> — 2023-08-23T01:17:45Z

    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

  23. Re: initdb caching during tests

    Daniel Gustafsson <daniel@yesql.se> — 2023-08-23T08:10:31Z

    > On 23 Aug 2023, at 03:17, Andres Freund <andres@anarazel.de> 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

  24. Re: initdb caching during tests

    Andres Freund <andres@anarazel.de> — 2023-08-24T22:10:00Z

    Hi,

On 2023-08-23 10:10:31 +0200, Daniel Gustafsson wrote:
> > On 23 Aug 2023, at 03:17, Andres Freund <andres@anarazel.de> 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

  25. Re: initdb caching during tests

    Thomas Munro <thomas.munro@gmail.com> — 2023-08-25T05:50:45Z

    On Fri, Aug 25, 2023 at 10:10 AM Andres Freund <andres@anarazel.de> 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

  26. Re: initdb caching during tests

    Daniel Gustafsson <daniel@yesql.se> — 2023-08-25T07:00:24Z

    > On 25 Aug 2023, at 07:50, Thomas Munro <thomas.munro@gmail.com> wrote:
> 
> On Fri, Aug 25, 2023 at 10:10 AM Andres Freund <andres@anarazel.de> 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

  27. Re: initdb caching during tests

    Andres Freund <andres@anarazel.de> — 2023-08-25T13:57:33Z

    Hi,

On 2023-08-25 09:00:24 +0200, Daniel Gustafsson wrote:
> > On 25 Aug 2023, at 07:50, Thomas Munro <thomas.munro@gmail.com> wrote:
> > 
> > On Fri, Aug 25, 2023 at 10:10 AM Andres Freund <andres@anarazel.de> 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

  28. Re: initdb caching during tests

    Nathan Bossart <nathandbossart@gmail.com> — 2023-08-25T16:29:59Z

    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

  29. Re: initdb caching during tests

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-12-07T13:50:46Z

    On Fri, 25 Aug 2023 at 00:16, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2023-08-23 10:10:31 +0200, Daniel Gustafsson wrote:
> > > On 23 Aug 2023, at 03:17, Andres Freund <andres@anarazel.de> 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)

  30. Re: initdb caching during tests

    Daniel Gustafsson <daniel@yesql.se> — 2023-12-07T14:06:41Z

    > On 7 Dec 2023, at 14:50, Matthias van de Meent <boekewurm+postgres@gmail.com> 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

  31. Re: initdb caching during tests

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-12-07T14:27:10Z

    On Thu, 7 Dec 2023 at 15:06, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 7 Dec 2023, at 14:50, Matthias van de Meent <boekewurm+postgres@gmail.com> 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)

  32. Re: initdb caching during tests

    Daniel Gustafsson <daniel@yesql.se> — 2023-12-08T12:59:22Z

    > On 7 Dec 2023, at 15:27, Matthias van de Meent <boekewurm+postgres@gmail.com> 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