Thread: Removing link-time cross-module refs in contrib
Pursuant to Andres' suggestion in https://www.postgresql.org/message-id/20161002223927.57xns3arkdg4hu6x@alap3.anarazel.de attached is a draft patch that gets rid of link-time references from hstore_plpython to both hstore and plpython. I've verified that this allows "LOAD 'hstore_plpython'" to succeed in a fresh session without having loaded the prerequisite modules first. The patch seems pretty successful in terms of being noninvasive to the code. I think the major objection to it would be that we no longer have any direct compiler-verified connection between the signatures of the called functions in hstore/plpython and what hstore_plpython thinks they are. That is, if someone were to modify hstore and change the signature of say hstoreCheckKeyLen, this would not cause any compiler complaints in hstore_plpython, just runtime problems. A slightly ugly way of alleviating that risk would be to put the function typedefs right beside the externs in the originating modules, eg in hstore.h extern size_t hstoreCheckKeyLen(size_t len); +typedef size_t (*hstoreCheckKeyLen_t) (size_t len); I'm not sure if that is a net improvement or not --- thoughts? If we were to push forward with this idea, the remaining work would be to fix the other two contrib transform modules similarly, after which I would want to revert the changes in commit cac765820 and later that suppressed linker errors for unresolved symbols in contrib links. The possibility of getting back that error detection is actually the main motivation for this IMO. Comments? regards, tom lane diff --git a/contrib/hstore_plpython/Makefile b/contrib/hstore_plpython/Makefile index c4dad6f..a55c9a1 100644 *** a/contrib/hstore_plpython/Makefile --- b/contrib/hstore_plpython/Makefile *************** MODULE_big = hstore_plpython$(python_maj *** 4,10 **** OBJS = hstore_plpython.o $(WIN32RES) PGFILEDESC = "hstore_plpython - hstore transform for plpython" ! PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec) -I$(top_srcdir)/contrib/hstore EXTENSION = hstore_plpythonu hstore_plpython2u hstore_plpython3u DATA = hstore_plpythonu--1.0.sql hstore_plpython2u--1.0.sql hstore_plpython3u--1.0.sql --- 4,10 ---- OBJS = hstore_plpython.o $(WIN32RES) PGFILEDESC = "hstore_plpython - hstore transform for plpython" ! PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec) -I$(top_srcdir)/contrib/hstore -DPLPYTHON_LIBNAME='"plpython$(python_majorversion)"' EXTENSION = hstore_plpythonu hstore_plpython2u hstore_plpython3u DATA = hstore_plpythonu--1.0.sql hstore_plpython2u--1.0.sql hstore_plpython3u--1.0.sql *************** include $(top_builddir)/src/Makefile.glo *** 23,41 **** include $(top_srcdir)/contrib/contrib-global.mk endif ! # In configurations that forbid undefined symbols in libraries, link with each ! # dependency. This does preclude pgxs builds. ifeq ($(PORTNAME), aix) rpathdir = $(pkglibdir):$(python_libdir) ! SHLIB_LINK += ../hstore/libhstore.exp $(python_libspec) $(python_additional_libs) $(sort $(wildcard ../../src/pl/plpython/libplpython*.exp)) ! endif ifeq ($(PORTNAME), win32) ! SHLIB_LINK += ../hstore/libhstore.a $(sort $(wildcard ../../src/pl/plpython/libpython*.a)) $(sort $(wildcard ../../src/pl/plpython/libplpython*.a)) endif - - ifeq ($(PORTNAME), cygwin) - SHLIB_LINK += -L../hstore -lhstore -L../../src/pl/plpython \ - -lplpython$(python_majorversion) $(python_libspec) endif REGRESS_OPTS += --load-extension=hstore --- 23,40 ---- include $(top_srcdir)/contrib/contrib-global.mk endif ! # We must link libpython explicitly ifeq ($(PORTNAME), aix) rpathdir = $(pkglibdir):$(python_libdir) ! SHLIB_LINK += $(python_libspec) $(python_additional_libs) ! else ifeq ($(PORTNAME), win32) ! # ... see silliness in plpython Makefile ... ! SHLIB_LINK += $(sort $(wildcard ../../src/pl/plpython/libpython*.a)) ! else ! rpathdir = $(python_libdir) ! SHLIB_LINK += $(python_libspec) endif endif REGRESS_OPTS += --load-extension=hstore diff --git a/contrib/hstore_plpython/hstore_plpython.c b/contrib/hstore_plpython/hstore_plpython.c index 6f2751a..9e2eb2f 100644 *** a/contrib/hstore_plpython/hstore_plpython.c --- b/contrib/hstore_plpython/hstore_plpython.c *************** *** 1,4 **** --- 1,5 ---- #include "postgres.h" + #include "fmgr.h" #include "plpython.h" #include "plpy_typeio.h" *************** *** 6,11 **** --- 7,67 ---- PG_MODULE_MAGIC; + extern void _PG_init(void); + + /* Linkage to functions in plpython module */ + typedef char *(*PLyObject_AsString_t) (PyObject *plrv); + + static PLyObject_AsString_t PLyObject_AsString_p; + + #define PLyObject_AsString PLyObject_AsString_p + + /* Linkage to functions in hstore module */ + typedef HStore *(*hstoreUpgrade_t) (Datum orig); + typedef int (*hstoreUniquePairs_t) (Pairs *a, int32 l, int32 *buflen); + typedef HStore *(*hstorePairs_t) (Pairs *pairs, int32 pcount, int32 buflen); + typedef size_t (*hstoreCheckKeyLen_t) (size_t len); + typedef size_t (*hstoreCheckValLen_t) (size_t len); + + static hstoreUpgrade_t hstoreUpgrade_p; + static hstoreUniquePairs_t hstoreUniquePairs_p; + static hstorePairs_t hstorePairs_p; + static hstoreCheckKeyLen_t hstoreCheckKeyLen_p; + static hstoreCheckValLen_t hstoreCheckValLen_p; + + #define hstoreUpgrade hstoreUpgrade_p + #define hstoreUniquePairs hstoreUniquePairs_p + #define hstorePairs hstorePairs_p + #define hstoreCheckKeyLen hstoreCheckKeyLen_p + #define hstoreCheckValLen hstoreCheckValLen_p + + + /* + * Module initialize function: fetch function pointers for cross-module calls. + */ + void + _PG_init(void) + { + PLyObject_AsString_p = (PLyObject_AsString_t) + load_external_function("$libdir/" PLPYTHON_LIBNAME, "PLyObject_AsString", + true, NULL); + hstoreUpgrade_p = (hstoreUpgrade_t) + load_external_function("$libdir/hstore", "hstoreUpgrade", + true, NULL); + hstoreUniquePairs_p = (hstoreUniquePairs_t) + load_external_function("$libdir/hstore", "hstoreUniquePairs", + true, NULL); + hstorePairs_p = (hstorePairs_t) + load_external_function("$libdir/hstore", "hstorePairs", + true, NULL); + hstoreCheckKeyLen_p = (hstoreCheckKeyLen_t) + load_external_function("$libdir/hstore", "hstoreCheckKeyLen", + true, NULL); + hstoreCheckValLen_p = (hstoreCheckValLen_t) + load_external_function("$libdir/hstore", "hstoreCheckValLen", + true, NULL); + } + PG_FUNCTION_INFO_V1(hstore_to_plpython); diff --git a/contrib/hstore_plpython/hstore_plpython2u--1.0.sql b/contrib/hstore_plpython/hstore_plpython2u--1.0.sql index e3aea63..800765f 100644 *** a/contrib/hstore_plpython/hstore_plpython2u--1.0.sql --- b/contrib/hstore_plpython/hstore_plpython2u--1.0.sql *************** *** 3,13 **** -- complain if script is sourced in psql, rather than via CREATE EXTENSION \echo Use "CREATE EXTENSION hstore_plpython2u" to load this file. \quit - -- make sure the prerequisite libraries are loaded - LOAD 'plpython2'; - SELECT NULL::hstore; - - CREATE FUNCTION hstore_to_plpython2(val internal) RETURNS internal LANGUAGE C STRICT IMMUTABLE AS 'MODULE_PATHNAME', 'hstore_to_plpython'; --- 3,8 ---- diff --git a/contrib/hstore_plpython/hstore_plpython3u--1.0.sql b/contrib/hstore_plpython/hstore_plpython3u--1.0.sql index a964a49..0b410ab 100644 *** a/contrib/hstore_plpython/hstore_plpython3u--1.0.sql --- b/contrib/hstore_plpython/hstore_plpython3u--1.0.sql *************** *** 3,13 **** -- complain if script is sourced in psql, rather than via CREATE EXTENSION \echo Use "CREATE EXTENSION hstore_plpython3u" to load this file. \quit - -- make sure the prerequisite libraries are loaded - LOAD 'plpython3'; - SELECT NULL::hstore; - - CREATE FUNCTION hstore_to_plpython3(val internal) RETURNS internal LANGUAGE C STRICT IMMUTABLE AS 'MODULE_PATHNAME', 'hstore_to_plpython'; --- 3,8 ---- diff --git a/contrib/hstore_plpython/hstore_plpythonu--1.0.sql b/contrib/hstore_plpython/hstore_plpythonu--1.0.sql index d79bdc9..5283291 100644 *** a/contrib/hstore_plpython/hstore_plpythonu--1.0.sql --- b/contrib/hstore_plpython/hstore_plpythonu--1.0.sql *************** *** 3,13 **** -- complain if script is sourced in psql, rather than via CREATE EXTENSION \echo Use "CREATE EXTENSION hstore_plpythonu" to load this file. \quit - -- make sure the prerequisite libraries are loaded - LOAD 'plpython2'; -- change to plpython3 if that ever becomes the default - SELECT NULL::hstore; - - CREATE FUNCTION hstore_to_plpython(val internal) RETURNS internal LANGUAGE C STRICT IMMUTABLE AS 'MODULE_PATHNAME'; --- 3,8 ----
Hi, On 2016-10-03 12:29:18 -0400, Tom Lane wrote: > The patch seems pretty successful in terms of being noninvasive to > the code. I think the major objection to it would be that we no > longer have any direct compiler-verified connection between the > signatures of the called functions in hstore/plpython and what > hstore_plpython thinks they are. That is, if someone were to > modify hstore and change the signature of say hstoreCheckKeyLen, > this would not cause any compiler complaints in hstore_plpython, > just runtime problems. > A slightly ugly way of alleviating that risk would be to put the > function typedefs right beside the externs in the originating > modules, eg in hstore.h > > extern size_t hstoreCheckKeyLen(size_t len); > +typedef size_t (*hstoreCheckKeyLen_t) (size_t len); We could instead add a AssertVariableIsOfType(), besides the library lookup maybe? > If we were to push forward with this idea, the remaining work > would be to fix the other two contrib transform modules similarly, > after which I would want to revert the changes in commit cac765820 > and later that suppressed linker errors for unresolved symbols in > contrib links. The possibility of getting back that error detection > is actually the main motivation for this IMO. That'd be rather neat. Andres
Andres Freund <andres@anarazel.de> writes:
> On 2016-10-03 12:29:18 -0400, Tom Lane wrote:
>> The patch seems pretty successful in terms of being noninvasive to
>> the code. I think the major objection to it would be that we no
>> longer have any direct compiler-verified connection between the
>> signatures of the called functions in hstore/plpython and what
>> hstore_plpython thinks they are.
> We could instead add a AssertVariableIsOfType(), besides the library
> lookup maybe?
Hmm ... had not occurred to me that that might work on a function name.
I'll go try it.
>> If we were to push forward with this idea, the remaining work
>> would be to fix the other two contrib transform modules similarly,
>> after which I would want to revert the changes in commit cac765820
>> and later that suppressed linker errors for unresolved symbols in
>> contrib links. The possibility of getting back that error detection
>> is actually the main motivation for this IMO.
> That'd be rather neat.
I experimented with that aspect a bit today. For macOS it's sufficient
to remove the "-Wl,-undefined,dynamic_lookup" linker flag that cac765820
added. However, ignoring unresolved symbols in shlibs is the default
on Linux, and while you can make it throw errors, that just leads to
errors for all the references into the core backend. Not very helpful.
AFAICS, GNU ld lacks any equivalent to macOS' -bundle_loader switch,
which is what we'd need to make this usable.
A workable compromise for Linux might be to enable -Wl,-z,now which
changes unresolved symbol resolution from lazy to on-load. That would
at least guarantee that any testing whatsoever would detect incorrect
references, even if the bad call wasn't exercised.
No idea about what to do to change this on Windows. If we had error
detection on Linux+Windows+macOS that would be good enough for me ---
anyone who feels like fixing it for minor platforms is welcome to,
but it would be unlikely that we'd detect any new problems.
regards, tom lane
On 2016-10-03 14:49:20 -0400, Tom Lane wrote: > > On 2016-10-03 12:29:18 -0400, Tom Lane wrote: > >> The patch seems pretty successful in terms of being noninvasive to > >> the code. I think the major objection to it would be that we no > >> longer have any direct compiler-verified connection between the > >> signatures of the called functions in hstore/plpython and what > >> hstore_plpython thinks they are. > > > We could instead add a AssertVariableIsOfType(), besides the library > > lookup maybe? > > Hmm ... had not occurred to me that that might work on a function name. > I'll go try it. I'm pretty sure it does, I've used it for that in the past. > Andres Freund <andres@anarazel.de> writes: > >> If we were to push forward with this idea, the remaining work > >> would be to fix the other two contrib transform modules similarly, > >> after which I would want to revert the changes in commit cac765820 > >> and later that suppressed linker errors for unresolved symbols in > >> contrib links. The possibility of getting back that error detection > >> is actually the main motivation for this IMO. > > > That'd be rather neat. > > I experimented with that aspect a bit today. For macOS it's sufficient > to remove the "-Wl,-undefined,dynamic_lookup" linker flag that cac765820 > added. However, ignoring unresolved symbols in shlibs is the default > on Linux, and while you can make it throw errors, that just leads to > errors for all the references into the core backend. Not very helpful. > AFAICS, GNU ld lacks any equivalent to macOS' -bundle_loader switch, > which is what we'd need to make this usable. Hm. I wonder if it's actually possible to link against the main backend, when compiling as a position-independent-executable... > A workable compromise for Linux might be to enable -Wl,-z,now which > changes unresolved symbol resolution from lazy to on-load. That would > at least guarantee that any testing whatsoever would detect incorrect > references, even if the bad call wasn't exercised. Hm, I think that's the default already no? At least linux has #define pg_dlopen(f) dlopen((f), RTLD_NOW | RTLD_GLOBAL) which should trigger that behaviour, and I do remember seing errors because of non-exercised function and variable references. Regards, Andres
Andres Freund <andres@anarazel.de> writes:
> On 2016-10-03 14:49:20 -0400, Tom Lane wrote:
>> ... ignoring unresolved symbols in shlibs is the default
>> on Linux, and while you can make it throw errors, that just leads to
>> errors for all the references into the core backend. Not very helpful.
>> AFAICS, GNU ld lacks any equivalent to macOS' -bundle_loader switch,
>> which is what we'd need to make this usable.
> Hm. I wonder if it's actually possible to link against the main backend,
> when compiling as a position-independent-executable...
I tried that, didn't work.
>> A workable compromise for Linux might be to enable -Wl,-z,now which
>> changes unresolved symbol resolution from lazy to on-load.
> Hm, I think that's the default already no?
Oh, maybe, I was looking for compile switches. Will test.
regards, tom lane
On 2016-10-03 15:40:12 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2016-10-03 14:49:20 -0400, Tom Lane wrote: > >> ... ignoring unresolved symbols in shlibs is the default > >> on Linux, and while you can make it throw errors, that just leads to > >> errors for all the references into the core backend. Not very helpful. > >> AFAICS, GNU ld lacks any equivalent to macOS' -bundle_loader switch, > >> which is what we'd need to make this usable. > > > Hm. I wonder if it's actually possible to link against the main backend, > > when compiling as a position-independent-executable... > > I tried that, didn't work. Too bad :(. The only way I could see checking this properly would be to LD_PRELOAD the .so against postgres, at build time. That should then error out if there's undefined symbols; without the issue that _PG_init might error out for some reason. Regards, Andres
On Mon, Oct 03, 2016 at 12:29:18PM -0400, Tom Lane wrote: > Pursuant to Andres' suggestion in > https://www.postgresql.org/message-id/20161002223927.57xns3arkdg4hu6x@alap3.anarazel.de > attached is a draft patch that gets rid of link-time references > from hstore_plpython to both hstore and plpython. I've verified > that this allows "LOAD 'hstore_plpython'" to succeed in a fresh > session without having loaded the prerequisite modules first. I like how that turned out. However, ... > *** a/contrib/hstore_plpython/Makefile > --- b/contrib/hstore_plpython/Makefile > --- 23,40 ---- > include $(top_srcdir)/contrib/contrib-global.mk > endif > > ! # We must link libpython explicitly > ifeq ($(PORTNAME), aix) > rpathdir = $(pkglibdir):$(python_libdir) ... adding $(pkglibdir) to rpath is obsolete, now that this ceased to link to hstore explicitly. > ! SHLIB_LINK += $(python_libspec) $(python_additional_libs) > ! else > ifeq ($(PORTNAME), win32) > ! # ... see silliness in plpython Makefile ... > ! SHLIB_LINK += $(sort $(wildcard ../../src/pl/plpython/libpython*.a)) > ! else > ! rpathdir = $(python_libdir) > ! SHLIB_LINK += $(python_libspec) For consistency with longstanding src/pl/plpython practice, $(python_libspec) should always have an accompanying $(python_additional_libs). This matters on few configurations. I propose to clean up both points as attached. Tested on AIX, which ceases to be a special case. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Noah Misch <noah@leadboat.com> writes:
> ... adding $(pkglibdir) to rpath is obsolete, now that this ceased to link to
> hstore explicitly.
OK...
> For consistency with longstanding src/pl/plpython practice, $(python_libspec)
> should always have an accompanying $(python_additional_libs). This matters on
> few configurations.
Good point.
> I propose to clean up both points as attached. Tested on AIX, which ceases to
> be a special case.
Looks reasonable to the naked eye. I have not tested it, but presumably
if there is any problem the buildfarm would soon tell us.
regards, tom lane