pgsql: Disable -faggressive-loop-optimizations in gcc 4.8+ for pre-9.2

Disable -faggressive-loop-optimizations in gcc 4.8+ for pre-9.2 branches.

With this optimization flag enabled, recent versions of gcc can generate
incorrect code that assumes variable-length arrays (such as oidvector)
are actually fixed-length because they're embedded in some larger struct.
The known instance of this problem was fixed in 9.2 and up by commit
8137f2c32322c624e0431fac1621e8e9315202f9 and followon work, which hides
actually-variable-length catalog fields from the compiler altogether.
And we plan to gradually convert variable-length fields to official
"flexible array member" notation over time, which should prevent this type
of bug from reappearing as gcc gets smarter. We're not going to try to
back-port those changes into older branches, though, so apply this
band-aid instead.

Andres Freund

Branch
------
REL8_4_STABLE

Details
-------
http://git.postgresql.org/pg/commitdiff/8396d230fa489c8766ce0a9128bb0d8e5ff6667e

Modified Files
--------------
configure | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
configure.in | 3 +++
2 files changed, 54 insertions(+)

Tom Lane wrote:
> Disable -faggressive-loop-optimizations in gcc 4.8+ for pre-9.2 branches.
>
> With this optimization flag enabled, recent versions of gcc can generate
> incorrect code that assumes variable-length arrays (such as oidvector)
> are actually fixed-length because they're embedded in some larger struct.
> The known instance of this problem was fixed in 9.2 and up by commit
> 8137f2c32322c624e0431fac1621e8e9315202f9 and followon work, which hides
> actually-variable-length catalog fields from the compiler altogether.
> And we plan to gradually convert variable-length fields to official
> "flexible array member" notation over time, which should prevent this type
> of bug from reappearing as gcc gets smarter. We're not going to try to
> back-port those changes into older branches, though, so apply this
> band-aid instead.

Would anybody object to me pushing this commit to branches 8.2 and 8.3?

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

On Tue, Jan 20, 2015 at 10:30 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Tom Lane wrote:
>> Disable -faggressive-loop-optimizations in gcc 4.8+ for pre-9.2 branches.
>> With this optimization flag enabled, recent versions of gcc can generate
>> incorrect code that assumes variable-length arrays (such as oidvector)
>> are actually fixed-length because they're embedded in some larger struct.
>> The known instance of this problem was fixed in 9.2 and up by commit
>> 8137f2c32322c624e0431fac1621e8e9315202f9 and followon work, which hides
>> actually-variable-length catalog fields from the compiler altogether.
>> And we plan to gradually convert variable-length fields to official
>> "flexible array member" notation over time, which should prevent this type
>> of bug from reappearing as gcc gets smarter. We're not going to try to
>> back-port those changes into older branches, though, so apply this
>> band-aid instead.
>
> Would anybody object to me pushing this commit to branches 8.2 and 8.3?

Since those branches are out of support, I am not sure what the point
is. If we want people to be able to use those branches reasonably we
need to back-port fixes for critical security and stability issues,
not just this one thing.

But maybe I am missing something.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2015-01-20 11:10:53 -0500, Robert Haas wrote:
> On Tue, Jan 20, 2015 at 10:30 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > Tom Lane wrote:
> >> Disable -faggressive-loop-optimizations in gcc 4.8+ for pre-9.2 branches.
> >> With this optimization flag enabled, recent versions of gcc can generate
> >> incorrect code that assumes variable-length arrays (such as oidvector)
> >> are actually fixed-length because they're embedded in some larger struct.
> >> The known instance of this problem was fixed in 9.2 and up by commit
> >> 8137f2c32322c624e0431fac1621e8e9315202f9 and followon work, which hides
> >> actually-variable-length catalog fields from the compiler altogether.
> >> And we plan to gradually convert variable-length fields to official
> >> "flexible array member" notation over time, which should prevent this type
> >> of bug from reappearing as gcc gets smarter. We're not going to try to
> >> back-port those changes into older branches, though, so apply this
> >> band-aid instead.
> >
> > Would anybody object to me pushing this commit to branches 8.2 and 8.3?
>
> Since those branches are out of support, I am not sure what the point
> is. If we want people to be able to use those branches reasonably we
> need to back-port fixes for critical security and stability issues,
> not just this one thing.
>
> But maybe I am missing something.

Supporting and being able to compile and run 'make check' (which doesn't
complete >= gcc 4.8) aren't the same thing though. And we e.g. try to
provide pg_dump and libpq support for older versions, which is hard to
ensure if you can't run them.

I personally think that being able to at least compile/make check old
versions a bit longer is a good idea.

Greetings,

Andres Freund

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

Robert Haas wrote:

> > Would anybody object to me pushing this commit to branches 8.2 and 8.3?
>
> Since those branches are out of support, I am not sure what the point
> is. If we want people to be able to use those branches reasonably we
> need to back-port fixes for critical security and stability issues,
> not just this one thing.
>
> But maybe I am missing something.

I just want to make it easy to compile those branches with current
toolset so that I can study their behavior to suggest workarounds for
customer problems etc -- nothing more. I am not proposing to open them
up again for support. Of course, I can carry the patched branches
locally if there is strong opposition, but since it's harmless, I don't
see why would there be any such. Another easy workaround is to add -O0
to CFLAGS, and I can script that easily too.

Without this patch or -O0, initdb fails with

inicializando pg_authid ... FATAL: wrong number of index expressions
SENTENCIA: CREATE TRIGGER pg_sync_pg_database AFTER INSERT OR UPDATE OR DELETE ON pg_database FOR EACH STATEMENT EXECUTE PROCEDURE flatfile_update_trigger();

There is the additional problem that contrib/cube fails to compile, but
I don't care enough about that one:

/pgsql/source/REL8_3_STABLE/contrib/cube/cubeparse.y:61:17: error: ‘result’ undeclared (first use in this function)
*((void **)result) = write_box( dim, $2, $4 );
^

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

On Tue, Jan 20, 2015 at 11:18 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> > Would anybody object to me pushing this commit to branches 8.2 and 8.3?
>>
>> Since those branches are out of support, I am not sure what the point
>> is. If we want people to be able to use those branches reasonably we
>> need to back-port fixes for critical security and stability issues,
>> not just this one thing.
>>
>> But maybe I am missing something.
>
> I just want to make it easy to compile those branches with current
> toolset so that I can study their behavior to suggest workarounds for
> customer problems etc -- nothing more. I am not proposing to open them
> up again for support.

Oh, I see. Well, that doesn't bother me, then.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--On 20. Januar 2015 17:15:01 +0100 Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:

> I personally think that being able to at least compile/make check old
> versions a bit longer is a good idea.

+1 from me for this idea.

--
Thanks

Bernd

Bernd Helmle wrote:
>
>
> --On 20. Januar 2015 17:15:01 +0100 Andres Freund <andres(at)2ndquadrant(dot)com>
> wrote:
>
> >I personally think that being able to at least compile/make check old
> >versions a bit longer is a good idea.
>
> +1 from me for this idea.

Already done yesterday :-)

Thanks,

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