pg.ddx.io pgsql-hackers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
Race to build pg_isolation_regress in "make -j check-world" 4+ messages / 1 participants [nested] [flat]
* Race to build pg_isolation_regress in "make -j check-world" @ 2017-11-06 08:07 Noah Misch <noah@leadboat.com> 2017-12-17 04:06 ` Re: Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> 0 siblings, 1 reply; 4+ messages in thread From: Noah Misch @ 2017-11-06 08:07 UTC (permalink / raw) To: pgsql-hackers I've been enjoying the speed of parallel check-world, but I get spurious failures from makefile race conditions. Commit c66b438 fixed the simple ones. More tricky is this problem of multiple "make" processes entering src/test/regress concurrently, which causes failures like these: gcc: error: pg_regress.o: No such file or directory make[4]: *** [pg_isolation_regress] Error 1 /bin/sh: ../../../src/test/isolation/pg_isolation_regress: Permission denied make -C test_extensions check make[2]: *** [check] Error 126 make[2]: Leaving directory `/home/nm/src/pg/backbranch/10/src/test/isolation' /bin/sh: ../../../../src/test/isolation/pg_isolation_regress: Text file busy make[3]: *** [isolationcheck] Error 126 make[3]: Leaving directory `/home/nm/src/pg/backbranch/10/src/test/modules/snapshot_too_old' This is reproducible since commit 2038bf4 or earlier; "make -j check-world" had worse problems before that era. A workaround is to issue "make -j; make -j -C src/test/isolation" before the check-world. This problem doesn't affect src/test/regress/pg_regress. Every top-level "make" or "make install", including temp-install, builds pg_regress. I tried fixing this by building src/test/isolation at the same times we run install-temp. Naturally, that didn't help installcheck-world. It also caused multiple "make" processes to enter src/port concurrently. I could fix both check-world and installcheck-world with the attached hack of building src/test/isolation during every top-level build or install. 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. For now, I propose back-patching the attached, sad hack. Better ideas? Thanks, nm diff --git a/src/Makefile b/src/Makefile index 380da92..febbced 100644 --- a/src/Makefile +++ b/src/Makefile @@ -28,6 +28,7 @@ SUBDIRS = \ pl \ makefiles \ test/regress \ + test/isolation \ test/perl # There are too many interdependencies between the subdirectories, so diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile index 8eb4969..efbdc40 100644 --- a/src/test/isolation/Makefile +++ b/src/test/isolation/Makefile @@ -15,6 +15,13 @@ OBJS = specparse.o isolationtester.o $(WIN32RES) all: isolationtester$(X) pg_isolation_regress$(X) +# Though we don't install these binaries, build them during installation +# (including temp-install). Otherwise, "make -j check-world" and "make -j +# installcheck-world" would spawn multiple, concurrent builds in this +# directory. Later builds would overwrite files while earlier builds are +# reading them, causing occasional failures. +install: | all + submake-regress: $(MAKE) -C $(top_builddir)/src/test/regress pg_regress.o -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Attachments: [text/plain] isolation-build-v1.patch (1012B, ../../20171106080752.GA1298146@rfd.leadboat.com/2-isolation-build-v1.patch) download | inline diff: diff --git a/src/Makefile b/src/Makefile index 380da92..febbced 100644 --- a/src/Makefile +++ b/src/Makefile @@ -28,6 +28,7 @@ SUBDIRS = \ pl \ makefiles \ test/regress \ + test/isolation \ test/perl # There are too many interdependencies between the subdirectories, so diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile index 8eb4969..efbdc40 100644 --- a/src/test/isolation/Makefile +++ b/src/test/isolation/Makefile @@ -15,6 +15,13 @@ OBJS = specparse.o isolationtester.o $(WIN32RES) all: isolationtester$(X) pg_isolation_regress$(X) +# Though we don't install these binaries, build them during installation +# (including temp-install). Otherwise, "make -j check-world" and "make -j +# installcheck-world" would spawn multiple, concurrent builds in this +# directory. Later builds would overwrite files while earlier builds are +# reading them, causing occasional failures. +install: | all + submake-regress: $(MAKE) -C $(top_builddir)/src/test/regress pg_regress.o ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Race to build pg_isolation_regress in "make -j check-world" 2017-11-06 08:07 Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> @ 2017-12-17 04:06 ` Noah Misch <noah@leadboat.com> 2018-01-02 01:39 ` Re: Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> 0 siblings, 1 reply; 4+ messages in thread From: Noah Misch @ 2017-12-17 04:06 UTC (permalink / raw) To: pgsql-hackers On Mon, Nov 06, 2017 at 12:07:52AM -0800, Noah Misch wrote: > I've been enjoying the speed of parallel check-world, but I get spurious > failures from makefile race conditions. Commit c66b438 fixed the simple ones. > More tricky is this problem of multiple "make" processes entering > src/test/regress concurrently, which causes failures like these: > > gcc: error: pg_regress.o: No such file or directory > make[4]: *** [pg_isolation_regress] Error 1 > > /bin/sh: ../../../src/test/isolation/pg_isolation_regress: Permission denied > make -C test_extensions check > make[2]: *** [check] Error 126 > make[2]: Leaving directory `/home/nm/src/pg/backbranch/10/src/test/isolation' > > /bin/sh: ../../../../src/test/isolation/pg_isolation_regress: Text file busy > make[3]: *** [isolationcheck] Error 126 > make[3]: Leaving directory `/home/nm/src/pg/backbranch/10/src/test/modules/snapshot_too_old' > > This is reproducible since commit 2038bf4 or earlier; "make -j check-world" > had worse problems before that era. A workaround is to issue "make -j; make > -j -C src/test/isolation" before the check-world. Commit de0aca6 fixed that problem, but 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. If one is willing to assume that a lock-holding process never crashes, locking in a shell script is simple: mkdir to lock, rmdir to unlock. I don't want to assume that. The bakery algorithm provides convenient opportunities for checking whether the last locker crashed; I have attached a shell script demonstrating this approach. Better ideas? Otherwise, I'll look into integrating this design into the makefiles. Thanks, nm Attachments: [application/x-sh] bakery.sh (1.5K, ../../20171217040605.GA2421254@rfd.leadboat.com/2-bakery.sh) download ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Race to build pg_isolation_regress in "make -j check-world" 2017-11-06 08:07 Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> 2017-12-17 04:06 ` Re: Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> @ 2018-01-02 01:39 ` Noah Misch <noah@leadboat.com> 2018-12-24 22:16 ` Re: Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> 0 siblings, 1 reply; 4+ messages in thread From: Noah Misch @ 2018-01-02 01:39 UTC (permalink / raw) To: pgsql-hackers 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. > > If one is willing to assume that a lock-holding process never crashes, locking > in a shell script is simple: mkdir to lock, rmdir to unlock. I don't want to > assume that. The bakery algorithm provides convenient opportunities for > checking whether the last locker crashed; I have attached a shell script > demonstrating this approach. Better ideas? Otherwise, I'll look into > integrating this design into the makefiles. 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: 1. Replace the shell script implementation of the bakery algorithm with a C program that issues fcntl(F_SETLK) on the target directory's makefile. 2. Stop re-entering widely-referenced directories like src/common and src/port dozens of times per build. Instead, enter them before any "make -C", then assume they're built if $(MAKELEVEL) is nonzero. This reduced the total number of "make -C" calls and lock acquisitions from 276 to 147. 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. If I abandon (3), an empty build in an NFS-mounted source directory slows from 6.4s to 22s; with all three design changes intact, it slows to 6.6s. I think that justifies keeping (3). I considered abandoning (1). An empty build would then slow from 6.6s to 7.7s on NFS-mounted sources, and it would slow from 2.1s to 6.8s on Cygwin. (I expect similar on MSYS.) That is perhaps acceptable. It would save a few lines of code and perhaps avoid portability snags. Unlike abandoning (3), I would not expect long-term maintenance savings. For systems with low PID entropy[2], (1) also eliminates unreliable code for detecting that a lock holder has died. I plan to keep all three design changes. The non-performance problems that arose were easy to fix: a. In addition to avoiding overlapping "make install" invocations in each directory, we must ensure no directory is installed more than once per tmp_install. Otherwise, the second "make install" could unlink or overwrite a file while another process runs a test that uses the file. I fixed this with stamp files like we use elsewhere in Makefile.global. b. Our "make -C" call graph has cycles[3]. To avoid deadlocks, I accumulated lock-holding PIDs in an environment variable. If the holder of a desired lock appears in that variable, then that holder is sleeping until after the present process completes. We can then proceed without a lock acquisition. Comments? Otherwise, I'll look at finishing the patch series. Thanks, nm [1] I was surprised to see that GNU make is this efficient at determining there's nothing to rebuild. [2] For example, Cygwin reuses PIDs as often as every 645 processes. Also, Cygwin processes have both a Cygwin PID and a Windows PID, and kill() can match either PID. The shell script relied on kill(PID, 0) to deduce that a lock holder had died. We may have ended up needing a secondary factor, like process start time, on such systems. [3] For example, src/backend depends on libpgport_srv.a, and src/port depends on submake-errcodes of src/backend. ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Race to build pg_isolation_regress in "make -j check-world" 2017-11-06 08:07 Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> 2017-12-17 04:06 ` Re: Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> 2018-01-02 01:39 ` Re: Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> @ 2018-12-24 22:16 ` Noah Misch <noah@leadboat.com> 0 siblings, 0 replies; 4+ messages in thread From: Noah Misch @ 2018-12-24 22:16 UTC (permalink / raw) To: pgsql-hackers 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 --- 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 Attachments: [text/plain] extra_install-serial-v1.patch (2.5K, ../../20181224221601.GA3227827@rfd.leadboat.com/2-extra_install-serial-v1.patch) download | inline diff: --- 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 ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2018-12-24 22:16 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2017-11-06 08:07 Race to build pg_isolation_regress in "make -j check-world" Noah Misch <noah@leadboat.com> 2017-12-17 04:06 ` Noah Misch <noah@leadboat.com> 2018-01-02 01:39 ` Noah Misch <noah@leadboat.com> 2018-12-24 22:16 ` Noah Misch <noah@leadboat.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox