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

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(at)anarazel(dot)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(at)anarazel(dot)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(at)anarazel(dot)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(at)anarazel(dot)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.

Noah Misch <noah(at)leadboat(dot)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