Thread: pgsql: Fix thinko in previous commit
Fix thinko in previous commit Since postgres.h includes palloc.h, definitions that affect the latter must be present before the former is included. Per buildfarm results Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/878daf2e72755feadbfb8d21aad26dafd8658086 Modified Files -------------- src/backend/utils/mmgr/mcxt.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Alvaro Herrera wrote: > Fix thinko in previous commit > > Since postgres.h includes palloc.h, definitions that affect the latter > must be present before the former is included. Spoonbill is still failing. I don't really understand what's going on there -- I'll research further as best as I can. (It seems to me that spoonbill is disabling USE_INLINE because of new warnings that are probably caused by the FORTIFY_SOURCE stuff. This seems to be interacting strangely with the new uses of USE_INLINE in the headers I modified.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> (It seems to me that spoonbill is disabling USE_INLINE because of new
> warnings that are probably caused by the FORTIFY_SOURCE stuff. This
> seems to be interacting strangely with the new uses of USE_INLINE in the
> headers I modified.)
No, those aren't from _FORTIFY_SOURCE; that macro doesn't cause warnings
like this, and even if it did, we're not setting it on spoonbill because
that isn't a Linux machine. Spoonbill's toolchain seems to be afflicted
with a case of terminal stupidity (I'd use a more colorful word but
I'm trying to be polite):
configure:15121: checking for quiet inline (no complaint if unreferenced)
configure:15151: gcc -o conftest -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement
-Wendif-labels-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g
-I/usr/local/include/libxml2-I/usr/local/include -I/usr/local/include -I/usr/local/include/libxml2
-I/usr/include/kerberosV-L/usr/local/lib/ -lpthread -lcrypto -liconv -L/usr/local/lib -L/usr/local/lib
-L/usr/local/libconftest.c -lxslt -lxml2 -lssl -lcrypto -lkrb5 -lz -ledit -ltermcap -lm >&5
/usr/local/lib//libxml2.so.12.0: warning: strcpy() is almost always misused, please use strlcpy()
/usr/local/lib//libxml2.so.12.0: warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib//libxslt.so.3.8: warning: sprintf() is often misused, please use snprintf()
Note that these warnings are not due to anything in the source code
being compiled; they appear to be looking at references in the libraries
it's including. Whoever thought that would be useful deserves nothing
but contempt, even if you grant that his bias against any use whatsoever
of these functions is legitimate (which I don't).
But having said that, it seems there's something wrong with the
STATIC_IF_INLINE patch for the case where USE_INLINE doesn't get set.
I'm too tired to investigate right now, but if you manually disable
USE_INLINE in pg_config.h after configuring, do you see a problem?
regards, tom lane
I wrote:
> But having said that, it seems there's something wrong with the
> STATIC_IF_INLINE patch for the case where USE_INLINE doesn't get set.
> I'm too tired to investigate right now, but if you manually disable
> USE_INLINE in pg_config.h after configuring, do you see a problem?
I summoned the energy to run a build with an old non-inline-aware
C compiler, and it went through, so the explanation is not that
simple.
Looking again at the spoonbill failure, it's striking that it gets as
far as plpython before choking. The symptoms are consistent with the
theory that USE_INLINE is getting #define'd by some Python header or
other, after having *not* been defined by pg_config.h. Maybe we should
rename that to PG_USE_INLINE?
BTW, just noticing that this theory espoused in plpython.h:
* Undefine some things that get (re)defined in the Python headers. They aren't
* used by the PL/Python code, and all PostgreSQL headers should be included
* earlier, so this should be pretty safe.
is entirely full of it, as the plpy_*.h files feel free to pull in
random Postgres header files. Maybe that area needs another look.
It's (probably) not contributing to this particular breakage, but I
can't say that I think undef'ing things like _POSIX_C_SOURCE midstream
is a bright idea at all. Seems to me that could easily result in
inconsistent results from reading different system header files.
regards, tom lane
Tom Lane wrote: > I wrote: > > But having said that, it seems there's something wrong with the > > STATIC_IF_INLINE patch for the case where USE_INLINE doesn't get set. > > I'm too tired to investigate right now, but if you manually disable > > USE_INLINE in pg_config.h after configuring, do you see a problem? > > I summoned the energy to run a build with an old non-inline-aware > C compiler, and it went through, so the explanation is not that > simple. Yeah, I had tried that, sorry for not making it explicit. > Looking again at the spoonbill failure, it's striking that it gets as > far as plpython before choking. The symptoms are consistent with the > theory that USE_INLINE is getting #define'd by some Python header or > other, after having *not* been defined by pg_config.h. Maybe we should > rename that to PG_USE_INLINE? Ahh, that makes a lot of sense, yes; and I see that the Python headers in my machine do mention USE_INLINE (though this is not defined, even on pyconfig.h), so it is indeed possible that on some other machines it is defined somewhere in those headers. I will go rename ours. Thanks for the research. > BTW, just noticing that this theory espoused in plpython.h: > > * Undefine some things that get (re)defined in the Python headers. They aren't > * used by the PL/Python code, and all PostgreSQL headers should be included > * earlier, so this should be pretty safe. > > is entirely full of it, as the plpy_*.h files feel free to pull in > random Postgres header files. Maybe that area needs another look. > It's (probably) not contributing to this particular breakage, but I > can't say that I think undef'ing things like _POSIX_C_SOURCE midstream > is a bright idea at all. Seems to me that could easily result in > inconsistent results from reading different system header files. Ugh. I notice that this seems to be a recent development; I think it only started with 147c2482 (and I made it worse at 0a664ec2 and c219d9b0). Maybe we should just clean that up a bit. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Looking again at the spoonbill failure, it's striking that it gets as
>> far as plpython before choking. The symptoms are consistent with the
>> theory that USE_INLINE is getting #define'd by some Python header or
>> other, after having *not* been defined by pg_config.h. Maybe we should
>> rename that to PG_USE_INLINE?
> Ahh, that makes a lot of sense, yes; and I see that the Python headers
> in my machine do mention USE_INLINE (though this is not defined, even on
> pyconfig.h), so it is indeed possible that on some other machines it is
> defined somewhere in those headers. I will go rename ours. Thanks for
> the research.
I'd not tried a build with plpython on my old HPUX box recently, but
looking into its Python headers I find
$ grep -r USE_INLINE /usr/local/include/python2.5
/usr/local/include/python2.5/pyport.h:#undef USE_INLINE /* XXX - set via configure? */
/usr/local/include/python2.5/pyport.h:#elif defined(USE_INLINE)
which means on this box things would have failed in the opposite
direction (undefined references to non-provided external functions)
without the rename. So that's the right fix for sure.
>> It's (probably) not contributing to this particular breakage, but I
>> can't say that I think undef'ing things like _POSIX_C_SOURCE midstream
>> is a bright idea at all. Seems to me that could easily result in
>> inconsistent results from reading different system header files.
> Ugh.
The more I think about that, the more worried I get. It's not just
that _POSIX_C_SOURCE might be defined or not, it's that the *value*
it has affects what definitions you get. At least that's how it
looks on my box. So our existing code potentially allows Python.h
to redefine it with a different value than what we'd defined it as,
which could result in inconsistencies between the system headers
included by postgres.h and those included later. I think this needs
reconsideration. Not sure what to do about it exactly, though :-(
regards, tom lane