Re: improving speed of make check-world - Mailing list pgsql-hackers

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.