Thread: Re: Patch for Makefile race against current cvs

Re: Patch for Makefile race against current cvs

From
Klaus Naumann
Date:
On Fri, 9 Nov 2001, Tom Lane wrote:

> Klaus Naumann <knaumann@gmx-ag.de> writes:
> > It is ... the problem is that with the current Makefile there
> > are always two files which need the .y files
> > (i.e. preproc.c and preproc.h). If you now do a make -j3,
> > make will start things in parallel.
>
> But they're generated by the *same action*.  If make runs the same
> action twice in parallel, it's make that is broken.  I think your
> gripe is directed to the wrong place.

I don't have any gripe ! I just wanted to help a good (at least
as I thought) project ...

About the other thing:
This is GNU Make version 3.79.1, by Richard Stallman and Roland McGrath.
On Debian/Woody Linux. So well, this make runs these things in
parallel.
Also even if it would be make's fault I don't see what my patch makes
worse. But if you don't want to apply it, you don't apply it.

    Later, Klaus

--
Klaus Naumann
Systems Administration
GMX  Aktiengesellschaft
Riesstrasse 17, 80992 München
Telefon +49.89.143 39-0
Telefax +49.89.143 39-100
mailto:knaumann@gmx-ag.de
http://www.gmx.net/


Re: Patch for Makefile race against current cvs

From
Tom Lane
Date:
Klaus Naumann <knaumann@gmx-ag.de> writes:
> Also even if it would be make's fault I don't see what my patch makes
> worse. But if you don't want to apply it, you don't apply it.

Well, as to whether it gets applied or not, I'll defer to Peter
Eisentraut who has done most of the work recently on our configure and
make support.  The reason I'm asking all these questions is that I
want to understand what the problem really is.  It seems to me that if
we have a problem with these bison invocations then we are likely to
have similar problems elsewhere.  We need to understand why it's
unsafe and what the general rule is for avoiding such mistakes in
future.

What bothers me is that you seem to be saying that *any* construct
involving multiple outputs from one rule is unsafe in a parallel make.
That strikes me as a huge restriction, and one that would surely be
mentioned prominently in the gmake manual if it were real.  But I can't
find anything that says that.

I think what you are looking at here is a gmake bug, and that you should
report it to the gmake people.

            regards, tom lane

Re: Patch for Makefile race against current cvs

From
Klaus Naumann
Date:
On Fri, 9 Nov 2001, Tom Lane wrote:

> Klaus Naumann <knaumann@gmx-ag.de> writes:
> > Also even if it would be make's fault I don't see what my patch makes
> > worse. But if you don't want to apply it, you don't apply it.
>
> Well, as to whether it gets applied or not, I'll defer to Peter
> Eisentraut who has done most of the work recently on our configure and
> make support.  The reason I'm asking all these questions is that I
> want to understand what the problem really is.  It seems to me that if
> we have a problem with these bison invocations then we are likely to
> have similar problems elsewhere.  We need to understand why it's
> unsafe and what the general rule is for avoiding such mistakes in
> future.

To me it looks like make is calling the target twice as there are
two dependent files.
I think this is due to a dependency of a dependency.
Lemme explain: What I think is, that we enter a directory and make
a parallel build. We want to compile prepare.c , so make calls the
rule to build prepare.c . A second make (because it's parallel)
sees a file which needs prepare.h, so it calls the rule.
At that point the rule is called twice. If it's now doing things
which are not supposed to work in parallel (like moving away
a file) the build fails.
Maybe this is completely wrong - but this is what it looks
like to me ...

> What bothers me is that you seem to be saying that *any* construct
> involving multiple outputs from one rule is unsafe in a parallel make.

I didn't say that - I just was refering to this special case.
It happened to me - more then one time. Even about 20 times
(while creating the patch ...)

> That strikes me as a huge restriction, and one that would surely be
> mentioned prominently in the gmake manual if it were real.  But I can't
> find anything that says that.

I agree, and I don't think it's a problem which happens always.

> I think what you are looking at here is a gmake bug, and that you should
> report it to the gmake people.

Meybe we should ask them - but somehow I have the impression that
it's not a gmake problem ...

    CU, Klaus

--
Klaus Naumann
Systems Administration
GMX  Aktiengesellschaft
Riesstrasse 17, 80992 München
Telefon +49.89.143 39-0
Telefax +49.89.143 39-100
mailto:knaumann@gmx-ag.de
http://www.gmx.net/


Re: Patch for Makefile race against current cvs

From
Tom Lane
Date:
Klaus Naumann <knaumann@gmx-ag.de> writes:
> To me it looks like make is calling the target twice as there are
> two dependent files.
> I think this is due to a dependency of a dependency.
> Lemme explain: What I think is, that we enter a directory and make
> a parallel build. We want to compile prepare.c , so make calls the
> rule to build prepare.c . A second make (because it's parallel)
> sees a file which needs prepare.h, so it calls the rule.
> At that point the rule is called twice. If it's now doing things
> which are not supposed to work in parallel (like moving away
> a file) the build fails.

Hmm.  It sounds to me like the issue is not the dual-output rule as
such; it's that we have dependencies on one of the output files from
elsewhere in the source tree, and so we end up with multiple sub-makes
entering the backend/parser directory in parallel to build that header
file, and then it's broken because the rule is not safe for parallel
execution (quite independently of whether it has one or two output
files).

But that's supposed to be taken care of by having the parse.h file
updated before we start recursing into the rest of the backend --- see
src/backend/Makefile.  Why is that not stopping the problem?

Did you see any actual failures related to the other bison invocations,
or just the one in backend/parser?  AFAIK the other ones generate files
that are only used within their one source directory, and so they should
not have this problem anyway.  There should only be one sub-make looking
at the other ones.

            regards, tom lane

Re: Patch for Makefile race against current cvs

From
Klaus Naumann
Date:
On Fri, 9 Nov 2001, Tom Lane wrote:

> Klaus Naumann <knaumann@gmx-ag.de> writes:
> > To me it looks like make is calling the target twice as there are
> > two dependent files.
> > I think this is due to a dependency of a dependency.
> > Lemme explain: What I think is, that we enter a directory and make
> > a parallel build. We want to compile prepare.c , so make calls the
> > rule to build prepare.c . A second make (because it's parallel)
> > sees a file which needs prepare.h, so it calls the rule.
> > At that point the rule is called twice. If it's now doing things
> > which are not supposed to work in parallel (like moving away
> > a file) the build fails.
>
> Hmm.  It sounds to me like the issue is not the dual-output rule as
> such; it's that we have dependencies on one of the output files from
> elsewhere in the source tree, and so we end up with multiple sub-makes
> entering the backend/parser directory in parallel to build that header
> file, and then it's broken because the rule is not safe for parallel
> execution (quite independently of whether it has one or two output
> files).

This might be also true. But I that far to say, that this happens
within the same directory.
What I mean is, the file which generate the dependencies you are talking
about are in the same directory as the bison files.

> But that's supposed to be taken care of by having the parse.h file
> updated before we start recursing into the rest of the backend --- see
> src/backend/Makefile.  Why is that not stopping the problem?

s.a.

> Did you see any actual failures related to the other bison invocations,
> or just the one in backend/parser?  AFAIK the other ones generate files
> that are only used within their one source directory, and so they should
> not have this problem anyway.  There should only be one sub-make looking
> at the other ones.

Yes, I also have seen races in two other directories
(and decided to change all occorences of the two-file
target in any Makefile because I'm under the impression
that it's to dangerous ...).
I have seen problems in src/backend/bootstrap and in
src/pl/plpgsql/src too.

    Bye, Klaus

--
Klaus Naumann
Systems Administration
GMX  Aktiengesellschaft
Riesstrasse 17, 80992 München
Telefon +49.89.143 39-0
Telefax +49.89.143 39-100
mailto:knaumann@gmx-ag.de
http://www.gmx.net/


Re: Patch for Makefile race against current cvs

From
Tom Lane
Date:
Klaus Naumann <knaumann@gmx-ag.de> writes:
> Yes, I also have seen races in two other directories
> (and decided to change all occorences of the two-file
> target in any Makefile because I'm under the impression
> that it's to dangerous ...).

Now you're back to saying that any two-target rule is unsafe under
parallel make.  I find that hard to believe, given the lack of any
suggestion in the gmake documents that any care needs to be taken
to make things safe for parallel makes.

            regards, tom lane

Re: Patch for Makefile race against current cvs

From
Klaus Naumann
Date:
On Fri, 9 Nov 2001, Tom Lane wrote:

> Klaus Naumann <knaumann@gmx-ag.de> writes:
> > Yes, I also have seen races in two other directories
> > (and decided to change all occorences of the two-file
> > target in any Makefile because I'm under the impression
> > that it's to dangerous ...).
>
> Now you're back to saying that any two-target rule is unsafe under
> parallel make.  I find that hard to believe, given the lack of any
> suggestion in the gmake documents that any care needs to be taken
> to make things safe for parallel makes.

At least the last sentence is just plain wrong. I have seen
and heard of several projects which had to change their
Makefiles to make parallel builds work.
A good example would be the linux kernel.
If you do a make -j3 dep clean bzImage you really f*ck up things
as it's all done in parallel. If you would add a rule

anything: dep clean bzImage

things would get messed up too (I have tried it ...).
I don't count this as a bug in make ...

And no, I'm not saying that any two-target rule is unsafe,
I just said I removed all as I think that it's a good idea
to aviod possible problems which might occur in the future.

    CU, Klaus

--
Klaus Naumann
Systems Administration
GMX  Aktiengesellschaft
Riesstrasse 17, 80992 München
Telefon +49.89.143 39-0
Telefax +49.89.143 39-100
mailto:knaumann@gmx-ag.de
http://www.gmx.net/


Re: Patch for Makefile race against current cvs

From
Peter Eisentraut
Date:
Tom Lane writes:

> What bothers me is that you seem to be saying that *any* construct
> involving multiple outputs from one rule is unsafe in a parallel make.
> That strikes me as a huge restriction, and one that would surely be
> mentioned prominently in the gmake manual if it were real.  But I can't
> find anything that says that.

I think we have been victims of a misunderstanding of how multiple-target
rules work.  The gmake manual says:

"A rule with multiple targets is equivalent to writing many rules, each
with one target, and all identical aside from that."

So when we write this:

| $(srcdir)/preproc.c $(srcdir)/preproc.h: preproc.y
|         $(YACC) -d $(YFLAGS) $<
|         mv y.tab.c $(srcdir)/preproc.c
|         mv y.tab.h $(srcdir)/preproc.h

make reads it as this:

| $(srcdir)/preproc.c: preproc.y
|         $(YACC) -d $(YFLAGS) $<
|         mv y.tab.c $(srcdir)/preproc.c
|         mv y.tab.h $(srcdir)/preproc.h
|
| $(srcdir)/preproc.h: preproc.y
|         $(YACC) -d $(YFLAGS) $<
|         mv y.tab.c $(srcdir)/preproc.c
|         mv y.tab.h $(srcdir)/preproc.h

So it's completely conceivable that both rules get executed in parallel.
(In general, you cannot optimize this away, because the actual commands
may differ depending on the value of $@.)

This basically means that you cannot correctly write a target that
generates more than one file.  The only workaround I see is to cheat about
the dependencies, as Klaus' patch does:

y.tab.c: preproc.y
        $(YACC) -d $(YFLAGS) $<

$(srcdir)/preproc.c: y.tab.c
        mv y.tab.c $(srcdir)/preproc.c

$(srcdir)/preproc.h: y.tab.c
        mv y.tab.h $(srcdir)/preproc.h

It's the latter rule where we're technically cheating and which I was
initially regarding as part of the "details I don't like", but now I think
it's the best (only?) way to go.

--
Peter Eisentraut   peter_e@gmx.net


Re: Patch for Makefile race against current cvs

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> y.tab.c: preproc.y
>         $(YACC) -d $(YFLAGS) $<

> $(srcdir)/preproc.c: y.tab.c
>         mv y.tab.c $(srcdir)/preproc.c

> $(srcdir)/preproc.h: y.tab.c
>         mv y.tab.h $(srcdir)/preproc.h

If you're going to do that, hadn't those last two better be "cp" not
"mv"?

What I find particularly distressing about this is that it will mean
that y.tab.c and y.tab.h have to become part of the shipped distribution
tarball.  That's way ugly.  I'd personally sooner document the existing
restriction: don't use parallel make on CVS sources.

As long as cheating is the order of the day, have you thought about

| $(srcdir)/preproc.c: preproc.y
|         $(YACC) -d $(YFLAGS) $<
|         mv y.tab.c $(srcdir)/preproc.c
|         mv y.tab.h $(srcdir)/preproc.h
|
| $(srcdir)/preproc.h: $(srcdir)/preproc.c

Also, I'm still feeling that we are missing something fundamental about
parallel make.  Surely there's got to be *some* interlock in make that
prevents multiple subjobs from executing the same rule in parallel.
Otherwise there are just too many things that are risky.  I'm not
even convinced that "cp tmpfile targetfile" is totally safe under
such conditions.

            regards, tom lane

Re: Patch for Makefile race against current cvs

From
Tom Lane
Date:
I wrote:
> Also, I'm still feeling that we are missing something fundamental about
> parallel make.  Surely there's got to be *some* interlock in make that
> prevents multiple subjobs from executing the same rule in parallel.

Sigh, I guess my expectations are too high.  Some digging around in
gmake 3.79.1 shows that the submakes coordinate only to the extent
of limiting the *number* of simultaneous child processes, not to the
extent of determining what the children are doing.

I don't think that there is any really bulletproof way to invoke bison
under this sort of scenario: if there are multiple submakes executing
the same build rule then it's entirely likely that we'll see things like
one child installing a y.tab.h file that's been truncated and only
partially rewritten by another child.  Yes, the second child will
eventually make it good, but that's little comfort if the first child
launches compiles that fail meanwhile.

But we could provide some security for multiple children of a single
make by changing the rules to be like

$(srcdir)/parse.h: gram.y
ifdef YACC
    $(YACC) -d $(YFLAGS) $<
    mv y.tab.h $(srcdir)/parse.h
    mv y.tab.c $(srcdir)/gram.c
else
    @$(missing) bison $< $@
endif

$(srcdir)/gram.c: $(srcdir)/parse.h

Comments?

            regards, tom lane

Re: Patch for Makefile race against current cvs

From
Peter Eisentraut
Date:
Tom Lane writes:

> Also, I'm still feeling that we are missing something fundamental about
> parallel make.  Surely there's got to be *some* interlock in make that
> prevents multiple subjobs from executing the same rule in parallel.

There is, but remember that the two-target rule we're looking at is
actually two separate rules written in abbreviated form.  And under that
pretext, the two rules are lying: They don't generate exactly the file
they have as the target.  Unfortunately, there isn't a right way to fix
that.

Keep in mind that parallel make only executes the rule commands in
parallel, it does not cause the dependency analysis to be distributed.
In order to satisfy a dependency graph you only need to process each node
once, and parallelism doesn't change that.

--
Peter Eisentraut   peter_e@gmx.net


Re: Patch for Makefile race against current cvs

From
Peter Eisentraut
Date:
Tom Lane writes:

> But we could provide some security for multiple children of a single
> make by changing the rules to be like
>
> $(srcdir)/parse.h: gram.y
> ifdef YACC
>     $(YACC) -d $(YFLAGS) $<
>     mv y.tab.h $(srcdir)/parse.h
>     mv y.tab.c $(srcdir)/gram.c
> else
>     @$(missing) bison $< $@
> endif
>
> $(srcdir)/gram.c: $(srcdir)/parse.h

That seems to be okay, although I think I'd switch gram.c and parse.h for
stylistic reasons.

--
Peter Eisentraut   peter_e@gmx.net


Re: Patch for Makefile race against current cvs

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> Keep in mind that parallel make only executes the rule commands in
> parallel, it does not cause the dependency analysis to be distributed.
> In order to satisfy a dependency graph you only need to process each node
> once, and parallelism doesn't change that.

Well, the proposed patch eliminates the issue for a single sub-make,
by artificially serializing the execution of these rules.  But I'm still
concerned about what happens when a sub-make running in a different
directory sees a dependency on parse.h.  There *is* distributed
dependency analysis when you think about that scenario.

            regards, tom lane

Re: Patch for Makefile race against current cvs

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> Tom Lane writes:
>> But we could provide some security for multiple children of a single
>> make by changing the rules to be like
>>
>> $(srcdir)/parse.h: gram.y
>> ifdef YACC
>> $(YACC) -d $(YFLAGS) $<
>> mv y.tab.h $(srcdir)/parse.h
>> mv y.tab.c $(srcdir)/gram.c
>> else
>> @$(missing) bison $< $@
>> endif
>>
>> $(srcdir)/gram.c: $(srcdir)/parse.h

> That seems to be okay, although I think I'd switch gram.c and parse.h for
> stylistic reasons.

It's not real important, but I thought it would be a good idea to
minimize the dependencies seen by sub-makes entering the directory from
other backend directories.  Those sub-makes will only be interested in
parse.h, not in gram.c.

            regards, tom lane

Re: Patch for Makefile race against current cvs

From
Peter Eisentraut
Date:
I think we have found an acceptable solution.  But unless Tom already
applied the patches while I write this, I suggest we wait for 7.3devel.

--
Peter Eisentraut   peter_e@gmx.net


Re: Patch for Makefile race against current cvs

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> I think we have found an acceptable solution.  But unless Tom already
> applied the patches while I write this, I suggest we wait for 7.3devel.

Why wait?  Do you think there's a risk of breaking something?  It seemed
a pretty safe change to me.

I'll defer to you to apply the patch in any case.  But unless you see
a risk I would suggest dealing with it now, not later.

            regards, tom lane

Re: Patch for Makefile race against current cvs

From
Bruce Momjian
Date:
> I think we have found an acceptable solution.  But unless Tom already
> applied the patches while I write this, I suggest we wait for 7.3devel.

OK, that's all I needed to hear. If I don't hear something else, I will
throw the entire thread into the patches2 archive and we can sort it out
when we hit 7.3 development.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Re: Patch for Makefile race against current cvs

From
Klaus Naumann
Date:
On Thu, 15 Nov 2001, Bruce Momjian wrote:

> > I think we have found an acceptable solution.  But unless Tom already
> > applied the patches while I write this, I suggest we wait for 7.3devel.
>
> OK, that's all I needed to hear. If I don't hear something else, I will
> throw the entire thread into the patches2 archive and we can sort it out
> when we hit 7.3 development.

Well, it's a bugfix and I don't see what it could break
(wow, Tom and me agree :))) ).
So why wouldn't we want to commit it in ?

    CU, Klaus

--
Klaus Naumann
Systems Administration
GMX  Aktiengesellschaft
Riesstrasse 17, 80992 München
Telefon +49.89.143 39-0
Telefax +49.89.143 39-100
mailto:knaumann@gmx-ag.de
http://www.gmx.net/


Re: Patch for Makefile race against current cvs

From
Peter Eisentraut
Date:
Tom Lane writes:

> But we could provide some security for multiple children of a single
> make by changing the rules to be like
>
> $(srcdir)/parse.h: gram.y
> ifdef YACC
>     $(YACC) -d $(YFLAGS) $<
>     mv y.tab.h $(srcdir)/parse.h
>     mv y.tab.c $(srcdir)/gram.c
> else
>     @$(missing) bison $< $@
> endif
>
> $(srcdir)/gram.c: $(srcdir)/parse.h

So I did and tried that:

| peter ~/pgsql/src/backend/parser$ make gram.c -j
| bison -y -d  gram.y
| mv y.tab.c ./gram.c
| mv y.tab.h ./parse.h
| bison -y  gram.y
| mv -f y.tab.c gram.c
| peter ~/pgsql/src/backend/parser$

Huh?

The problem is that

$(srcdir)/gram.c: $(srcdir)/parse.h

says, "before trying to make gram.c you must make parse.h", but it does
*not* says *how* to make gram.c.  So when faced with that question, make
runs the built-in %.y => %.c rule that you see.  In backend/bootstrap you
can see crash and burn as a result of this.

The correct version uses the rule

$(srcdir)/gram.c: $(srcdir)/parse.h ;

which says, "to make gram.c, make parse.h and then run the empty command".

This is what I checked in.

--
Peter Eisentraut   peter_e@gmx.net