Re: [COMMITTERS] pgsql: Add transforms feature

On 5/3/15 2:27 PM, Tom Lane wrote:
> 1. Preventing undefined-symbol errors at link time might be standard
> according to some platforms, but it is not the universal default, and
> I think there is no very good reason to assume that it is possible
> everywhere. So I'm afraid that prairiedog is just the canary in the coal
> mine, warning us about portability problems we're going to have with
> other non-mainstream platforms not represented in the buildfarm.

I don't think this is an issue, for the following reasons:

Any dlopenable module will have undefined symbols, namely those that it
uses to call back into the executable that loads the module. The only
way it can know to not complain about those symbols is if we tell it
about that executable at build time. Our current shared library build
setup makes reference to the postgres executable only on three
platforms: Windows, OS X, AIX. All other platforms necessarily accept
all undefined symbols at build time. We already know that it can also
be made to work on Windows and OS X. I expect that we might need some
tweaking on AIX, but I'm quite sure it can be made to work there also,
because ...

More generally, the way to build a dlopenable module with GNU libtool is

libtool --mode=link gcc -module -o hello.la foo.lo hello.lo

This works on "all" platforms, for a very large value of "all". There
isn't even an option to make a reference to the loading executable.

As a side note, Perl and Python themselves don't even build without this
option, so even if such platforms existed, they couldn't install Perl or
Python as we know it, and so wouldn't be able to use these features.

> 2. Preventing undefined-symbol errors at link time will hide actual coding
> errors, not only the intended cross-plugin reference cases. We have
> repeatedly seen the buildfarm members that complain about this find actual
> bugs that were missed on laxer platforms. Therefore I think that this
> approach is going to lead to masking bugs that we'll find only by much
> more expensive/painful means than noticing a buildfarm failure.

I appreciate this issue, and I have actually done quite a bit of
research in the hope of being able to provide similar functionality on
other platforms. But on platforms such as Linux, there is no equivalent
option at all. That is, you cannot build a dlopenable module with the
provision to error on all undefined symbols
except those found, say, in the postgres binary. So there is no way to
make all platforms reasonably similar here.

Which leads to a social problem. This is a feature intended for
extension authors. Extension authors will just build their code on
Linux, and if it runs, they ship it. Not everyone (hardly anyone) has
the option of doing platform testing for extensions. So if some
non-mainstream platform is more picky than others, then the inevitable
result is that extensions are more likely to be broken on non-mainstream
platforms. This is already frequently the case for other reasons, with
"non-mainstream" effectively meaning anything other than the author's
own machine in some cases, it appears. We're not going to make this
better by maintaining platform-specific traps.

This issue already exists independent of this feature. If I want to
create an extension that adds some functionality to hstore or hll or
postgis, I'll just call their symbols, it works, I'll ship it. We don't
have any way to prevent this, because many mainstream platforms don't
have the required fine-grained undefined symbol controls.

And the extension authors' path of least resistance when faced with a
build failure report on OS X, say, is to just add an option to the link
command in their extension, it works, it ships.

Moreover, I'm not sure this error checking actually buys us much in
practice. A typoed symbol will be flagged by a compiler warning, and
any undefined symbol will be caught be the test suite as soon as the
module is loaded. So I can't imagine what those buildfarm complaints
are, although I'd be interested look into them the analyze the
circumstances if someone could point me to some concrete cases.

Nonetheless, if there is so much attachment to this behavior, there
might be a simple compromise. We keep the error checking on by default,
but allow a pgxs-level variable setting like SHLIB_ALLOW_UNDEFINED.
This would essentially be equivalent to the
I'll-just-hack-my-extension's-makefile approach mentioned above that
will inevitably happen, but that way we can document it and maintain
some level of control over it.

> Surely there are other ways to deal with this requirement that don't
> require forcing undefined symbols to be treated as valid. For instance,
> our existing find_rendezvous_variable() infrastructure might offer a
> usable solution for passing a function pointer from plugin A to plugin B.
> Indeed, that's pretty much what it was invented for.

The rendezvous variables were invented for a different problem, namely
that you can load modules in arbitrary order, and only the first guy
initializes a variable. That's not the functionality that we need.

Also, any such approach would likely entail casting all functions and
variables through common pointer types, which would lose all kinds of
compiler checking. (All rendezvous variables are void pointers.)

It would put quite a bit of extra burden on the authors of modules such
as hstore or postgis to not only maintain a list of exported functions
but also test those interfaces. Either they'd have to dogfood those
interfaces and replace calls like hstoreCheckKeyLen(...) by
((somecrazycast) myfunctionlookup(hstoreCheckKeyLen))(...), or they'd
have to maintain a parallel set of interfaces that they could only test
by implementing a separate mock user of those interfaces.

At the end of the day, you'll end up implementing a bad version of half
the dynamic linker again in anticipation of a problem we know nothing about.