Removing link-time cross-module refs in contrib - Mailing list pgsql-hackers

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 ----