Re: Bison 3.0 updates

Buildfarm member anchovy has been failing for the last couple of days,
evidently because its owner just couldn't wait to adopt bison 3.0,
which is all of 3 days old. The failures look like

cubeparse.y:42.1-13: warning: deprecated directive, use '%name-prefix' [-Wdeprecated]
%name-prefix="cube_yy"
^^^^^^^^^^^^^

(which looks like 3.0 isn't actually ready for prime time, but at least
it's only a warning)

cubeparse.c:163:5: error: conflicting types for 'cube_yyparse'
int cube_yyparse (void);
^
cubeparse.y:32:5: note: previous declaration of 'cube_yyparse' was here
int cube_yyparse(void *result);
^

A look in the Bison release notes explains this one: they stopped
supporting YYPARSE_PARAM, which contrib/cube and contrib/seg both use.
The recommended replacement is %parse-param, which is certainly a whole
lot cleaner: it lets you specify the datatype of the extra parser
parameter, instead of having it default to "void *". This option also
changes the signature of yyerror(), but that's not a problem.

At first I thought this was going to make us go through a tool upgrade
exercise, because I couldn't find %parse-param in the documentation for
bison 1.875, which is our oldest supported version. But further
research shows that %parse-param actually was introduced in 1.875,
they just forgot to document it :-(.

So I propose the attached patch, which I've verified still works with
1.875. I don't plan to install 3.0 just to test this, but I assume it's
OK there.

I'm thinking we should apply this to all supported branches, in case
somebody gets the idea to build an older branch with bleeding-edge
tools. Any objections?

regards, tom lane

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Buildfarm member anchovy has been failing for the last couple of days,
[...]
> I'm thinking we should apply this to all supported branches, in case
> somebody gets the idea to build an older branch with bleeding-edge
> tools. Any objections?

Certainly looks cleaner to me, and if it works for older bison then it
seems reasonable to back-patch it.

However, I comment on this mainly because anchovy has had issues with
9.1 and older for some time, which looks like an issue with GCC 4.8.0.
Did you happen to resolve or identify what is happening there..?

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> However, I comment on this mainly because anchovy has had issues with
> 9.1 and older for some time, which looks like an issue with GCC 4.8.0.
> Did you happen to resolve or identify what is happening there..?

Yeah, we know about that:
http://www.postgresql.org/message-id/14242.1365200084@sss.pgh.pa.us

The bottom line was:
>> It looks like our choices are (1) teach configure to enable
>> -fno-aggressive-loop-optimizations if the compiler recognizes it,
>> or (2) back-port commit 8137f2c32322c624e0431fac1621e8e9315202f9.

I am in favor of fixing the back branches via (1), because it's less
work and much less likely to break third-party extensions. Some other
people argued for (2), but I've not seen any patch emerge from them,
and you can bet I'm not going to do it.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > However, I comment on this mainly because anchovy has had issues with
> > 9.1 and older for some time, which looks like an issue with GCC 4.8.0.
> > Did you happen to resolve or identify what is happening there..?
>
> Yeah, we know about that:
> http://www.postgresql.org/message-id/14242.1365200084@sss.pgh.pa.us

Ah, right, read the thread but didn't attach it to anchovy.

> The bottom line was:
> >> It looks like our choices are (1) teach configure to enable
> >> -fno-aggressive-loop-optimizations if the compiler recognizes it,
> >> or (2) back-port commit 8137f2c32322c624e0431fac1621e8e9315202f9.
>
> I am in favor of fixing the back branches via (1), because it's less
> work and much less likely to break third-party extensions. Some other
> people argued for (2), but I've not seen any patch emerge from them,
> and you can bet I'm not going to do it.

Yea, just passing -fno-aggressive-loop-optimizations seems like the
safest and best option to me also..

Thanks,

Stephen

On 2013-07-29 07:11:13 -0400, Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > However, I comment on this mainly because anchovy has had issues with
> > > 9.1 and older for some time, which looks like an issue with GCC 4.8.0.
> > > Did you happen to resolve or identify what is happening there..?
> >
> > Yeah, we know about that:
> > http://www.postgresql.org/message-id/14242.1365200084@sss.pgh.pa.us
>
> Ah, right, read the thread but didn't attach it to anchovy.
>
> > The bottom line was:
> > >> It looks like our choices are (1) teach configure to enable
> > >> -fno-aggressive-loop-optimizations if the compiler recognizes it,
> > >> or (2) back-port commit 8137f2c32322c624e0431fac1621e8e9315202f9.
> >
> > I am in favor of fixing the back branches via (1), because it's less
> > work and much less likely to break third-party extensions. Some other
> > people argued for (2), but I've not seen any patch emerge from them,
> > and you can bet I'm not going to do it.
>
> Yea, just passing -fno-aggressive-loop-optimizations seems like the
> safest and best option to me also..

I think we need to do both. There very well might be other optimizations
made based on the unreachability information. Like concluding some
conditional block isn't reachable. (1) would be back branches only,
right?

If others aggree I'll happily (ok, that's a blatant lie), provide
patches if that actually helps $committer.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-07-29 07:11:13 -0400, Stephen Frost wrote:
>> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>>> The bottom line was:
>>> It looks like our choices are (1) teach configure to enable
>>> -fno-aggressive-loop-optimizations if the compiler recognizes it,
>>> or (2) back-port commit 8137f2c32322c624e0431fac1621e8e9315202f9.
>>>
>>> I am in favor of fixing the back branches via (1), because it's less
>>> work and much less likely to break third-party extensions. Some other
>>> people argued for (2), but I've not seen any patch emerge from them,
>>> and you can bet I'm not going to do it.

>> Yea, just passing -fno-aggressive-loop-optimizations seems like the
>> safest and best option to me also..

> I think we need to do both. There very well might be other optimizations
> made based on the unreachability information.

If we turn off the optimization, that will fix any other cases as well,
no? So why would we risk breaking third-party code by back-porting the
struct declaration changes?

regards, tom lane

On 2013-07-29 08:02:49 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-07-29 07:11:13 -0400, Stephen Frost wrote:
> >> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> >>> The bottom line was:
> >>> It looks like our choices are (1) teach configure to enable
> >>> -fno-aggressive-loop-optimizations if the compiler recognizes it,
> >>> or (2) back-port commit 8137f2c32322c624e0431fac1621e8e9315202f9.
> >>>
> >>> I am in favor of fixing the back branches via (1), because it's less
> >>> work and much less likely to break third-party extensions. Some other
> >>> people argued for (2), but I've not seen any patch emerge from them,
> >>> and you can bet I'm not going to do it.
>
> >> Yea, just passing -fno-aggressive-loop-optimizations seems like the
> >> safest and best option to me also..
>
> > I think we need to do both. There very well might be other optimizations
> > made based on the unreachability information.
>
> If we turn off the optimization, that will fix any other cases as well,
> no? So why would we risk breaking third-party code by back-porting the
> struct declaration changes?

The -fno-agressive-loop thingie afaics only controls the optimization
with regard to loopey constructs, not in general. I *think* there are
independent hazards with general unreachability detection. Not sure
whether they trigger at -O2 or only at -O3 though.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-07-29 08:02:49 -0400, Tom Lane wrote:
>> If we turn off the optimization, that will fix any other cases as well,
>> no? So why would we risk breaking third-party code by back-porting the
>> struct declaration changes?

> The -fno-agressive-loop thingie afaics only controls the optimization
> with regard to loopey constructs, not in general. I *think* there are
> independent hazards with general unreachability detection. Not sure
> whether they trigger at -O2 or only at -O3 though.

I'm not excited about breaking code in order to fix optimization bugs
that are purely hypothetical (and for which there's no particular reason
to believe that the proposed change would fix them anyway). If we were
seeing such things in the field it would be a different story.

regards, tom lane

On 2013-07-29 08:17:46 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-07-29 08:02:49 -0400, Tom Lane wrote:
> >> If we turn off the optimization, that will fix any other cases as well,
> >> no? So why would we risk breaking third-party code by back-porting the
> >> struct declaration changes?
>
> > The -fno-agressive-loop thingie afaics only controls the optimization
> > with regard to loopey constructs, not in general. I *think* there are
> > independent hazards with general unreachability detection. Not sure
> > whether they trigger at -O2 or only at -O3 though.
>
> I'm not excited about breaking code in order to fix optimization bugs
> that are purely hypothetical (and for which there's no particular reason
> to believe that the proposed change would fix them anyway). If we were
> seeing such things in the field it would be a different story.

Well, given the problems we're discussing here, it's not all that
hypothetical. Obviously the compiler *does* have information it believes
to proof some code to be unreachable. And we know that information comes
from the way the catalog structs are declared.
I don't really fancy finding more and more optimizations we didn't think
about and disabling them one by one after annoying debugging sessions.

Also, just about all code that would get broken by that change would be
code that's already pretty much broken.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-07-29 08:17:46 -0400, Tom Lane wrote:
>> I'm not excited about breaking code in order to fix optimization bugs
>> that are purely hypothetical (and for which there's no particular reason
>> to believe that the proposed change would fix them anyway). If we were
>> seeing such things in the field it would be a different story.

> Well, given the problems we're discussing here, it's not all that
> hypothetical. Obviously the compiler *does* have information it believes
> to proof some code to be unreachable.

The known case here has nothing to do with unreachability IMO; it has
to do with concluding that a loop can be unrolled into exactly one
execution. But in any case, there is no point in arguing about what
hypothetical versions of gcc might do in hypothetical cases. We have
experimental evidence that -fno-aggressive-loop-optimizations fixes the
observed problem with gcc 4.8.0. So we can apply a one-line patch that
fixes the observed problem and seems 100% safe, or we can apply a very
large patch that might possibly fix problems that we don't know exist,
and might also break third-party code. Given the available evidence
the choice seems pretty clear to me. If you want to provide concrete
proof that there are additional optimization hazards then that would
change the tradeoff.

regards, tom lane

On 07/29/2013 01:05 AM, Tom Lane wrote:
> Buildfarm member anchovy has been failing for the last couple of days,
> evidently because its owner just couldn't wait to adopt bison 3.0,
> which is all of 3 days old. The failures look like
>

Reminder to buildfarm animal owners: if you upgrade software please make
sure your buildfarm members are still working. And if the upgrade
involves the OS or the compiler, please use the udate_personality.pl
script to update the server.

cheers

andrew

On 2013-07-29 08:44:56 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-07-29 08:17:46 -0400, Tom Lane wrote:
> >> I'm not excited about breaking code in order to fix optimization bugs
> >> that are purely hypothetical (and for which there's no particular reason
> >> to believe that the proposed change would fix them anyway). If we were
> >> seeing such things in the field it would be a different story.
>
> > Well, given the problems we're discussing here, it's not all that
> > hypothetical. Obviously the compiler *does* have information it believes
> > to proof some code to be unreachable.
>
> The known case here has nothing to do with unreachability IMO; it has
> to do with concluding that a loop can be unrolled into exactly one
> execution.

If I understand the optimization correctly what it does is building a
flow control model of a loop and concluding which its iterations can be
reached assuming standard conforming code. In this case, since it knows
(from looking at the underlying data structures) that the upper bound of
the iteration has to be 1 it decides to stop there. If oidvector had
values[2] it would make two iterations.

> But in any case, there is no point in arguing about what
> hypothetical versions of gcc might do in hypothetical cases. We have
> experimental evidence that -fno-aggressive-loop-optimizations fixes the
> observed problem with gcc 4.8.0.

Well, it seems to me like we don't have particularly good experience
with fixing compiler optimization issues by disabling some specific
optimization. They tend to crop up again under the umbrella of another
feature a compiler version or two later.

FWIW cherry-picking bf66bfb4cd726b6ddab3fe2718648f65a7106149 and
e58f3181fdaacc91d4cc1bd98a4a8ad7d286544c fixes the issue for me
(After fixing trivial conflicts in the latter).

32 files changed, 98 insertions(+), 50 deletions(-)

> So we can apply a one-line patch that
> fixes the observed problem and seems 100% safe, or we can apply a very
> large patch that might possibly fix problems that we don't know exist,
> and might also break third-party code. Given the available evidence
> the choice seems pretty clear to me. If you want to provide concrete
> proof that there are additional optimization hazards then that would
> change the tradeoff.

No, don't really want to spend more time on this. We'll see whether it
crops up again or not.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

> On 07/29/2013 01:05 AM, Tom Lane wrote:
>> Buildfarm member anchovy has been failing for the last couple of days,
>> evidently because its owner just couldn't wait to adopt bison 3.0,
>> which is all of 3 days old.

Hmm? Anchovy is upgrading automatically to newest Arch Linux packages daily.

I assumed that's a good thing -- the purpose of build farm is to test
PostgreSQL in various different real-life environments? Arch Linux is
one such environment that adopts new packages very quickly. If Arch
users are unable to build PostgreSQL then surely it's good to be
notified by the build farm before real users start reporting problems?

I don't mean to sound reluctant, I'm open to suggestions, but please
help me understand why this is bad.

On Mon, Jul 29, 2013 at 4:59 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Reminder to buildfarm animal owners: if you upgrade software please make
> sure your buildfarm members are still working.

If I had checked the machine's status manually after upgrading, the
best course of action would be to report the incompatibility to
PostgreSQL mailing lists. The end result is the same, in that sense,
it was "working". Manual upgrades would only delay that reporting, not
prevent it?

I realize there have been problems with anchovy that are my own fault
and I'm sorry about that. I check the buildfarm status every week or
two.

> And if the upgrade involves
> the OS or the compiler, please use the udate_personality.pl script to update
> the server.

Is it OK to run update_personality.pl automatically every day from crontab?

Regards,
Marti

On 07/29/2013 10:28 AM, Marti Raudsepp wrote:
> Hi,
>
>> On 07/29/2013 01:05 AM, Tom Lane wrote:
>>> Buildfarm member anchovy has been failing for the last couple of days,
>>> evidently because its owner just couldn't wait to adopt bison 3.0,
>>> which is all of 3 days old.
> Hmm? Anchovy is upgrading automatically to newest Arch Linux packages daily.
>
> I assumed that's a good thing -- the purpose of build farm is to test
> PostgreSQL in various different real-life environments? Arch Linux is
> one such environment that adopts new packages very quickly. If Arch
> users are unable to build PostgreSQL then surely it's good to be
> notified by the build farm before real users start reporting problems?
>
> I don't mean to sound reluctant, I'm open to suggestions, but please
> help me understand why this is bad.

The buildfarm is principally designed to detect when some change in the
Postgres code breaks something. As such, the expectation is that the
animals will provide a fairly stable platform. A totally moving target
will present us with false positives, since the failure could be due to
no action of ours at all.

A machine that can auto-upgrade anything at any time is really not very
fit for the purpose. What that would be testing is not if a change in
Postgres code has broken something, but if the host packaging system has
broken something.

>> And if the upgrade involves
>> the OS or the compiler, please use the udate_personality.pl script to update
>> the server.
> Is it OK to run update_personality.pl automatically every day from crontab?

No. It is designed to support a fairly small number of changes.

cheers

andrew

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> FWIW cherry-picking bf66bfb4cd726b6ddab3fe2718648f65a7106149 and
> e58f3181fdaacc91d4cc1bd98a4a8ad7d286544c fixes the issue for me
> (After fixing trivial conflicts in the latter).

I've already spent more time on this than I wanted to, but just for
the archives' sake: neither of those commits exist in the master
repo.

regards, tom lane

On 2013-07-29 10:46:41 -0400, Andrew Dunstan wrote:
>
> On 07/29/2013 10:28 AM, Marti Raudsepp wrote:
> >Hi,
> >
> >>On 07/29/2013 01:05 AM, Tom Lane wrote:
> >>>Buildfarm member anchovy has been failing for the last couple of days,
> >>>evidently because its owner just couldn't wait to adopt bison 3.0,
> >>>which is all of 3 days old.
> >Hmm? Anchovy is upgrading automatically to newest Arch Linux packages daily.
> >
> >I assumed that's a good thing -- the purpose of build farm is to test
> >PostgreSQL in various different real-life environments? Arch Linux is
> >one such environment that adopts new packages very quickly. If Arch
> >users are unable to build PostgreSQL then surely it's good to be
> >notified by the build farm before real users start reporting problems?
> >
> >I don't mean to sound reluctant, I'm open to suggestions, but please
> >help me understand why this is bad.
>
>
> The buildfarm is principally designed to detect when some change in the
> Postgres code breaks something. As such, the expectation is that the animals
> will provide a fairly stable platform. A totally moving target will present
> us with false positives, since the failure could be due to no action of ours
> at all.
>
> A machine that can auto-upgrade anything at any time is really not very fit
> for the purpose. What that would be testing is not if a change in Postgres
> code has broken something, but if the host packaging system has broken
> something.

FWIW given the evidence here anchovy seems to do a good job. Both the
gcc 4.8 and the bison 3.0 problems are things the project needs to know
about and that need to get fixed. And none of the other animals seems to
run anything really up2date.
Also, afaik Arch has a rolling release model, so there isn't any other
specific release that should be tested instead.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-07-29 10:52:10 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > FWIW cherry-picking bf66bfb4cd726b6ddab3fe2718648f65a7106149 and
> > e58f3181fdaacc91d4cc1bd98a4a8ad7d286544c fixes the issue for me
> > (After fixing trivial conflicts in the latter).
>
> I've already spent more time on this than I wanted to, but just for
> the archives' sake: neither of those commits exist in the master
> repo.

Hrmpf, sorry for that. Posting the hashes *after* cherry-picking them
ontop REL9_1_STABLE obviously isn't very helpful. The correct ones are:
8a3f745f160d8334ad978676828d3926ac949f43
8137f2c32322c624e0431fac1621e8e9315202f9

Those definitely are reachable via gitweb.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 07/29/2013 10:28 AM, Marti Raudsepp wrote:
>> Hmm? Anchovy is upgrading automatically to newest Arch Linux packages daily.
>> I assumed that's a good thing -- the purpose of build farm is to test
>> PostgreSQL in various different real-life environments? Arch Linux is
>> one such environment that adopts new packages very quickly. If Arch
>> users are unable to build PostgreSQL then surely it's good to be
>> notified by the build farm before real users start reporting problems?

> The buildfarm is principally designed to detect when some change in the
> Postgres code breaks something. As such, the expectation is that the
> animals will provide a fairly stable platform. A totally moving target
> will present us with false positives, since the failure could be due to
> no action of ours at all.

I can see both sides of this. It's definitely nice to get early warning
when toolchain changes create new problems for Postgres, but it's not
clear that the buildfarm is the best way to get such notifications.

The big problem here is that it might take a long time before we have
a suitable fix, and in the meantime that buildfarm member is basically
useless: it can't tell us anything new, and even if it tried, probably
nobody would notice because they'd not realize the failure cause changed.
We've had cases before where a buildfarm member that was failing for
some known reason was unable to detect a later-introduced bug, so this
isn't hypothetical. (And I'm not too thrilled that we've let the
back-branch failures on anchovy persist for months, though I suppose
that's not as bad as a breakage in HEAD.)

Ideally we'd not rotate toolchain updates into the buildfarm until
they'd been checked out in some other way. On the other hand, this
doesn't seem like a big enough problem to justify building some
different infrastructure for it, so I'm not seeing a practical way
to do better.

regards, tom lane

On Mon, Jul 29, 2013 at 5:53 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Both the
> gcc 4.8 and the bison 3.0 problems are things the project needs to know
> about

Perl 5.18 too: http://www.postgresql.org/message-id/2825.1370226552@sss.pgh.pa.us

Marti

On 07/29/2013 11:05 AM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 07/29/2013 10:28 AM, Marti Raudsepp wrote:
>>> Hmm? Anchovy is upgrading automatically to newest Arch Linux packages daily.
>>> I assumed that's a good thing -- the purpose of build farm is to test
>>> PostgreSQL in various different real-life environments? Arch Linux is
>>> one such environment that adopts new packages very quickly. If Arch
>>> users are unable to build PostgreSQL then surely it's good to be
>>> notified by the build farm before real users start reporting problems?
>> The buildfarm is principally designed to detect when some change in the
>> Postgres code breaks something. As such, the expectation is that the
>> animals will provide a fairly stable platform. A totally moving target
>> will present us with false positives, since the failure could be due to
>> no action of ours at all.
> I can see both sides of this. It's definitely nice to get early warning
> when toolchain changes create new problems for Postgres, but it's not
> clear that the buildfarm is the best way to get such notifications.
>
> The big problem here is that it might take a long time before we have
> a suitable fix, and in the meantime that buildfarm member is basically
> useless: it can't tell us anything new, and even if it tried, probably
> nobody would notice because they'd not realize the failure cause changed.
> We've had cases before where a buildfarm member that was failing for
> some known reason was unable to detect a later-introduced bug, so this
> isn't hypothetical. (And I'm not too thrilled that we've let the
> back-branch failures on anchovy persist for months, though I suppose
> that's not as bad as a breakage in HEAD.)
>
> Ideally we'd not rotate toolchain updates into the buildfarm until
> they'd been checked out in some other way. On the other hand, this
> doesn't seem like a big enough problem to justify building some
> different infrastructure for it, so I'm not seeing a practical way
> to do better.
>
>

There has to be something between "set in stone and never changes" and
"auto-updates everything every 24 hours" that would suit us.

I'm toying with the idea of a check_upgrade mode for the buildfarm
client where it wouldn't do a git pull, but would report changes if the
build result was different from the previous result. You'd run this
immediately after pulling new changes into your OS. Other, brighter
ideas welcome.

cheers

andrew

On Mon, Jul 29, 2013 at 9:15 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> There has to be something between "set in stone and never changes" and
> "auto-updates everything every 24 hours" that would suit us.

Well sure I could change the update frequency. But again, it seems
like delaying the inevitable.

> I'm toying with the idea of a check_upgrade mode for the buildfarm client
> where it wouldn't do a git pull, but would report changes if the build
> result was different from the previous result. You'd run this immediately
> after pulling new changes into your OS. Other, brighter ideas welcome.

What would be the right course of action if check_upgrade fails? Is it
reasonable to expect buildfarm volunteers to downgrade the system and
postpone until the problem is resolved?

Or do you think the member should be removed from the farm until the
build succeeds again? Sounds like worst of both worlds.

Regards,
Marti

On 07/29/2013 02:26 PM, Marti Raudsepp wrote:
>> I'm toying with the idea of a check_upgrade mode for the buildfarm client
>> where it wouldn't do a git pull, but would report changes if the build
>> result was different from the previous result. You'd run this immediately
>> after pulling new changes into your OS. Other, brighter ideas welcome.
> What would be the right course of action if check_upgrade fails? Is it
> reasonable to expect buildfarm volunteers to downgrade the system and
> postpone until the problem is resolved?
>
> Or do you think the member should be removed from the farm until the
> build succeeds again? Sounds like worst of both worlds.
>

Neither, I don't think you're understanding me at all. The idea is to
have some way of saying "well, the code is the same, but the tools have
changed." i.e. we want to be able to hold one of these variables constant.

The buildfarm server knows how the run was invoked, and could
distinguish runs done in this mode from other runs.

cheers

andrew

On Mon, Jul 29, 2013 at 4:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I can see both sides of this. It's definitely nice to get early warning
> when toolchain changes create new problems for Postgres, but it's not
> clear that the buildfarm is the best way to get such notifications.

Perhaps something as simple as choosing a sufficiently oddball or
frightening animal name?

predator or alien?
killerbee? bedbug?

--
greg

On Mon, Jul 29, 2013 at 11:45 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 07/29/2013 02:26 PM, Marti Raudsepp wrote:
>>>
>>> I'm toying with the idea of a check_upgrade mode for the buildfarm client
>>> where it wouldn't do a git pull, but would report changes if the build
>>> result was different from the previous result. You'd run this immediately
>>> after pulling new changes into your OS. Other, brighter ideas welcome.
>>
>> What would be the right course of action if check_upgrade fails? Is it
>> reasonable to expect buildfarm volunteers to downgrade the system and
>> postpone until the problem is resolved?
>>
>> Or do you think the member should be removed from the farm until the
>> build succeeds again? Sounds like worst of both worlds.
>>
>
>
> Neither, I don't think you're understanding me at all. The idea is to have
> some way of saying "well, the code is the same, but the tools have changed."
> i.e. we want to be able to hold one of these variables constant.

Could it remove the need for manual intervention, by detecting if the
tools have changed first, and then suppressing the git pull and adding
the appropriate reporting flag, if they have?

Cheers,

Jeff

* Andrew Dunstan wrote:

> I'm toying with the idea of a check_upgrade mode for the buildfarm
> client where it wouldn't do a git pull, but would report changes if the
> build result was different from the previous result. You'd run this
> immediately after pulling new changes into your OS. Other, brighter
> ideas welcome.

Might it be possible for the buildfarm client to query the various tools
(compiler, autotools, flex, bison, perl, shell, etc.) for their version
numbers and report those too? If the config page said "since the last
successful build, these commits have been added, and the compiler is now
tomorrow's snapshot of gcc 4.8.4711", diagnosing a failed build could be
easier.

This could be done either through the usual -V, --version flags or by
asking the host's packaging system, although there are probably too many
of the latter to support.

I would not want to have to implement this, but perhaps the client could
also automatically drop back to the last known good commit if it gets a
failed build immediately after a tool version change.

--
Christian

Christian Ullrich <chris(at)chrullrich(dot)net> writes:
> Might it be possible for the buildfarm client to query the various tools
> (compiler, autotools, flex, bison, perl, shell, etc.) for their version
> numbers and report those too?

The configure script already takes pains to log the version numbers for
most of those things. (Of course, that doesn't help on Windows ...)
The disadvantage of course is that somebody looking at a failure may not
think to compare tool version numbers right away, since the info is
on a page that isn't normally where the reported failure is, and it's
buried in a long and mostly very boring report.

However, I'm not sure that the buildfarm script can improve on this much.
It has no business guessing which tools configure will select for use,
so it can't just go and make its own queries. Maybe it could grep some
of the interesting lines out of the configure report?

regards, tom lane

On Mon, Jul 29, 2013 at 11:05:52AM -0400, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > On 07/29/2013 10:28 AM, Marti Raudsepp wrote:
> >> Hmm? Anchovy is upgrading automatically to newest Arch Linux packages daily.
> >> I assumed that's a good thing -- the purpose of build farm is to test
> >> PostgreSQL in various different real-life environments? Arch Linux is
> >> one such environment that adopts new packages very quickly. If Arch
> >> users are unable to build PostgreSQL then surely it's good to be
> >> notified by the build farm before real users start reporting problems?
>
> > The buildfarm is principally designed to detect when some change in the
> > Postgres code breaks something. As such, the expectation is that the
> > animals will provide a fairly stable platform. A totally moving target
> > will present us with false positives, since the failure could be due to
> > no action of ours at all.
>
> I can see both sides of this. It's definitely nice to get early warning
> when toolchain changes create new problems for Postgres, but it's not
> clear that the buildfarm is the best way to get such notifications.

Early notification when a dependency's compatibility break affects PostgreSQL
is a Very Good Thing.

> The big problem here is that it might take a long time before we have
> a suitable fix, and in the meantime that buildfarm member is basically
> useless: it can't tell us anything new, and even if it tried, probably
> nobody would notice because they'd not realize the failure cause changed.
> We've had cases before where a buildfarm member that was failing for
> some known reason was unable to detect a later-introduced bug, so this
> isn't hypothetical. (And I'm not too thrilled that we've let the
> back-branch failures on anchovy persist for months, though I suppose
> that's not as bad as a breakage in HEAD.)

True, but the solution, if any, is more buildfarm members. anchovy plays the
important role of a sentinel for trouble with bleeding-edge dependency
packages. Doing that with excellence inevitably makes it less useful for
detecting other kinds of problems.

> On the other hand, this
> doesn't seem like a big enough problem to justify building some
> different infrastructure for it, so I'm not seeing a practical way
> to do better.

Agreed. Let's stick an "Updates OS packages daily, regularly acquiring
bleeding-edge upstream releases" note on the buildfarm status page, like we
have for the CLOBBER_CACHE_ALWAYS members. That should be enough for folks to
realize that a failure in this animal alone is more likely the fault of a new
package version than of the latest commit.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

On Tue, Jul 30, 2013 at 3:56 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> Agreed. Let's stick an "Updates OS packages daily, regularly acquiring
> bleeding-edge upstream releases" note on the buildfarm status page

FWIW, I added "[updated daily]" to the OS version field.

I haven't changed other configuration yet, will wait until there's a consensus.

Regards,
Marti

On 2013-07-29 08:02:49 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-07-29 07:11:13 -0400, Stephen Frost wrote:
> >> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> >>> The bottom line was:
> >>> It looks like our choices are (1) teach configure to enable
> >>> -fno-aggressive-loop-optimizations if the compiler recognizes it,
> >>> or (2) back-port commit 8137f2c32322c624e0431fac1621e8e9315202f9.
> >>>
> >>> I am in favor of fixing the back branches via (1), because it's less
> >>> work and much less likely to break third-party extensions. Some other
> >>> people argued for (2), but I've not seen any patch emerge from them,
> >>> and you can bet I'm not going to do it.
>
> >> Yea, just passing -fno-aggressive-loop-optimizations seems like the
> >> safest and best option to me also..
>
> > I think we need to do both. There very well might be other optimizations
> > made based on the unreachability information.
>
> If we turn off the optimization, that will fix any other cases as well,
> no? So why would we risk breaking third-party code by back-porting the
> struct declaration changes?

This seems to be the agreed upon course of action, so I've prepared a
patch including a preliminary commit message. I confirmed that it fixes
the issue with gcc 4.8 and 9.1 for me.

The patch needs to be applied to 9.1, 9.0 and 8.4.

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-07-29 08:02:49 -0400, Tom Lane wrote:
>>> It looks like our choices are (1) teach configure to enable
>>> -fno-aggressive-loop-optimizations if the compiler recognizes it,
>>> or (2) back-port commit 8137f2c32322c624e0431fac1621e8e9315202f9.
>>>
>>> I am in favor of fixing the back branches via (1), because it's less
>>> work and much less likely to break third-party extensions.

> This seems to be the agreed upon course of action, so I've prepared a
> patch including a preliminary commit message. I confirmed that it fixes
> the issue with gcc 4.8 and 9.1 for me.

Committed --- thanks for doing the legwork to check it fixes the problem.

regards, tom lane

I'm getting some more of these, including some I thought you had fixed.

Bison 3.0.2 on current head.

<>
Writing postgres.bki
Writing schemapg.h
Writing postgres.description
Writing postgres.shdescription
gram.y:172.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
%name-prefix="base_yy"
^^^^^^^^^^^^^
Writing fmgroids.h
Writing fmgrtab.c
bootparse.y:96.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
%name-prefix="boot_yy"
^^^^^^^^^^^^^
In file included from gram.y:14004:0:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:10188:23: warning: unused variable ‘yyg’ [-Wunused-variable]
struct yyguts_t * yyg = (struct yyguts_t*)yyscanner; /* This var
may be unused depending upon options. */
^
repl_gram.y:43.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
%name-prefix="replication_yy"
^^^^^^^^^^^^^
preproc.y:576.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
%name-prefix="base_yy"
^^^^^^^^^^^^^
pl_gram.y:113.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
%name-prefix="plpgsql_yy"
^^^^^^^^^^^^^
cubeparse.y:42.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
%name-prefix="cube_yy"
^^^^^^^^^^^^^
segparse.y:45.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
%name-prefix="seg_yy"
^^^^^^^^^^^^^
PostgreSQL, contrib, and documentation installation complete.
</>

On 07/29/2013 01:05 AM, Tom Lane wrote:
> Buildfarm member anchovy has been failing for the last couple of days,
> evidently because its owner just couldn't wait to adopt bison 3.0,
> which is all of 3 days old. The failures look like
>
> cubeparse.y:42.1-13: warning: deprecated directive, use '%name-prefix' [-Wdeprecated]
> %name-prefix="cube_yy"
> ^^^^^^^^^^^^^
>
> (which looks like 3.0 isn't actually ready for prime time, but at least
> it's only a warning)
>
> cubeparse.c:163:5: error: conflicting types for 'cube_yyparse'
> int cube_yyparse (void);
> ^
> cubeparse.y:32:5: note: previous declaration of 'cube_yyparse' was here
> int cube_yyparse(void *result);
> ^
>
> A look in the Bison release notes explains this one: they stopped
> supporting YYPARSE_PARAM, which contrib/cube and contrib/seg both use.
> The recommended replacement is %parse-param, which is certainly a whole
> lot cleaner: it lets you specify the datatype of the extra parser
> parameter, instead of having it default to "void *". This option also
> changes the signature of yyerror(), but that's not a problem.
>
> At first I thought this was going to make us go through a tool upgrade
> exercise, because I couldn't find %parse-param in the documentation for
> bison 1.875, which is our oldest supported version. But further
> research shows that %parse-param actually was introduced in 1.875,
> they just forgot to document it :-(.
>
> So I propose the attached patch, which I've verified still works with
> 1.875. I don't plan to install 3.0 just to test this, but I assume it's
> OK there.
>
> I'm thinking we should apply this to all supported branches, in case
> somebody gets the idea to build an older branch with bleeding-edge
> tools. Any objections?
>
> regards, tom lane
>
>
>

--
Vik

Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
> I'm getting some more of these, including some I thought you had fixed.
> Bison 3.0.2 on current head.

I didn't do anything to suppress those warnings:

> gram.y:172.1-13: warning: deprecated directive, use %name-prefix
> [-Wdeprecated]
> %name-prefix="base_yy"
> ^^^^^^^^^^^^^

because it's hard to see how that's anything but a bison bug.

regards, tom lane

On 05/21/2014 12:29 PM, Tom Lane wrote:
> Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
>> I'm getting some more of these, including some I thought you had fixed.
>> Bison 3.0.2 on current head.
> I didn't do anything to suppress those warnings:
>
>> gram.y:172.1-13: warning: deprecated directive, use ‘%name-prefix’
>> [-Wdeprecated]
>> %name-prefix="base_yy"
>> ^^^^^^^^^^^^^
> because it's hard to see how that's anything but a bison bug.

The wording is a little odd as it seems we're supposed to replace
%name-prefix with... %name-prefix, but the docs say we're supposed to
use api.prefix now instead. I'll just take your word for it as I've
never done anything with bison before.

--
Vik

Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
> On 05/21/2014 12:29 PM, Tom Lane wrote:
>> I didn't do anything to suppress those warnings:
>>> gram.y:172.1-13: warning: deprecated directive, use %name-prefix
>>> [-Wdeprecated]
>>> %name-prefix="base_yy"
>>> ^^^^^^^^^^^^^
>> because it's hard to see how that's anything but a bison bug.

> The wording is a little odd as it seems we're supposed to replace
> %name-prefix with... %name-prefix, but the docs say we're supposed to
> use api.prefix now instead.

Oh, just a bad error message eh?

[ Reads docs ... ] AFAICT, they've deprecated this in favor of some
other syntax that they introduced in 2.6, which was released in July 2012.
That makes it a nonstarter for us. We're not going to break PG altogether
for most people in order to hide a warning message on the bleeding edge
version.

(For comparison, I've got bison 2.3 on my Mac and 2.4.1 on my RHEL6
machine, both of which are pretty up-to-date platforms as such things
go. Some of the buildfarm machines are still running 1.875.)

regards, tom lane

On 5/21/14, 12:29 PM, Tom Lane wrote:
> Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
>> I'm getting some more of these, including some I thought you had fixed.
>> Bison 3.0.2 on current head.
>
> I didn't do anything to suppress those warnings:
>
>> gram.y:172.1-13: warning: deprecated directive, use ‘%name-prefix’
>> [-Wdeprecated]
>> %name-prefix="base_yy"
>> ^^^^^^^^^^^^^
>
> because it's hard to see how that's anything but a bison bug.

What they want is that you use

%name-prefix "base_yy"

instead of

%name-prefix="base_yy"

That makes the warning go away.

The %something=something syntax is not documented anywhere, so it looks
like it worked more or less by accident. We don't use it anywhere else
either (e.g. %expect 0, not %expect=0).

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> What they want is that you use
> %name-prefix "base_yy"
> instead of
> %name-prefix="base_yy"
> That makes the warning go away.

Oh really!?

> The %something=something syntax is not documented anywhere, so it looks
> like it worked more or less by accident. We don't use it anywhere else
> either (e.g. %expect 0, not %expect=0).

Hah. That's probably my fault, somewhere way back when. I'll fix it,
unless you're already on it.

regards, tom lane

On 5/28/14, 2:43 PM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> What they want is that you use
>> %name-prefix "base_yy"
>> instead of
>> %name-prefix="base_yy"
>> That makes the warning go away.
>
> Oh really!?
>
>> The %something=something syntax is not documented anywhere, so it looks
>> like it worked more or less by accident. We don't use it anywhere else
>> either (e.g. %expect 0, not %expect=0).
>
> Hah. That's probably my fault, somewhere way back when. I'll fix it,
> unless you're already on it.

Go ahead.