Horton

Thread

Commits

  1. In transam.h, don't expose static inline functions to frontend code.

  1. FullTransactionId changes are causing portability issues

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-22T13:56:07Z

    Our Solaris packager reports that 12beta1 is failing to build for him
on some Solaris variants:

> The link failure is:

> ---
> Undefined			first referenced
>  symbol  			    in file
> ReadNextFullTransactionId           pg_checksums.o
> ld: fatal: symbol referencing errors. No output written to pg_checksums
> ---

> Now, ReadNextFullTransactionId() is implemented in
> src/backend/access/transam/varsup.c but I cannot see varsop.o being
> included in any of the libraries pg_checksum is linked against
> (libpgcommon.a and libpgport.a).

> When I check the pg_checksum.o I find that it references
> ReadNextFullTransactionId on the platforms that fail but not where it
> doesn't. The failed platforms are all sparc variants plus 64-bit x86
> on Solaris 11.

> The compiler used in Sun Studio 12u1, very old and and I can try to
> upgrade and see if that helps.
> [ it didn't ]

I'm a bit mystified why we did not see this problem in the buildfarm,
especially since we have at least one critter (damselfly) running an
OpenSolaris variant.  Nonetheless, it sure looks like a "somebody
was sloppy about frontend/backend separation" problem.

Fix ideas anyone?  I think we need to not only solve the immediate
problem (which might just take an #ifndef FRONTEND somewhere) but
also close the testing gap so we don't get blindsided like this
again.

			regards, tom lane

  2. Re: FullTransactionId changes are causing portability issues

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-22T14:48:08Z

    I wrote:
> Our Solaris packager reports that 12beta1 is failing to build for him
> on some Solaris variants:

>> The link failure is:
>> ---
>> Undefined			first referenced
>> symbol  			    in file
>> ReadNextFullTransactionId           pg_checksums.o
>> ld: fatal: symbol referencing errors. No output written to pg_checksums
>> ---

On looking closer, the fix is simple and matches what we've done
elsewhere: transam.h needs to have "#ifndef FRONTEND" to protect
its static inline function from being compiled into frontend code.

So the disturbing thing here is that we no longer have any active
buildfarm members that can build HEAD but have the won't-elide-
unused-static-functions problem.  Clearly we'd better close that
gap somehow ... anyone have an idea about how to test it better?

			regards, tom lane

  3. Re: FullTransactionId changes are causing portability issues

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-22T19:55:50Z

    I wrote:
>> Our Solaris packager reports that 12beta1 is failing to build for him
>> on some Solaris variants:
>>> The link failure is:
>>> Undefined			first referenced
>>> symbol  			    in file
>>> ReadNextFullTransactionId           pg_checksums.o

> On looking closer, the fix is simple and matches what we've done
> elsewhere: transam.h needs to have "#ifndef FRONTEND" to protect
> its static inline function from being compiled into frontend code.

> So the disturbing thing here is that we no longer have any active
> buildfarm members that can build HEAD but have the won't-elide-
> unused-static-functions problem.  Clearly we'd better close that
> gap somehow ... anyone have an idea about how to test it better?

Ah-hah --- some study of the gcc manual finds that modern versions
of gcc have

`-fkeep-inline-functions'
     In C, emit `static' functions that are declared `inline' into the
     object file, even if the function has been inlined into all of its
     callers.  This switch does not affect functions using the `extern
     inline' extension in GNU C89.  In C++, emit any and all inline
     functions into the object file.

This seems to do exactly what we need to test for this problem.
I've confirmed that with it turned on, a modern platform finds
the ReadNextFullTransactionId problem with yesterday's sources,
and that everything seems green as of HEAD.

So, we'd obviously not want to turn this on for normal builds,
but could we get a buildfarm animal or two to use this switch?

			regards, tom lane

  4. Re: FullTransactionId changes are causing portability issues

    Andres Freund <andres@anarazel.de> — 2019-05-22T20:03:12Z

    Hi,

On 2019-05-22 15:55:50 -0400, Tom Lane wrote:
> I wrote:
> >> Our Solaris packager reports that 12beta1 is failing to build for him
> >> on some Solaris variants:
> >>> The link failure is:
> >>> Undefined			first referenced
> >>> symbol  			    in file
> >>> ReadNextFullTransactionId           pg_checksums.o
> 
> > On looking closer, the fix is simple and matches what we've done
> > elsewhere: transam.h needs to have "#ifndef FRONTEND" to protect
> > its static inline function from being compiled into frontend code.
> 
> > So the disturbing thing here is that we no longer have any active
> > buildfarm members that can build HEAD but have the won't-elide-
> > unused-static-functions problem.  Clearly we'd better close that
> > gap somehow ... anyone have an idea about how to test it better?

I'm somewhat inclined to just declare that people using such old
compilers ought to just use something newer. Having to work around
broken compilers that are so old that we don't even have a buildfarm
animal actually exposing that behaviour, seems like wasted effort. IMO
it'd make sense to just treat this as part of the requirements for a C99
compiler.


> Ah-hah --- some study of the gcc manual finds that modern versions
> of gcc have
> 
> `-fkeep-inline-functions'
>      In C, emit `static' functions that are declared `inline' into the
>      object file, even if the function has been inlined into all of its
>      callers.  This switch does not affect functions using the `extern
>      inline' extension in GNU C89.  In C++, emit any and all inline
>      functions into the object file.
> 
> This seems to do exactly what we need to test for this problem.
> I've confirmed that with it turned on, a modern platform finds
> the ReadNextFullTransactionId problem with yesterday's sources,
> and that everything seems green as of HEAD.
> 
> So, we'd obviously not want to turn this on for normal builds,
> but could we get a buildfarm animal or two to use this switch?

I could easily add that to one of mine, if we decide to go for that.

Greetings,

Andres Freund

  5. Re: FullTransactionId changes are causing portability issues

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-22T20:13:02Z

    Andres Freund <andres@anarazel.de> writes:
> On 2019-05-22 15:55:50 -0400, Tom Lane wrote:
>>> So the disturbing thing here is that we no longer have any active
>>> buildfarm members that can build HEAD but have the won't-elide-
>>> unused-static-functions problem.  Clearly we'd better close that
>>> gap somehow ... anyone have an idea about how to test it better?

> I'm somewhat inclined to just declare that people using such old
> compilers ought to just use something newer. Having to work around
> broken compilers that are so old that we don't even have a buildfarm
> animal actually exposing that behaviour, seems like wasted effort. IMO
> it'd make sense to just treat this as part of the requirements for a C99
> compiler.

TBH, I too supposed the requirement for this had gone away with the C99
move.  But according to the discussion today on -packagers, there are
still supported variants of Solaris that have compilers that speak C99
but don't have this behavior.  Per Bjorn's report:

>> The compiler used in Sun Studio 12u1, very old and and I can try to
>> upgrade and see if that helps.
> I tried Sun Studio 12u2 and then a more drastic upgrade to Developer
> Studio 12.5 but both had the same effect.

It doesn't sound like "use a newer compiler" is going to be a helpful
answer there.

(It is annoying that nobody is running such a platform in the buildfarm,
I agree.  But I don't have the resources to spin up a real-Solaris
buildfarm member.)

			regards, tom lane

  6. Re: FullTransactionId changes are causing portability issues

    Andres Freund <andres@anarazel.de> — 2019-05-23T17:11:34Z

    Hi,

On 2019-05-22 16:13:02 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-05-22 15:55:50 -0400, Tom Lane wrote:
> Per Bjorn's report:
> >> The compiler used in Sun Studio 12u1, very old and and I can try to
> >> upgrade and see if that helps.
> > I tried Sun Studio 12u2 and then a more drastic upgrade to Developer
> > Studio 12.5 but both had the same effect.
> 
> It doesn't sound like "use a newer compiler" is going to be a helpful
> answer there.

Well, GCC is available on solaris, and IIRC not that hard to install
(isn't it just a 'pkg install gcc' or such?). Don't think we need to
invest a lot of energy fixing a compiler / OS combo that effectively
isn't developed further.

Greetings,

Andres Freund

  7. Re: FullTransactionId changes are causing portability issues

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-23T17:46:15Z

    Andres Freund <andres@anarazel.de> writes:
> On 2019-05-22 16:13:02 -0400, Tom Lane wrote:
>> It doesn't sound like "use a newer compiler" is going to be a helpful
>> answer there.

> Well, GCC is available on solaris, and IIRC not that hard to install
> (isn't it just a 'pkg install gcc' or such?). Don't think we need to
> invest a lot of energy fixing a compiler / OS combo that effectively
> isn't developed further.

I'm not really excited about adopting a position that PG will only
build on GCC and clones thereof.

			regards, tom lane

  8. Re: FullTransactionId changes are causing portability issues

    Andres Freund <andres@anarazel.de> — 2019-05-23T17:52:31Z

    Hi,

On 2019-05-23 13:46:15 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-05-22 16:13:02 -0400, Tom Lane wrote:
> >> It doesn't sound like "use a newer compiler" is going to be a helpful
> >> answer there.
> 
> > Well, GCC is available on solaris, and IIRC not that hard to install
> > (isn't it just a 'pkg install gcc' or such?). Don't think we need to
> > invest a lot of energy fixing a compiler / OS combo that effectively
> > isn't developed further.
> 
> I'm not really excited about adopting a position that PG will only
> build on GCC and clones thereof.

That's not what I said though? Not supporting one compiler, on an OS
that's effectively not being developed anymore, with a pretty
indefensible behaviour, requiring not insignificant work by everyone,
isn't the same as standardizing on gcc. I mean, we obviously are going
to continue at the absolute very least gcc, llvm/clang and msvc.

Greetings,

Andres Freund

  9. Re: FullTransactionId changes are causing portability issues

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-23T18:05:19Z

    Andres Freund <andres@anarazel.de> writes:
> On 2019-05-23 13:46:15 -0400, Tom Lane wrote:
>> I'm not really excited about adopting a position that PG will only
>> build on GCC and clones thereof.

> That's not what I said though? Not supporting one compiler, on an OS
> that's effectively not being developed anymore, with a pretty
> indefensible behaviour, requiring not insignificant work by everyone,
> isn't the same as standardizing on gcc. I mean, we obviously are going
> to continue at the absolute very least gcc, llvm/clang and msvc.

I think you're vastly overstating the case for refusing support for this.
Adding "#ifndef FRONTEND" to relevant headers isn't a huge amount of work
--- it's certainly far less of a problem than the Microsoft-droppings
we've had to put in in so many places.  The only real issue in my mind
is the lack of buildfarm support for detecting that we need to do so.

Also relevant here is that you have no evidence for the assumption that
these old Solaris compilers are the only live platform with the problem.
Yeah, we wish our buildfarm covered everything of interest, but it does
not.  Maybe, if we get to beta2 without any additional reports of build
failures on beta1, that would be a bit of evidence that nobody else cares
--- but we have no such evidence right now.  We certainly can't assume
that any pre-v12 release provides evidence of that, because up till
I retired pademelon, it was forcing us to keep this case supported.

			regards, tom lane

  10. Re: FullTransactionId changes are causing portability issues

    Andres Freund <andres@anarazel.de> — 2019-05-23T18:49:08Z

    Hi,

On 2019-05-23 14:05:19 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-05-23 13:46:15 -0400, Tom Lane wrote:
> >> I'm not really excited about adopting a position that PG will only
> >> build on GCC and clones thereof.
> 
> > That's not what I said though? Not supporting one compiler, on an OS
> > that's effectively not being developed anymore, with a pretty
> > indefensible behaviour, requiring not insignificant work by everyone,
> > isn't the same as standardizing on gcc. I mean, we obviously are going
> > to continue at the absolute very least gcc, llvm/clang and msvc.
> 
> I think you're vastly overstating the case for refusing support for this.
> Adding "#ifndef FRONTEND" to relevant headers isn't a huge amount of work
> --- it's certainly far less of a problem than the Microsoft-droppings
> we've had to put in in so many places.  The only real issue in my mind
> is the lack of buildfarm support for detecting that we need to do so.

Well, doing it for every single inline function is pretty annoying, just
from a bulkiness perspective. And figuring out exactly which inline
function needs this isn't easy without something that actually shows the
problem.


> Also relevant here is that you have no evidence for the assumption that
> these old Solaris compilers are the only live platform with the problem.
> Yeah, we wish our buildfarm covered everything of interest, but it does
> not.  Maybe, if we get to beta2 without any additional reports of build
> failures on beta1, that would be a bit of evidence that nobody else cares
> --- but we have no such evidence right now.  We certainly can't assume
> that any pre-v12 release provides evidence of that, because up till
> I retired pademelon, it was forcing us to keep this case supported.

I don't think I'm advocating for not fixing the issue we had for
solaris, for 12. I just don't think this a reasonable approach going
forward.

Greetings,

Andres Freund

  11. Re: FullTransactionId changes are causing portability issues

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-23T22:27:53Z

    Andres Freund <andres@anarazel.de> writes:
> On 2019-05-23 14:05:19 -0400, Tom Lane wrote:
>> I think you're vastly overstating the case for refusing support for this.
>> Adding "#ifndef FRONTEND" to relevant headers isn't a huge amount of work
>> --- it's certainly far less of a problem than the Microsoft-droppings
>> we've had to put in in so many places.  The only real issue in my mind
>> is the lack of buildfarm support for detecting that we need to do so.

> Well, doing it for every single inline function is pretty annoying, just
> from a bulkiness perspective.

Oh, I certainly wasn't suggesting we do that.

> And figuring out exactly which inline
> function needs this isn't easy without something that actually shows the
> problem.

... which is why we need a buildfarm animal that shows the problem.
We had some, up until the C99 move.

If the only practical way to detect the issue were to run some ancient
platform or other, I'd tend to agree with you that it's not worth the
trouble.  But if we can spot it just by using -fkeep-inline-functions
on an animal or two, I think it's a reasonable thing to keep supporting
the case for a few years more.

			regards, tom lane