Fixing parallel make of libpq

On Wed, Jan 08, 2020 at 01:33:13PM +0000, Dagfinn Ilmari Mannsåker wrote:

I noticed to my annoyance that 'make -j4 -C src/interfaces/libpq'
doesn't work in a clean checkout, because it can't find
libpgcommon_shlib and libpgport_shlib. It looks like that's because the
submake-libpgport dependency is declared on the all-lib target, not on
the shlib itself. Moving it to SHLIB_PREREQS instead fixes it, patch
for which is attached.

Hmm. That logically makes sense. Isn't that a side effect of 7143b3e
then? Now, FWIW, I am not able to reproduce it here, after trying on
two different machines, various parallel job numbers (up to 32), and a
couple of dozen attempts. Perhaps somebody else can see the failures?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Wed, Jan 08, 2020 at 01:33:13PM +0000, Dagfinn Ilmari Mannsåker wrote:

I noticed to my annoyance that 'make -j4 -C src/interfaces/libpq'
doesn't work in a clean checkout, because it can't find
libpgcommon_shlib and libpgport_shlib. It looks like that's because the
submake-libpgport dependency is declared on the all-lib target, not on
the shlib itself. Moving it to SHLIB_PREREQS instead fixes it, patch
for which is attached.

Hmm. That logically makes sense. Isn't that a side effect of 7143b3e
then? Now, FWIW, I am not able to reproduce it here, after trying on
two different machines, various parallel job numbers (up to 32), and a
couple of dozen attempts. Perhaps somebody else can see the failures?

It fails reliably for me on Debian Buster, with make 4.2.1-1.2and -j4.
The command to reproduce it is:

git clean -xfd && ./configure && make -Otarget -j4 -C src/interfaces/libpq/

Attached is the output of the `make` step with and without the patch.

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl

ilmari@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:

Michael Paquier <michael@paquier.xyz> writes:

Hmm. That logically makes sense. Isn't that a side effect of 7143b3e
then? Now, FWIW, I am not able to reproduce it here, after trying on
two different machines, various parallel job numbers (up to 32), and a
couple of dozen attempts. Perhaps somebody else can see the failures?

It fails reliably for me on Debian Buster, with make 4.2.1-1.2and -j4.

Yeah, it's also reliable for me on Fedora 30:

$ make -s clean
$ make -s -j4 -C src/interfaces/libpq
/usr/bin/ld: cannot find -lpgcommon_shlib
/usr/bin/ld: cannot find -lpgport_shlib
collect2: error: ld returned 1 exit status
make: *** [../../../src/Makefile.shlib:293: libpq.so.5.13] Error 1
make: *** Waiting for unfinished jobs....

On a RHEL6 box, the same test only draws a complaint about
-lpgcommon_shlib, so it does seem like there's some make version
dependency in here. And of course the whole thing is a race condition
anyway, so naturally it's going to be pretty context-sensitive.

My thoughts about the patch:

1) Changing from an "|"-style dependency to a plain dependency seems
like a semantics change. I've never been totally clear on the
difference though. I think Peter introduced our use of the "|" style,
so maybe he can comment.

2) The same coding pattern is used in a bunch of other places, so if
this spot is broken, there probably are a lot of others that need a
similar change. On the other hand, there may not be that many
directories that are likely places to start a parallel build from,
so maybe we don't care elsewhere.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

My thoughts about the patch:

1) Changing from an "|"-style dependency to a plain dependency seems
like a semantics change. I've never been totally clear on the
difference though. I think Peter introduced our use of the "|" style,
so maybe he can comment.

Makefile.shlib puts $(SHLIB_PREREQS) after the "|":

$ grep SHLIB_PREREQS src/Makefile.shlib
# SHLIB_PREREQS Order-only prerequisites for library build target
$(stlib): $(OBJS) | $(SHLIB_PREREQS)
$(shlib): $(OBJS) | $(SHLIB_PREREQS)
$(shlib): $(OBJS) | $(SHLIB_PREREQS)
$(shlib): $(OBJS) | $(SHLIB_PREREQS)
$(shlib): $(OBJS) | $(SHLIB_PREREQS)
$(shlib): $(OBJS) $(DLL_DEFFILE) | $(SHLIB_PREREQS)

2) The same coding pattern is used in a bunch of other places, so if
this spot is broken, there probably are a lot of others that need a
similar change. On the other hand, there may not be that many
directories that are likely places to start a parallel build from,
so maybe we don't care elsewhere.

Grepping the Makefiles for ':.*submake-' shows that they are on the
actual build artifact target, libpq was just the outlier having it on
the phony "all" target. For example pg_basebackup:

pg_basebackup: pg_basebackup.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) pg_basebackup.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)

pg_receivewal: pg_receivewal.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) pg_receivewal.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)

pg_recvlogical: pg_recvlogical.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) pg_recvlogical.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)

- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen

On 2020-01-09 15:17, Tom Lane wrote:

1) Changing from an "|"-style dependency to a plain dependency seems
like a semantics change. I've never been totally clear on the
difference though. I think Peter introduced our use of the "|" style,
so maybe he can comment.

If you have a phony target as a prerequisite of a real-file target, you
should make that an order-only ("|") prerequisite. Otherwise the
real-file target rules will *always* be run, on account of the phony
target prerequisite.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2020-01-09 15:17, Tom Lane wrote:

1) Changing from an "|"-style dependency to a plain dependency seems
like a semantics change. I've never been totally clear on the
difference though. I think Peter introduced our use of the "|" style,
so maybe he can comment.

If you have a phony target as a prerequisite of a real-file target, you
should make that an order-only ("|") prerequisite. Otherwise the
real-file target rules will *always* be run, on account of the phony
target prerequisite.

OK, got that. But that doesn't directly answer the question of whether
it's wrong to use a phony target as an order-only prerequisite of
another phony target. Grepping around for other possible issues,
I see that you recently added

update-unicode: | submake-generated-headers submake-libpgport
$(MAKE) -C src/common/unicode $@
$(MAKE) -C contrib/unaccent $@

Doesn't that also have parallel-make hazards, if libpq does?

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

OK, got that. But that doesn't directly answer the question of whether
it's wrong to use a phony target as an order-only prerequisite of
another phony target. Grepping around for other possible issues,
I see that you recently added

update-unicode: | submake-generated-headers submake-libpgport
$(MAKE) -C src/common/unicode $@
$(MAKE) -C contrib/unaccent $@

Doesn't that also have parallel-make hazards, if libpq does?

The part of 'update-unicode' that needs the generated headers and
libpgport is src/common/unicode/norm_test, which is depended by on by
the normalization-check target in the same directory. Running 'make -C
src/common/unicode normalization-check' in a freshly-configured tree
does indeed fail, independent of parallelism or the update-unicode
target.

Adding the deps to the norm_test target fixes 'make -C
src/common/unicode normalization-check', but 'make -C src/common/unicode
update-unicode' still fails, because submake-generated-headers only does
its thing in the top-level make invocation, so that needs an explicit
dep as well.

Please find a patch attached. However, I don't think it's fair to block
fixing the actual libpq parallel-make bug that has bitten me several
times on auditing the entire source tree for vaguely related issues that
nobody has complained about yet.

- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen