Thread: improving speed of make check-world

make check-world creates a temporary installation in every subdirectory
it runs a test in, which is stupid: it's very slow and uses a lot of
disk space.  It's enough to do this once per run.  That is the essence
of what I have implemented.  It cuts the time for make check-world in
half or less, and it saves gigabytes of disk space.

The idea is that we only maintain one temporary installation under the
top-level directory.  By looking at the variable MAKELEVEL, we can tell
whether we are the top-level make invocation.  If so, make a temp
installation.  If not, we know that the upper layers have already done
it and we can just point to the existing temp installation.

I do this by ripping out the temp installation logic from pg_regress and
implementing it directly in the makefiles.  This is much simpler and has
additional potential benefits:

The pg_regress temp install mode is actually a combination of two
functionalities: temp install plus temp instance.  Until now, you could
only get the two together, but the temp instance functionality is
actually quite useful by itself.  It opens up the possibility of
implementing "make check" for external pgxs modules, for example.

Also, you could now run the temp installation step using parallel make,
making it even faster.  This was previously disabled because the make
flags would have to pass through pg_regress.  It still won't quite work
to run make check-world -j8, say, because we can't actually run the
tests in parallel (yet), but something like make -C src/test/regress
check -j8 should work.

To that end, I have renamed the pg_regress --temp-install option to
--temp-instance.  Since --temp-install is only used inside the source
tree, this shouldn't cause any compatibility problems.

MSVC build is not yet adjusted for this.  Looking at vcregress.pl, this
should be fairly straightforward.

On 08/15/2014 08:45 AM, Peter Eisentraut wrote:
> make check-world creates a temporary installation in every subdirectory
> it runs a test in, which is stupid: it's very slow and uses a lot of
> disk space.  It's enough to do this once per run.  That is the essence
> of what I have implemented.  It cuts the time for make check-world in
> half or less, and it saves gigabytes of disk space.

Nice!

> The idea is that we only maintain one temporary installation under the
> top-level directory.  By looking at the variable MAKELEVEL, we can tell
> whether we are the top-level make invocation.  If so, make a temp
> installation.  If not, we know that the upper layers have already done
> it and we can just point to the existing temp installation.
>
> I do this by ripping out the temp installation logic from pg_regress and
> implementing it directly in the makefiles.  This is much simpler and has
> additional potential benefits:
>
> The pg_regress temp install mode is actually a combination of two
> functionalities: temp install plus temp instance.  Until now, you could
> only get the two together, but the temp instance functionality is
> actually quite useful by itself.  It opens up the possibility of
> implementing "make check" for external pgxs modules, for example.
>
> Also, you could now run the temp installation step using parallel make,
> making it even faster.  This was previously disabled because the make
> flags would have to pass through pg_regress.  It still won't quite work
> to run make check-world -j8, say, because we can't actually run the
> tests in parallel (yet), but something like make -C src/test/regress
> check -j8 should work.
>
> To that end, I have renamed the pg_regress --temp-install option to
> --temp-instance.  Since --temp-install is only used inside the source
> tree, this shouldn't cause any compatibility problems.

Yeah, that all makes a lot of sense.

The new EXTRA_INSTALL makefile variable ought to be documented in 
extend.sgml, where we list REGRESS_OPTS and others.

- Heikki

On 8/25/14 1:32 PM, Heikki Linnakangas wrote:
> The new EXTRA_INSTALL makefile variable ought to be documented in
> extend.sgml, where we list REGRESS_OPTS and others.

But EXTRA_INSTALL is only of use inside the main source tree, not by
extensions.

Updated, rebased patch.

Hello Peter,

Here is a review:

The version 2 of the patch applies cleanly on current head.

The ability to generate and reuse a temporary installation for different 
tests looks quite useful, thus putting install out of pg_regress and in 
make seems reasonnable.

However I'm wondering whether there could be some use case of pg_regress 
where doing the install might be useful nevertheless, say for manually 
doing things on the command line, in which case keeping the feature, even 
if not used by default, could be nice?

About changes: I think that more comments would be welcome, eg 
with_temp_install.

There is not a single documentation change. Should there be some? Hmmm... 
I have not found much documentation about "pg_regress"...

I have tested make check, check-world, as well as make check in contrib 
sub-directories. It seems to work fine in sequential mode.

Running "make -j2 check-world" does not work because "initdb" is not found 
by "pg_regress". but "make -j1 check-world" does work fine. It seems that 
some dependencies might be missing and there is a race condition between 
temporary install and running some checks?? Maybe it is not expected to 
work anyway? See below suggestions to make it work.

However in this case the error message passed by pg_regress contains an 
error:
 pg_regress: initdb failed Examine /home/fabien/DEV/GIT/postgresql/contrib/btree_gin/log/initdb.log for the reason.
Commandwas: "initdb" -D "/home/fabien/DEV/GIT/postgresql/contrib/btree_gin/./tmp_check/data" --noclean --nosync >
"/home/fabien/DEV/GIT/postgresql/contrib/btree_gin/./tmp_check/log/initdb.log"2>&1 make[2]: *** [check] Error 2
 

The error messages point to non existing log file (./tmp_check is 
missing), the temp_instance variable should be used in the error message 
as well as in the commands. Maybe other error messages have the same 
issues.

I do not like much the MAKELEVEL hack in a phony target. When running in 
parallel, several makes may have the same level anyway, not sure what 
would happen... Maybe it is the origin of the race condition which makes 
initdb not to be found above. I would suggest to try to rely on 
dependences, maybe something like the following could work to ensure that 
an installation is done once and only once per make invocation, whatever 
the underlying parallelism & levels, as well as a possibility to reuse the 
previous installation.
  # must be defined somewhere common to all makefiles  ifndef MAKE_NONCE  # define a nanosecond timestamp  export
MAKE_NONCE:= $(shell date +%s.%N)  endif
 
  # actual new tmp installation  .tmp_install:    $(RM) ./.tmp_install.*    $(RM) -r ./tmp_install    # create tmp
installation...   touch $@
 
  # tmp installation for the nonce  .tmp_install.$(MAKE_NONCE): .tmp_install      touch $@
  # generate a new tmp installation by default  # "make USE_TMP_INSTALL=1 ..." reuses a previous installation if
available ifdef USE_TMP_INSTALL  TMP_INSTALL = .tmp_install  else  TMP_INSTALL = .tmp_install.$(MAKE_NONCE)  endif #
USE_TMP_INSTALL
  .PHONY: main-temp-install  main-temp-install: $(TMP_INSTALL)
  .PHONY: extra-temp-install  extra-temp-install: main-temp-install    # do EXTRA_INSTALL


> MSVC build is not yet adjusted for this.  Looking at vcregress.pl, this 
> should be fairly straightforward.

No idea about that point.

-- 
Fabien.

>  # actual new tmp installation
>  .tmp_install:
>     $(RM) ./.tmp_install.*
>     $(RM) -r ./tmp_install
>     # create tmp installation...
>     touch $@
>
>  # tmp installation for the nonce
>  .tmp_install.$(MAKE_NONCE): .tmp_install
>      touch $@

Oops, I got it wrong, the install would not be reexecuted the second time.

Maybe someting more like:
  ifdef USE_ONE_INSTALL  TMP_INSTALL = .tmp_install.once  else  TMP_INSTALL = .tmp_install.$(MAKE_NONCE)  endif
  $(TMP_INSTALL):      $(RM) -r ./tmp_install .tmp_install.*      # do installation...      touch $@

So that the file target is different each time it is run. Hopefully.

-- 
Fabien.

On 8/31/14 5:36 AM, Fabien COELHO wrote:
> Running "make -j2 check-world" does not work because "initdb" is not
> found by "pg_regress". but "make -j1 check-world" does work fine. It
> seems that some dependencies might be missing and there is a race
> condition between temporary install and running some checks?? Maybe it
> is not expected to work anyway? See below suggestions to make it work.

Here is an updated patch that fixes this problem.

The previous problem was simply a case were the make rules were not
parallel-safe.

For recursive make, we (through magic) set up targets like this:

    check: check-subdir1-check check-subdir2-check

And with my old patch we added

    check: temp-install

So the aggregate prerequisites were in effect something like

    check: check-subdir1-check check-subdir2-check temp-install

And so there was nothing stopping a parallel make to descend into the
subdirectories before the temp install was set up.

What we need is additional prerequisites like

    check-subdir1-check check-subdir2-check: temp-install

I have hacked this directly into the $(recurse) function, which is ugly.
 This could possibly be improved somehow, but the effect would be the
same in any case.

With this, I can now run things like

    make -C src/pl check -j3
    make -C src/bin check -j8

A full make check-world -j$X is still prone to fail because some test
suites can't run in parallel with others, but that's a separate problem
to fix.


Note:  We have in the meantime added logic to pg_regress to clean up the
temporary installation after the run.  This changes that because
pg_regress is no longer responsible for the temporary installation.
pg_regress still cleans up the temporary data directory, so you still
get quite a bit of space savings.  But the temporary installation is not
cleaned up.  But since we would now only use a single temporary
installation, the disk space usage still stays in the same order of
magnitude.

Peter Eisentraut <peter_e@gmx.net> writes:
> On 8/31/14 5:36 AM, Fabien COELHO wrote:
>> Running "make -j2 check-world" does not work because "initdb" is not
>> found by "pg_regress". but "make -j1 check-world" does work fine. It
>> seems that some dependencies might be missing and there is a race
>> condition between temporary install and running some checks?? Maybe it
>> is not expected to work anyway? See below suggestions to make it work.

> Here is an updated patch that fixes this problem.

Doesn't the Windows side of the house still depend on that functionality
you removed from pg_regress?  Perhaps that's not a big deal to fix, but
it seems like a commit-blocker from here.
        regards, tom lane

On Sun, Feb 15, 2015 at 11:01 AM, Peter Eisentraut wrote:
> Here is an updated patch.

Nice patch. This is going to save a lot of resources.

An update of vcregress.pl is necessary. This visibly just consists in
updating the options that have been renamed in pg_regress (don't mind
testing any code sent out).

-               {"top-builddir", required_argument, NULL, 11},
+               {"datadir", required_argument, NULL, 12},
In pg_regress.c datadir is a new option but it is used nowhere, so it
could be as well removed.
-- 
Michael

On 2/24/15 3:06 AM, Michael Paquier wrote:
> On Sun, Feb 15, 2015 at 11:01 AM, Peter Eisentraut wrote:
>> Here is an updated patch.
>
> Nice patch. This is going to save a lot of resources.
>
> An update of vcregress.pl is necessary. This visibly just consists in
> updating the options that have been renamed in pg_regress (don't mind
> testing any code sent out).

Well, that turns out to be more complicated than initially thought.
Apparently, the msvc has a bit of a different idea of what check and
installcheck do with respect to temporary installs.  For instance,
vcregress installcheck does not use psql from the installation but from
the build tree.  vcregress check uses psql from the build tree but other
binaries (initdb, pg_ctl) from the temporary installation.  It is hard
for me to straighten this out by just looking at the code.  Attached is
a patch that shows the idea, but I can't easily take it further than that.

> -               {"top-builddir", required_argument, NULL, 11},
> +               {"datadir", required_argument, NULL, 12},
> In pg_regress.c datadir is a new option but it is used nowhere, so it
> could be as well removed.

Yeah, that's an oversight that is easily corrected.

On Sun, Mar 8, 2015 at 10:22 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 2/24/15 3:06 AM, Michael Paquier wrote:
>> On Sun, Feb 15, 2015 at 11:01 AM, Peter Eisentraut wrote:
>>> Here is an updated patch.
>>
>> Nice patch. This is going to save a lot of resources.
>>
>> An update of vcregress.pl is necessary. This visibly just consists in
>> updating the options that have been renamed in pg_regress (don't mind
>> testing any code sent out).
>
> Well, that turns out to be more complicated than initially thought.
> Apparently, the msvc has a bit of a different idea of what check and
> installcheck do with respect to temporary installs.  For instance,
> vcregress installcheck does not use psql from the installation but from
> the build tree.  vcregress check uses psql from the build tree but other
> binaries (initdb, pg_ctl) from the temporary installation.  It is hard
> for me to straighten this out by just looking at the code.  Attached is
> a patch that shows the idea, but I can't easily take it further than that.

Urg. Yes for installcheck I guess that we cannot do much but simply
use the psql from the tree, but on the contrary for all the other
targets we can use the temporary installation as $topdir/tmp_install.

Regarding the patch you sent, IMO it is not a good idea to kick
install with system() as this can fail as an unrecognized command
runnable. And the command that should be used is not "vcregress
install $path" but simply "vcregress install". Hence I think that
calling simply Install() makes more sense. Also, I think that it would
be better to not enforce PATH before kicking the commands.

Speaking of which, attached is a patch rewritten in-line with those
comments, simplifying a bit the whole at the same time. Note this
patch changes ecpgcheck as it should be patched, but as ecpgcheck test
is broken even on HEAD, I'll raise a separate thread about that with a
proper fix (for example bowerbird skips this test).
On my side, with this patch, installcheck, check, plcheck,
upgradecheck work properly and all of them use the common
installation. It would be more adapted to add checks on the existence
of $tmp_installdir/bin though in InstallTemp instead of kicking an
installation all the time. But that's simple enough to change.
Regards,
--
Michael

On Sun, Mar 8, 2015 at 10:46 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Speaking of which, attached is a patch rewritten in-line with those
> comments, simplifying a bit the whole at the same time. Note this
> patch changes ecpgcheck as it should be patched, but as ecpgcheck test
> is broken even on HEAD, I'll raise a separate thread about that with a
> proper fix (for example bowerbird skips this test).

Correction: HEAD is fine, but previous patch was broken because it
tried to use --top-builddir. Also for ecpgcheck it looks that tricking
PATH is needed to avoid dll loading errors related to libecpg.dll and
libecpg_compat.dll. Updated patch is attached.

Bonus track: pg_regress.c is missing the description of --bindir in help().
--
Michael

On 3/9/15 2:51 AM, Michael Paquier wrote:
> On Sun, Mar 8, 2015 at 10:46 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Speaking of which, attached is a patch rewritten in-line with those
>> comments, simplifying a bit the whole at the same time. Note this
>> patch changes ecpgcheck as it should be patched, but as ecpgcheck test
>> is broken even on HEAD, I'll raise a separate thread about that with a
>> proper fix (for example bowerbird skips this test).
> 
> Correction: HEAD is fine, but previous patch was broken because it
> tried to use --top-builddir. Also for ecpgcheck it looks that tricking
> PATH is needed to avoid dll loading errors related to libecpg.dll and
> libecpg_compat.dll. Updated patch is attached.

To clarify: Are you of the opinion that your patch (on top of my base
patch) is sufficient to make all of this work on Windows?

On Sat, Apr 11, 2015 at 4:35 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 3/9/15 2:51 AM, Michael Paquier wrote:
>> On Sun, Mar 8, 2015 at 10:46 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> Speaking of which, attached is a patch rewritten in-line with those
>>> comments, simplifying a bit the whole at the same time. Note this
>>> patch changes ecpgcheck as it should be patched, but as ecpgcheck test
>>> is broken even on HEAD, I'll raise a separate thread about that with a
>>> proper fix (for example bowerbird skips this test).
>>
>> Correction: HEAD is fine, but previous patch was broken because it
>> tried to use --top-builddir. Also for ecpgcheck it looks that tricking
>> PATH is needed to avoid dll loading errors related to libecpg.dll and
>> libecpg_compat.dll. Updated patch is attached.
>
> To clarify: Are you of the opinion that your patch (on top of my base
> patch) is sufficient to make all of this work on Windows?

Things will work. I just tested again each test target with the patch
attached while wrapping again my mind on this stuff (actually found
one bug in plcheck where --bindir was not correct, and removed
tmp_install in upgradecheck). Now, what this patch does is enforcing
the temporary install for each *check target of vcregress.pl. This has
the disadvantage of making the benefits of MAKELEVEL=0 seen for build
methods using the Makefiles go away for MSVC, but it minimizes the use
of ENV variables which is a good thing to me by setting --bindir to a
value as much as possible (ecpgcheck being an exception), and it makes
the set of tests more consistent with each other in the way they run.
Another thing to know that this patch changes is that vcregress does
not rely anymore on the contents of Release/$projects, aka the build
structure of MS stuff, but just fetches binaries from the temporary
installation. That's more consistent with Makefile builds, now perhaps
some people have a different opinion.

What we could add later on a new target, allcheck, that would kick all
the tests at once and installs the temporary installation just once.
It would be straight-forward to write a patch, but this is a separate
discussion as installcheck would need to be skipped. isolationcheck
should as well be changed to be more self-dependent as even of HEAD it
needs to have an instance of PG running to work.
Regards,
--
Michael

On Sat, Apr 11, 2015 at 8:48 PM, Michael Paquier wrote:
> Now, what this patch does is enforcing
> the temporary install for each *check target of vcregress.pl. This has
> the disadvantage of making the benefits of MAKELEVEL=0 seen for build
> methods using the Makefiles go away for MSVC

A trick that we could use here is to store a timestamp when running up
to completion "build" and the temporary installation in vcregress, and
skip the temporary installation if timestamp of vcregress' temporary
installation is newer than the one of "build".
-- 
Michael

On 4/23/15 1:22 PM, Jeff Janes wrote:
> Something about this commit (dcae5faccab64776376d354d) broke "make
> check" in parallel conditions when started from a clean directory.  It
> fails with a different error each time, one example:
> 
> make -j4 check > /dev/null
> 
> In file included from gram.y:14515:
> scan.c: In function 'yy_try_NUL_trans':
> scan.c:10307: warning: unused variable 'yyg'
> /usr/bin/ld: tab-complete.o: No such file: No such file or directory
> collect2: ld returned 1 exit status
> make[3]: *** [psql] Error 1
> make[2]: *** [all-psql-recurse] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [all-bin-recurse] Error 2
> make: *** [all-src-recurse] Error 2
> make: *** Waiting for unfinished jobs....

I think the problem is that "check" depends on "all", but now also
depends on temp-install, which in turn runs install and all.  With a
sufficient amount of parallelism, you end up running two "all"s on top
of each other.

It seems this can be fixed by removing the check: all dependency.  Try
removing that in the top-level GNUmakefile.in and see if the problem
goes away.  For completeness, we should then also remove it in the other
makefiles.

On Sat, Apr 25, 2015 at 7:59 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 4/23/15 1:22 PM, Jeff Janes wrote:
>> Something about this commit (dcae5faccab64776376d354d) broke "make
>> check" in parallel conditions when started from a clean directory.  It
>> fails with a different error each time, one example:
>>
>> make -j4 check > /dev/null
>>
>> In file included from gram.y:14515:
>> scan.c: In function 'yy_try_NUL_trans':
>> scan.c:10307: warning: unused variable 'yyg'
>> /usr/bin/ld: tab-complete.o: No such file: No such file or directory
>> collect2: ld returned 1 exit status
>> make[3]: *** [psql] Error 1
>> make[2]: *** [all-psql-recurse] Error 2
>> make[2]: *** Waiting for unfinished jobs....
>> make[1]: *** [all-bin-recurse] Error 2
>> make: *** [all-src-recurse] Error 2
>> make: *** Waiting for unfinished jobs....
>
> I think the problem is that "check" depends on "all", but now also
> depends on temp-install, which in turn runs install and all.  With a
> sufficient amount of parallelism, you end up running two "all"s on top
> of each other.
>
> It seems this can be fixed by removing the check: all dependency.  Try
> removing that in the top-level GNUmakefile.in and see if the problem
> goes away.  For completeness, we should then also remove it in the other
> makefiles.

Yep. I spent some time yesterday and today poking at that, but I
clearly missed that dependency.. Attached is a patch fixing the
problem. I tested check and check-world with some parallel jobs and
both worked. In the case of check, the amount of logs is very reduced
because all the install process is done by temp-install which
redirects everything into tmp_install/log/install.log.
--
Michael

On Tue, Apr 28, 2015 at 1:46 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
> On Sat, Apr 25, 2015 at 7:23 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Sat, Apr 25, 2015 at 7:59 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
>> > On 4/23/15 1:22 PM, Jeff Janes wrote:
>> >> Something about this commit (dcae5faccab64776376d354d) broke "make
>> >> check" in parallel conditions when started from a clean directory.  It
>> >> fails with a different error each time, one example:
>> >>
>> >> make -j4 check > /dev/null
>> >>
>> >> In file included from gram.y:14515:
>> >> scan.c: In function 'yy_try_NUL_trans':
>> >> scan.c:10307: warning: unused variable 'yyg'
>> >> /usr/bin/ld: tab-complete.o: No such file: No such file or directory
>> >> collect2: ld returned 1 exit status
>> >> make[3]: *** [psql] Error 1
>> >> make[2]: *** [all-psql-recurse] Error 2
>> >> make[2]: *** Waiting for unfinished jobs....
>> >> make[1]: *** [all-bin-recurse] Error 2
>> >> make: *** [all-src-recurse] Error 2
>> >> make: *** Waiting for unfinished jobs....
>> >
>> > I think the problem is that "check" depends on "all", but now also
>> > depends on temp-install, which in turn runs install and all.  With a
>> > sufficient amount of parallelism, you end up running two "all"s on top
>> > of each other.
>> >
>> > It seems this can be fixed by removing the check: all dependency.  Try
>> > removing that in the top-level GNUmakefile.in and see if the problem
>> > goes away.  For completeness, we should then also remove it in the other
>> > makefiles.
>>
>> Yep. I spent some time yesterday and today poking at that, but I
>> clearly missed that dependency.. Attached is a patch fixing the
>> problem. I tested check and check-world with some parallel jobs and
>> both worked. In the case of check, the amount of logs is very reduced
>> because all the install process is done by temp-install which
>> redirects everything into tmp_install/log/install.log.
>
>
> This change fixed the problem for me.
>
> It also made this age-old compiler warning go away:
>
> In file included from gram.y:14515:
> scan.c: In function 'yy_try_NUL_trans':
> scan.c:10307: warning: unused variable 'yyg'
>
> I guess by redirecting it into the log file you indicated, but is that a
> good idea to redirect stderr?

I am sure that Peter did that on purpose, both approaches having
advantages and disadvantages. Personally I don't mind looking at the
install log file in tmp_install to see the state of the installation,
but it is true that this change is a bit disturbing regarding the fact
that everything was directly outputted to stderr and stdout for many
years.
-- 
Michael

On 4/28/15 9:09 AM, Michael Paquier wrote:
>> I guess by redirecting it into the log file you indicated, but is that a
>> > good idea to redirect stderr?
> I am sure that Peter did that on purpose, both approaches having
> advantages and disadvantages. Personally I don't mind looking at the
> install log file in tmp_install to see the state of the installation,
> but it is true that this change is a bit disturbing regarding the fact
> that everything was directly outputted to stderr and stdout for many
> years.

Not really.  Before, pg_regress put the output of its internal make
install run into a log file.  Now make is just replicating that
behavior.  I would agree that that seems kind of obsolete now, because
it's really just another sub-make.  So if no one disagrees, I'd just
remove the log file redirection in temp-install.

Michael Paquier <michael.paquier@gmail.com> writes:
> On Tue, Apr 28, 2015 at 1:46 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
>> This change fixed the problem for me.
>> It also made this age-old compiler warning go away:
>> 
>> In file included from gram.y:14515:
>> scan.c: In function 'yy_try_NUL_trans':
>> scan.c:10307: warning: unused variable 'yyg'
>> 
>> I guess by redirecting it into the log file you indicated, but is that a
>> good idea to redirect stderr?

> I am sure that Peter did that on purpose, both approaches having
> advantages and disadvantages. Personally I don't mind looking at the
> install log file in tmp_install to see the state of the installation,
> but it is true that this change is a bit disturbing regarding the fact
> that everything was directly outputted to stderr and stdout for many
> years.

Hm.  I don't really like the idea that "make all" behaves differently
if invoked by "make check" than if invoked directly.  Hiding warnings
from you is not very good, and hiding errors would be even more annoying.
        regards, tom lane