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.
 Command was: "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" 
 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)

  # actual new tmp installation
        $(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
  TMP_INSTALL = .tmp_install
  TMP_INSTALL = .tmp_install.$(MAKE_NONCE)

  .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.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to