Date: Mon, 24 Dec 2018 14:16:01 -0800
From: Noah Misch <noah@leadboat.com>
To: pgsql-hackers@postgresql.org
Subject: Re: Race to build pg_isolation_regress in "make -j check-world"
Message-ID: <20181224221601.GA3227827@rfd.leadboat.com>
References: <20171106080752.GA1298146@rfd.leadboat.com>
 <20171217040605.GA2421254@rfd.leadboat.com>
 <20180102013956.GA898314@rfd.leadboat.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="wac7ysb48OaltWcw"
Content-Disposition: inline
In-Reply-To: <20180102013956.GA898314@rfd.leadboat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--wac7ysb48OaltWcw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Jan 01, 2018 at 05:39:56PM -0800, Noah Misch wrote:
> On Sat, Dec 16, 2017 at 08:06:05PM -0800, Noah Misch wrote:
> > On Mon, Nov 06, 2017 at 12:07:52AM -0800, Noah Misch wrote:
> 
> > I now see similar trouble from multiple
> > "make" processes running "make -C contrib/test_decoding install" concurrently.
> > This is a risk for any directory named in an EXTRA_INSTALL variable of more
> > than one makefile.  Under the right circumstances, this would affect
> > contrib/hstore and others in addition to contrib/test_decoding.  That brings
> > me back to the locking idea:
> > 
> > > The problem of multiple "make" processes in a directory (especially src/port)
> > > shows up elsewhere.  In a cleaned tree, "make -j -C src/bin" or "make -j
> > > installcheck-world" will do it.  For more-prominent use cases, src/Makefile
> > > prevents this with ".NOTPARALLEL:" and building first the directories that are
> > > frequent submake targets.  Perhaps we could fix the general problem with
> > > directory locking; targets that call "$(MAKE) -C FOO" would first sleep until
> > > FOO's lock is available.  That could be tricky to make robust.

> Performance has been the principal snare.  I value "make -j" being fast when
> there's little to rebuild, but that shell script approach slowed an empty
> build by 340% (GNU/Linux w/ SSD) to 2300% (Cygwin).  In a build having nothing
> to do, merely adding a no-op wrapper around "make -C" (does nothing but
> execvp() the real GNU make) slowed the build by over 10%[1].  To get what I
> considered decent performance took several design changes:

> 3. Lock only directories known to be entered more than once per top-level
>    target.  Preliminarily, this reduced the 147 lock acquisitions to 25.
> 
> I regret that (3) entails ongoing maintenance of a whitelist of such
> directories; adding a "make -C" call can expand the list.  However, I can
> automate verification of that whitelist.

As I developed this more, I found it dissatisfying.  To verify the whitelist,
one empties the whitelist, rebuilds each makefile target, and reassembles the
whitelist from the list of directories visited more than once within a target.
That's too expensive to run all the time (e.g. with every "make check").  I
could make a buildfarm animal turn red when the whitelist is out of date, but
I wouldn't enjoy discovering or fixing whitelist drift that way.  Hence, I'm
back to preferring fixes targeting the known problems:

- Fix conflicting writes in src/test/regress:
  https://postgr.es/m/flat/20181224034411.GA3224776%40rfd.leadboat.com

- Fix conflicting EXTRA_INSTALL, detailed above.  In the MAKELEVEL-0 part of
  the temp-install Makefile target, serially process all applicable
  EXTRA_INSTALL.  See attached patch.  On my usual development system, this
  adds <2s to "make check-world" and <0.1s to "make check" or "make -C
  contrib/earthdistance check".

To corroborate the sufficiency of those two patches, I ran 110 iterations of
"make -j40" and "make -j40 check-world" without encountering a failure
attributable to parallel make.  I also ran those targets at excess parallelism
(-j120) and audited the list of commands appearing more than once:

  make clean && make -j120 2>&1 | sort | uniq -c | sort -rn
  make -j120 check-world 2>&1   | sort | uniq -c | sort -rn

Unlike the locking method, this doesn't fix clean-tree "make -j -C src/bin" or
clean-tree "make -j installcheck-world".

Thanks,
nm

--wac7ysb48OaltWcw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="extra_install-serial-v1.patch"

--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -63,10 +63,12 @@ distclean maintainer-clean:
 	@rm -rf autom4te.cache/
 	rm -f config.cache config.log config.status GNUmakefile
 
+check check-tests installcheck installcheck-parallel installcheck-tests: CHECKPREP_TOP=src/test/regress
 check check-tests installcheck installcheck-parallel installcheck-tests: submake-generated-headers
 	$(MAKE) -C src/test/regress $@
 
 $(call recurse,check-world,src/test src/pl src/interfaces/ecpg contrib src/bin,check)
+$(call recurse,checkprep,  src/test src/pl src/interfaces/ecpg contrib src/bin)
 
 $(call recurse,installcheck-world,src/test src/pl src/interfaces/ecpg contrib src/bin,installcheck)
 
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -19,7 +19,7 @@
 #
 # Meta configuration
 
-standard_targets = all install installdirs uninstall distprep clean distclean maintainer-clean coverage check installcheck init-po update-po
+standard_targets = all install installdirs uninstall distprep clean distclean maintainer-clean coverage check checkprep installcheck init-po update-po
 # these targets should recurse even into subdirectories not being built:
 standard_always_targets = distprep clean distclean maintainer-clean
 
@@ -390,11 +390,17 @@ ifeq ($(MAKELEVEL),0)
 	rm -rf '$(abs_top_builddir)'/tmp_install
 	$(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
 	$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
+	$(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep >>'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
 endif
-	$(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C '$(top_builddir)'/$$extra DESTDIR='$(abs_top_builddir)'/tmp_install install >>'$(abs_top_builddir)'/tmp_install/log/install.log || exit; done)
 endif
 endif
 
+# Tasks to run serially at the end of temp-install.  Some EXTRA_INSTALL
+# entries appear more than once in the tree, and parallel installs of the same
+# file can fail with EEXIST.
+checkprep:
+	$(if $(EXTRA_INSTALL),for extra in $(EXTRA_INSTALL); do $(MAKE) -C '$(top_builddir)'/$$extra DESTDIR='$(abs_top_builddir)'/tmp_install install || exit; done)
+
 PROVE = @PROVE@
 # There are common routines in src/test/perl, and some test suites have
 # extra perl modules in their own directory.
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -435,7 +435,7 @@ endif
 endif # PGXS
 
 ifndef NO_TEMP_INSTALL
-temp-install: EXTRA_INSTALL+=$(subdir)
+checkprep: EXTRA_INSTALL+=$(subdir)
 endif
 
 
--wac7ysb48OaltWcw--