Horton

Thread

Commits

  1. Always use -fPIC, not -fpic, when building shared libraries with gcc.

  1. Getting Citus into (Debian) PGDG

    Jason Petersen <jason@citusdata.com> — 2016-03-25T22:43:47Z

    Hey all,

The latest version of Citus (5.0) is now both open-source and a PostgreSQL extension. As such, I’m looking to get it into PGDG. It appears Devrim’s already taken my RPM spec file and build packages for those distros, so all that’s left are the Debian flavors.

I’ve been collecting my packaging work in a repo at https://github.com/citusdata/packaging <https://github.com/citusdata/packaging> , though the most interesting bit is the debian directory <https://github.com/citusdata/packaging/tree/525a46c8b6ac5ab7c52015f8b74bee254559de40/debian>. Christoph already suggested I add a watch file, which I’ve tested using uscan. It has a rule for finding the upstream signing key, which I’ve included in the directory (it’s my key <https://keybase.io/jasonmp85>, which I’ve been using to sign emails and git commits for some time now). If that part is unnecessary, it can be removed. Our release candidate version numbers use a hyphen, so I’ve included a mangler for that as well (to turn it into a tilde).

The extension builds unmodified on PostgreSQL 9.4 and 9.5 on Debian Jessie/Wheezy and Ubuntu Precise/Trusty/Wily. Lintian has no warnings or errors (save the typical one about not closing an ITP bug), and we’ve been using a Jessie package built with this repo in our Docker image.

I still have two remaining questions:

What do I do about the changelog? I’ve included one, but I expect it’s not useful and will need to be replaced by the ultimate packager

I’d really like to avoid having to have a special “debian” branch upstream just to have my debian directory available. I noticed many packages (psqlodbc, check-postgres as two examples) have only a debian folder in the apt repo. Since uscan/uupdate can update solely from a downloaded tarball, it seems like this should be an option. Can I take this approach?

Thanks for any help, and looking forward to having our project in the repo!

--
Jason Petersen
Software Engineer | Citus Data
303.736.9255
jason@citusdata.com

  2. Re: Getting Citus into (Debian) PGDG

    Jason Petersen <jason@citusdata.com> — 2016-03-25T23:03:16Z

    > On Mar 25, 2016, at 4:43 PM, Jason Petersen <jason@citusdata.com> wrote:
> 
> though the most interesting bit is the debian directory <https://github.com/citusdata/packaging/tree/525a46c8b6ac5ab7c52015f8b74bee254559de40/debian>

And of course I provided a link to a stale debian directory commit. Latest is here: https://github.com/citusdata/packaging/tree/8cc20250b8df0b0d8cfb95f1a68145e5ef11a205/debian <https://github.com/citusdata/packaging/tree/8cc20250b8df0b0d8cfb95f1a68145e5ef11a205/debian>

Sorry for any confusion.

--
Jason Petersen
Software Engineer | Citus Data
303.736.9255
jason@citusdata.com

  3. Re: Getting Citus into (Debian) PGDG

    Jason Petersen <jason@citusdata.com> — 2016-04-01T19:26:26Z

    > On Mar 25, 2016, at 4:43 PM, Jason Petersen <jason@citusdata.com> wrote:
> 
> Thanks for any help, and looking forward to having our project in the repo!


Is there anything I need to do to kick this off?

--
Jason Petersen
Software Engineer | Citus Data
303.736.9255
jason@citusdata.com

  4. Re: Getting Citus into (Debian) PGDG

    Christoph Berg <myon@debian.org> — 2016-04-26T15:13:39Z

    Re: Jason Petersen 2016-03-26 <980D4281-A01F-4F5D-9874-FFED701F403F@citusdata.com>
> > On Mar 25, 2016, at 4:43 PM, Jason Petersen <jason@citusdata.com> wrote:
> > 
> > though the most interesting bit is the debian directory <https://github.com/citusdata/packaging/tree/525a46c8b6ac5ab7c52015f8b74bee254559de40/debian>
> 
> And of course I provided a link to a stale debian directory commit. Latest is here: https://github.com/citusdata/packaging/tree/8cc20250b8df0b0d8cfb95f1a68145e5ef11a205/debian <https://github.com/citusdata/packaging/tree/8cc20250b8df0b0d8cfb95f1a68145e5ef11a205/debian>

Hi Jason,

to continue here after the discussion on IRC, citus was just accepted
into Debian unstable. In other words, welcome to the wonderful world
of interesting architectures with arcane features :)

https://buildd.debian.org/status/package.php?p=citus

That said, there's a build failure on s390x:

https://buildd.debian.org/status/fetch.php?pkg=citus&arch=s390x&ver=5.0.1-1&stamp=1461670278

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -I/usr/include/mit-krb5 -fPIC -pie  -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security  -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers -Wno-clobbered -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wmissing-declarations -Wmissing-prototypes -shared -o citus.so ./shared_library_init.o commands/create_distributed_table.o commands/transmit.o executor/multi_utility.o executor/multi_router_executor.o executor/multi_server_executor.o executor/multi_real_time_executor.o executor/multi_executor.o executor/multi_task_tracker_executor.o executor/multi_client_executor.o master/master_stage_protocol.o master/master_metadata_utility.o master/master_delete_protocol.o master/master_repair_shards.o master/master_create_shards.o master/worker_node_manager.o master/master_node_protocol.o planner/multi_physical_planner.o planner/multi_master_planner.o planner/multi_explain.o planner/multi_logical_planner.o planner/multi_planner.o planner/multi_join_order.o planner/modify_planner.o planner/multi_logical_optimizer.o relay/relay_event_utility.o test/generate_ddl_commands.o test/connection_utils.o test/prune_shard_list.o test/distribution_metadata.o test/test_helper_functions.o test/connection_cache.o test/fake_fdw.o test/create_shards.o utils/listutils.o utils/citus_readfuncs_94.o utils/ruleutils_94.o utils/multi_resowner.o utils/resource_lock.o utils/citus_outfuncs.o utils/metadata_cache.o utils/connection_cache.o utils/citus_read.o utils/citus_ruleutils.o utils/citus_nodefuncs.o utils/citus_readfuncs_95.o utils/ruleutils_95.o worker/worker_partition_protocol.o worker/task_tracker_protocol.o worker/task_tracker.o worker/worker_file_access_protocol.o worker/worker_data_fetch_protocol.o worker/worker_merge_protocol.o -L/usr/lib/s390x-linux-gnu -Wl,-z,relro -Wl,-z,now -Wl,--as-needed -L/usr/lib/mit-krb5 -L/usr/lib/s390x-linux-gnu/mit-krb5  -Wl,--as-needed -Wl,-z,relro  -L/usr/lib/s390x-linux-gnu -lpq 
utils/ruleutils_95.o: In function `simple_quote_literal':
/«PKGBUILDDIR»/src/backend/distributed/utils/ruleutils_95.c:6114:(.text+0x596): relocation truncated to fit: R_390_GOT12 against undefined symbol `standard_conforming_strings'

I've had that problem before, but forgot most of it. I think the
problem is that "-fPIC -pie  -fpic" isn't consistent and should
probably be "-fPIC -PIE", but I might be totally wrong. (I'll try to
dig up some docs.)

Christoph

  5. Re: Getting Citus into (Debian) PGDG

    Christoph Berg <myon@debian.org> — 2016-04-26T15:32:35Z

    Re: To Jason Petersen 2016-04-26 <20160426151339.GF12563@msg.df7cb.de>
> I've had that problem before, but forgot most of it. I think the
> problem is that "-fPIC -pie  -fpic" isn't consistent and should
> probably be "-fPIC -PIE", but I might be totally wrong. (I'll try to
> dig up some docs.)

Just talked to a s390x porter, the flags used should be -fPIC -pie",
i.e. without the lower case version.

Christoph

  6. Re: Getting Citus into (Debian) PGDG

    Andres Freund <andres@anarazel.de> — 2016-04-27T23:05:16Z

    On 2016-04-26 17:32:35 +0200, Christoph Berg wrote:
> Re: To Jason Petersen 2016-04-26 <20160426151339.GF12563@msg.df7cb.de>
> > I've had that problem before, but forgot most of it. I think the
> > problem is that "-fPIC -pie  -fpic" isn't consistent and should
> > probably be "-fPIC -PIE", but I might be totally wrong. (I'll try to
> > dig up some docs.)
> 
> Just talked to a s390x porter, the flags used should be -fPIC -pie",
> i.e. without the lower case version.

Afaics, that's citus independent, and coming from postgres code.
Makefile.port:
ifeq "$(findstring sparc,$(host_cpu))" "sparc"
CFLAGS_SL = -fPIC
else
CFLAGS_SL = -fpic
endif

I'm not clear why citus triggers this, when other extensions don't?

Andres

  7. relocation truncated to fit: citus build failure on s390x

    Christoph Berg <myon@debian.org> — 2016-04-28T08:08:24Z

    [Cc'ing -hackers]

I said:
> That said, there's a build failure on s390x:
> 
> https://buildd.debian.org/status/fetch.php?pkg=citus&arch=s390x&ver=5.0.1-1&stamp=1461670278
> 
> gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
> -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -g -O2 -fstack-protector-strong -Wformat
> -Werror=format-security -I/usr/include/mit-krb5 -fPIC -pie  -fpic -g -O2 -fstack-protector-strong -Wformat
> -Werror=format-security  -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers
> -Wno-clobbered -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wmissing-declarations
> -Wmissing-prototypes -shared -o citus.so ./shared_library_init.o commands/create_distributed_table.o
> commands/transmit.o executor/multi_utility.o executor/multi_router_executor.o executor/multi_server_executor.o
> executor/multi_real_time_executor.o executor/multi_executor.o executor/multi_task_tracker_executor.o
> executor/multi_client_executor.o master/master_stage_protocol.o master/master_metadata_utility.o
> master/master_delete_protocol.o master/master_repair_shards.o master/master_create_shards.o
> master/worker_node_manager.o master/master_node_protocol.o planner/multi_physical_planner.o
> planner/multi_master_planner.o planner/multi_explain.o planner/multi_logical_planner.o planner/multi_planner.o
> planner/multi_join_order.o planner/modify_planner.o planner/multi_logical_optimizer.o relay/relay_event_utility.o
> test/generate_ddl_commands.o test/connection_utils.o test/prune_shard_list.o test/distribution_metadata.o
> test/test_helper_functions.o test/connection_cache.o test/fake_fdw.o test/create_shards.o utils/listutils.o
> utils/citus_readfuncs_94.o utils/ruleutils_94.o utils/multi_resowner.o utils/resource_lock.o utils/citus_outfuncs.o
> utils/metadata_cache.o utils/connection_cache.o utils/citus_read.o utils/citus_ruleutils.o utils/citus_nodefuncs.o
> utils/citus_readfuncs_95.o utils/ruleutils_95.o worker/worker_partition_protocol.o worker/task_tracker_protocol.o
> worker/task_tracker.o worker/worker_file_access_protocol.o worker/worker_data_fetch_protocol.o
> worker/worker_merge_protocol.o -L/usr/lib/s390x-linux-gnu -Wl,-z,relro -Wl,-z,now -Wl,--as-needed -L/usr/lib/mit-krb5
> -L/usr/lib/s390x-linux-gnu/mit-krb5  -Wl,--as-needed -Wl,-z,relro  -L/usr/lib/s390x-linux-gnu -lpq
> utils/ruleutils_95.o: In function `simple_quote_literal':
> /«PKGBUILDDIR»/src/backend/distributed/utils/ruleutils_95.c:6114:(.text+0x596): relocation truncated to fit:
> R_390_GOT12 against undefined symbol `standard_conforming_strings'
> 
> I've had that problem before, but forgot most of it. I think the
> problem is that "-fPIC -pie  -fpic" isn't consistent and should
> probably be "-fPIC -PIE", but I might be totally wrong. (I'll try to
> dig up some docs.)

Re: Andres Freund 2016-04-28 <20160427230516.44fuzdxlifch2wwp@alap3.anarazel.de>
> > Just talked to a s390x porter, the flags used should be -fPIC -pie",
> > i.e. without the lower case version.
> 
> Afaics, that's citus independent, and coming from postgres code.
> Makefile.port:
> ifeq "$(findstring sparc,$(host_cpu))" "sparc"
> CFLAGS_SL = -fPIC
> else
> CFLAGS_SL = -fpic
> endif
> 
> 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.

(The m68k build has yet to happen, I'd guess it would exhibit the same
problem.)

Christoph

  8. Re: relocation truncated to fit: citus build failure on s390x

    Christoph Berg <myon@debian.org> — 2016-04-30T20:12:21Z

    Re: To Andres Freund 2016-04-28 <20160428080824.GA22412@msg.df7cb.de>
> 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.
> 
> (The m68k build has yet to happen, I'd guess it would exhibit the same
> problem.)

Fwiw, the m68k build finished successfully with -fpic:
https://buildd.debian.org/status/fetch.php?pkg=citus&arch=m68k&ver=5.0.1-1&stamp=1461978369

Christoph

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

    Christoph Berg <myon@debian.org> — 2017-05-29T15:58:50Z

    Re: To Andres Freund 2016-04-28 <20160428080824.GA22412@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&arch=s390x
https://buildd.debian.org/status/logs.php?pkg=pglogical&arch=s390x

Christoph

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-29T19:45:11Z

    Christoph Berg <myon@debian.org> writes:
> Re: To Andres Freund 2016-04-28 <20160428080824.GA22412@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

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

    Robert Haas <robertmhaas@gmail.com> — 2017-05-30T14:02:56Z

    On Mon, May 29, 2017 at 3:45 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-30T14:12:46Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, May 29, 2017 at 3:45 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

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

    Andres Freund <andres@anarazel.de> — 2017-05-30T16:15:41Z

    On 2017-05-29 15:45:11 -0400, Tom Lane wrote:
> Christoph Berg <myon@debian.org> writes:
> > Re: To Andres Freund 2016-04-28 <20160428080824.GA22412@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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-30T16:47:20Z

    Andres Freund <andres@anarazel.de> 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

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

    Christoph Berg <myon@debian.org> — 2017-05-30T16:52:58Z

    Re: Andres Freund 2017-05-30 <20170530161541.koj5xbvvovrrtxtd@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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-30T17:04:06Z

    Christoph Berg <myon@debian.org> 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

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

    Christoph Berg <myon@debian.org> — 2017-05-30T17:21:19Z

    Re: Tom Lane 2017-05-30 <25131.1496163846@sss.pgh.pa.us>
> Christoph Berg <myon@debian.org> 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-30T20:38:52Z

    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:
   text    data     bss     dec     hex filename
  85467    2632      64   88163   15863 postgres_fdw.so
-fPIC:
   text    data     bss     dec     hex filename
  85467    2632      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:
   text    data     bss     dec     hex filename
  86638     420      32   87090   15432 postgres_fdw.so
-fPIC:
   text    data     bss     dec     hex filename
  86474    1860      32   88366   1592e postgres_fdw.so
that's a 1.47% penalty

PPC, NetBSD 5.1.5:

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

HPPA, HPUX 10.20:

-fpic:
   text    data     bss     dec     hex filename
  97253   17296       8  114557   1bf7d postgres_fdw.sl
-fPIC:
   text    data     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

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

    Robert Haas <robertmhaas@gmail.com> — 2017-05-30T20:56:22Z

    On Tue, May 30, 2017 at 4:38 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

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

    Christoph Berg <myon@debian.org> — 2017-05-31T09:43:38Z

    Re: Tom Lane 2017-05-30 <1564.1496176732@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:

        text    data     bss     dec     hex filename
-fpic: 79520     928    1768   82216   14128 postgres_fdw.so
-fPIC: 79520     928    1768   82216   14128 postgres_fdw.so
-> no change

s390x, Debian unstable:

        text    data     bss     dec     hex filename
-fpic: 80735    2552      48   83335   14587 postgres_fdw.so
-fPIC: 81247    2552      48   83847   14787 postgres_fdw.so
-> +0.61%

arm64, Debian unstable:

        text    data     bss     dec     hex filename
-fpic: 64130    2600      48   66778   104da postgres_fdw.so
-fPIC: 64274    2600      48   66922   1056a postgres_fdw.so
-> +0.22%

sparc64, Debian unstable:

        text    data     bss     dec     hex filename
-fpic: 75804    3296      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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-31T13:55:31Z

    Christoph Berg <myon@debian.org> writes:
> Re: Tom Lane 2017-05-30 <1564.1496176732@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

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

    Christoph Berg <myon@debian.org> — 2017-05-31T15:09:40Z

    Re: Tom Lane 2017-05-31 <28752.1496238931@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

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-05-31T15:57:16Z

    Christoph Berg wrote:
> Re: Tom Lane 2017-05-31 <28752.1496238931@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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-31T17:05:25Z

    Alvaro Herrera <alvherre@2ndquadrant.com> 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

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

    Andres Freund <andres@anarazel.de> — 2017-05-31T17:48:26Z

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

+1

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-02T15:45:32Z

    Andres Freund <andres@anarazel.de> 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