Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

Andres Freund  writes:
> On 2017-05-31 11:57:16 -0400, Alvaro Herrera wrote:
>> My vote would be to backpatch it all the way.

> +1

Done, buildfarm seems happy.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Andres Freund]

On 2017-05-31 11:57:16 -0400, Alvaro Herrera wrote:
> My vote would be to backpatch it all the way.

+1


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

Alvaro Herrera  writes:
> My vote would be to backpatch it all the way.

That's my thought too.  Otherwise it'll be five years before extension
authors can stop worrying about this issue.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Alvaro Herrera]

Christoph Berg wrote:
> Re: Tom Lane 2017-05-31 <28752.1496238...@sss.pgh.pa.us>

> > Next question: should we back-patch this change, or just do it in HEAD?
> 
> Debian "needs" it for 9.6, but I've already pushed the s390x patch in
> the original posting, so I could just live with it being just in head.
> But of course it would be nice to have everything in sync.

I think it's only a problem for you in 9.6-only because you've not tried
pglogical or some other large-shlib extension with earlier branches; in
other words, eventually this is going to bite somebody using the old
branches as well, unless we believe that the platforms are dead enough
that nobody really cares other than for academic purposes.

My vote would be to backpatch it all the way.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Christoph Berg]

Re: Tom Lane 2017-05-31 <28752.1496238...@sss.pgh.pa.us>
> OK, this looks good to me.  Just to make sure everyone's on the
> same page, what I propose to do is simplify all our platform-specific
> Makefiles that use either -fpic or -fPIC to use -fPIC unconditionally.
> This affects the netbsd, linux, and openbsd ports.  Looks like we should
> also change the example link commands in dfunc.sgml.

Ack.

> Next question: should we back-patch this change, or just do it in HEAD?

Debian "needs" it for 9.6, but I've already pushed the s390x patch in
the original posting, so I could just live with it being just in head.
But of course it would be nice to have everything in sync.

Christoph


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

Christoph Berg  writes:
> Re: Tom Lane 2017-05-30 <1564.1496176...@sss.pgh.pa.us>
>> It'd be interesting if people could gather similar numbers on other
>> platforms of more real-world relevance, such as ppc64.  But based on
>> this small sample, I wouldn't object to just going to -fPIC across
>> the board.

> [ more numbers ]

OK, this looks good to me.  Just to make sure everyone's on the
same page, what I propose to do is simplify all our platform-specific
Makefiles that use either -fpic or -fPIC to use -fPIC unconditionally.
This affects the netbsd, linux, and openbsd ports.  Looks like we should
also change the example link commands in dfunc.sgml.

Next question: should we back-patch this change, or just do it in HEAD?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Christoph Berg]

Re: Tom Lane 2017-05-30 <1564.1496176...@sss.pgh.pa.us>
> It'd be interesting if people could gather similar numbers on other
> platforms of more real-world relevance, such as ppc64.  But based on
> this small sample, I wouldn't object to just going to -fPIC across
> the board.

ppc64el, Debian unstable:

textdata bss dec hex filename
-fpic: 79520 9281768   82216   14128 postgres_fdw.so
-fPIC: 79520 9281768   82216   14128 postgres_fdw.so
-> no change

s390x, Debian unstable:

textdata bss dec hex filename
-fpic: 807352552  48   83335   14587 postgres_fdw.so
-fPIC: 812472552  48   83847   14787 postgres_fdw.so
-> +0.61%

arm64, Debian unstable:

textdata bss dec hex filename
-fpic: 641302600  48   66778   104da postgres_fdw.so
-fPIC: 642742600  48   66922   1056a postgres_fdw.so
-> +0.22%

sparc64, Debian unstable:

textdata bss dec hex filename
-fpic: 758043296  48   79148   1352c postgres_fdw.so
-fPIC: 72748 920  48   73716   11ff4 postgres_fdw.so
-> 6.9% decrease (!)

9.6.3, gcc (Debian 6.3.0-18) 6.3.0 20170516, -O2, all objects unstripped
(sparc64 is gcc (Debian 6.3.0-17) 6.3.0 20170510)

Christoph


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Robert Haas]

On Tue, May 30, 2017 at 4:38 PM, Tom Lane  wrote:
> It'd be interesting if people could gather similar numbers on other
> platforms of more real-world relevance, such as ppc64.  But based on
> this small sample, I wouldn't object to just going to -fPIC across
> the board.

That seems pretty sensible to me.  I think we should aim for a
configuration that works by default.  If we use -fPIC where -fpic
would have been better, the result is that on some platforms there
might be a speed penalty, probably small.  In the reverse situation,
the build fails.  I believe that the average PostgreSQL extension
developer will prefer the first situation.  If somebody has got an
extension which is small enough to use -fpic and that person cares
about minimizing the performance penalty on s390 or other platforms
where they have this problem, then they can arrange an override in the
opposite direction.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

I wrote:
> Very possibly true, but I wish we had some hard facts and not just
> guesses.

As a simple but on-point test, I compared sizes of postgres_fdw.so
built with -fpic and -fPIC.  I no longer have access to a wide variety
of weird architectures, but on what I do have in my office:

x86_64, RHEL6:

-fpic:
   textdata bss dec hex filename
  854672632  64   88163   15863 postgres_fdw.so
-fPIC:
   textdata bss dec hex filename
  854672632  64   88163   15863 postgres_fdw.so

This seems to confirm Andres' opinion that it makes no difference on
x86_64.

PPC, FreeBSD 10.3:

-fpic:
   textdata bss dec hex filename
  86638 420  32   87090   15432 postgres_fdw.so
-fPIC:
   textdata bss dec hex filename
  864741860  32   88366   1592e postgres_fdw.so
that's a 1.47% penalty

PPC, NetBSD 5.1.5:

-fpic:
   textdata bss dec hex filename
  81880 420  56   82356   141b4 postgres_fdw.so
-fPIC:
   textdata bss dec hex filename
  816882044  56   83788   1474c postgres_fdw.so
that's a 1.74% penalty

HPPA, HPUX 10.20:

-fpic:
   textdata bss dec hex filename
  97253   17296   8  114557   1bf7d postgres_fdw.sl
-fPIC:
   textdata bss dec hex filename
 102629   17320   8  119957   1d495 postgres_fdw.sl
that's a 4.7% penalty


It's somewhat noteworthy that the PPC builds show a large increase in data
segment size.  That likely corresponds to relocatable pointer fields that
have to be massaged by the dynamic linker during shlib load.  Thus one
could speculate that shlib load might be noticeably slower with -fPIC,
at least on PPC.  But people rarely complain about the speed of that
anyway, so it's unlikely to be worth worrying about.

It'd be interesting if people could gather similar numbers on other
platforms of more real-world relevance, such as ppc64.  But based on
this small sample, I wouldn't object to just going to -fPIC across
the board.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Christoph Berg]

Re: Tom Lane 2017-05-30 <25131.1496163...@sss.pgh.pa.us>
> Christoph Berg  writes:
> > My main point here would be that we are already setting this for all
> > extensions for sparc and sparc64, so s390(x) would just follow suit.
> 
> For some values of "we", sure ;-).

Afaict for all values of "we":

ifeq "$(findstring sparc,$(host_cpu))" "sparc"
CFLAGS_SL = -fPIC
else
CFLAGS_SL = -fpic
endif

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/makefiles/Makefile.linux

Christoph


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

Christoph Berg  writes:
> My main point here would be that we are already setting this for all
> extensions for sparc and sparc64, so s390(x) would just follow suit.

For some values of "we", sure ;-).  But I think what is really under
discussion here is whether to change -fpic to -fPIC for all platforms.
It looks like Makefile.netbsd and Makefile.openbsd would be affected along
with Makefile.linux.  Some other platforms such as freebsd are already
in the fPIC-always camp.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Christoph Berg]

Re: Andres Freund 2017-05-30 <20170530161541.koj5xbvvovrrt...@alap3.anarazel.de>
> I think we can fix this easily enough in Citus, postgis, and whatever.
> But it's not a particularly good user/developer experience, and
> presumably is going to become more and more common. On x86 there
> shouldn't be a difference in efficiency between the two, but some others
> will see some.  Given that most distributions switched to building the
> main executables with -fPIE anyway, to allow for ASLR, it seems unlikely
> that the intra extension overhead is going to be very meaningful in
> comparison.

My main point here would be that we are already setting this for all
extensions for sparc and sparc64, so s390(x) would just follow suit.

-fPIC is the default in Debian now, see the discussion starting at
https://lists.debian.org/debian-devel/2016/05/msg00028.html
including the Fedora: 
https://lists.debian.org/debian-devel/2016/05/msg00219.html
and Ubuntu: https://lists.debian.org/debian-devel/2016/05/msg00225.html
situation, which all do that.

Christoph


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

Andres Freund  writes:
> On 2017-05-29 15:45:11 -0400, Tom Lane wrote:
>> Maybe this is small enough to not be something we need to worry about,
>> but I'm wondering if we should ask citus and other large .so's to set
>> some additional make flag that would cue usage of -fPIC over -fpic.

> I think we can fix this easily enough in Citus, postgis, and whatever.
> But it's not a particularly good user/developer experience, and
> presumably is going to become more and more common. On x86 there
> shouldn't be a difference in efficiency between the two, but some others
> will see some.  Given that most distributions switched to building the
> main executables with -fPIE anyway, to allow for ASLR, it seems unlikely
> that the intra extension overhead is going to be very meaningful in
> comparison.

Very possibly true, but I wish we had some hard facts and not just
guesses.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Andres Freund]

On 2017-05-29 15:45:11 -0400, Tom Lane wrote:
> Christoph Berg  writes:
> > Re: To Andres Freund 2016-04-28 <20160428080824.ga22...@msg.df7cb.de>
> >>> I'm not clear why citus triggers this, when other extensions don't?
> 
> >> Maybe it's simply because citus.so is bigger than all the other
> >> extension .so files:
> 
> I wonder what the overhead is of using -fPIC when -fpic would be
> sufficient.  Whatever it is, the proposed patch imposes it on every
> shlib or extension, to accommodate one single extension that isn't
> even one we ship.

> Maybe this is small enough to not be something we need to worry about,
> but I'm wondering if we should ask citus and other large .so's to set
> some additional make flag that would cue usage of -fPIC over -fpic.

I think we can fix this easily enough in Citus, postgis, and whatever.
But it's not a particularly good user/developer experience, and
presumably is going to become more and more common. On x86 there
shouldn't be a difference in efficiency between the two, but some others
will see some.  Given that most distributions switched to building the
main executables with -fPIE anyway, to allow for ASLR, it seems unlikely
that the intra extension overhead is going to be very meaningful in
comparison.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

Robert Haas  writes:
> On Mon, May 29, 2017 at 3:45 PM, Tom Lane  wrote:
>> I wonder what the overhead is of using -fPIC when -fpic would be
>> sufficient.

> Do we have an idea how to measure the increased overhead?  Just from
> reading the description, I'm guessing that the increased cost would
> happen when the extension calls back into core, but maybe that doesn't
> happen often enough to worry about?

My gut feeling is that it'd be a pretty distributed cost, because every
internal cross-reference in the .so (for instance, loading the address of
a string literal) would involve a bit more overhead to support a wider
offset field.  An easy thing to look at would be how much the code expands
by.  That might or might not be a good proxy for the runtime slowdown
percentage, but it seems like it ought to serve as a zero-order
approximation.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Robert Haas]

On Mon, May 29, 2017 at 3:45 PM, Tom Lane  wrote:
> I wonder what the overhead is of using -fPIC when -fpic would be
> sufficient.  Whatever it is, the proposed patch imposes it on every
> shlib or extension, to accommodate one single extension that isn't
> even one we ship.
>
> Maybe this is small enough to not be something we need to worry about,
> but I'm wondering if we should ask citus and other large .so's to set
> some additional make flag that would cue usage of -fPIC over -fpic.

Do we have an idea how to measure the increased overhead?  Just from
reading the description, I'm guessing that the increased cost would
happen when the extension calls back into core, but maybe that doesn't
happen often enough to worry about?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

Christoph Berg  writes:
> Re: To Andres Freund 2016-04-28 <20160428080824.ga22...@msg.df7cb.de>
>>> I'm not clear why citus triggers this, when other extensions don't?

>> Maybe it's simply because citus.so is bigger than all the other
>> extension .so files:

I wonder what the overhead is of using -fPIC when -fpic would be
sufficient.  Whatever it is, the proposed patch imposes it on every
shlib or extension, to accommodate one single extension that isn't
even one we ship.

Maybe this is small enough to not be something we need to worry about,
but I'm wondering if we should ask citus and other large .so's to set
some additional make flag that would cue usage of -fPIC over -fpic.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Christoph Berg]

Re: To Andres Freund 2016-04-28 <20160428080824.ga22...@msg.df7cb.de>
> > I'm not clear why citus triggers this, when other extensions don't?
> 
> Maybe it's simply because citus.so is bigger than all the other
> extension .so files:
> 
>-fpic
>  Generate position-independent code (PIC) suitable for use
>  in a shared library, if supported for the target machine.
>  Such code accesses all constant addresses through a global
>  offset table (GOT).  The dynamic loader resolves the GOT
>  entries when the program starts (the dynamic loader is not
>  part of GCC; it is part of the operating system).  If the
>  GOT size for the linked executable exceeds a machine-
>  specific maximum size, you get an error message from the
>  linker indicating that -fpic does not work; in that case,
>  recompile with -fPIC instead.  (These maximums are 8k on
>  the SPARC and 32k on the m68k and RS/6000.  The 386 has no
>  such limit.)
> 
>  Position-independent code requires special support, and
>  therefore works only on certain machines.  For the 386, GCC
>  supports PIC for System V but not for the Sun 386i.  Code
>  generated for the IBM RS/6000 is always
>  position-independent.
> 
>  When this flag is set, the macros "__pic__" and "__PIC__"
>  are defined to 1.
> 
>-fPIC
>  If supported for the target machine, emit
>  position-independent code, suitable for dynamic linking and
>  avoiding any limit on the size of the global offset table.
>  This option makes a difference on the m68k, PowerPC and
>  SPARC.
> 
>  Position-independent code requires special support, and
>  therefore works only on certain machines.
> 
>  When this flag is set, the macros "__pic__" and "__PIC__"
>  are defined to 2.
> 
> This doesn't mention s390(x), but citus.so 382952 bytes (on amd64) is
> well beyond the 8k/32k limits mentioned above.
> 
> PostgreSQL itself links correctly on s390x:
> ... -I/usr/include/mit-krb5 -fPIC -pie -I../../../../src/include
> 
> I'm not an expert in compiler flags, but it seems to me CFLAGS_SL is
> wrong on s390(x) in Makefile.port, it should use -fPIC like sparc.

After talking to a s390x Debian porter, -fPIC is the correct flag to
use. The quoted patch made the previously failing builds for citus and
pglogical (which have larger-than-average .so files) on s390x succeed
(and the sparc64 case still works):

--- a/src/makefiles/Makefile.linux
+++ b/src/makefiles/Makefile.linux
@@ -5,7 +5,8 @@ export_dynamic = -Wl,-E
 rpath = -Wl,-rpath,'$(rpathdir)',--enable-new-dtags
 DLSUFFIX = .so
 
-ifeq "$(findstring sparc,$(host_cpu))" "sparc"
+# Enable -fPIC to avoid "relocation truncated to fit" linker errors
+ifneq "$(filter sparc% s390%,$(host_cpu))" ""
 CFLAGS_SL = -fPIC
 else
 CFLAGS_SL = -fpic


The patch was made against 9.6; I'd opt to include it in master and
the back branches.

https://buildd.debian.org/status/logs.php?pkg=citus=s390x
https://buildd.debian.org/status/logs.php?pkg=pglogical=s390x

Christoph


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers