Re: RTLD_GLOBAL (& JIT inlining)

[Kyotaro HORIGUCHI]

At Thu, 24 Jan 2019 01:02:14 -0500, Tom Lane  wrote in 
<24744.1548309...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI  writes:
> > With the attached patch, external modules are loaded RTLD_LOCAL
> > by default. PG_MODULE_EXPORT_SYMBOL in a module source files let
> > it loaded RTLD_GLOBAL. I suppose this is the change with the
> > least impact on existing modules because I believe most of them
> > don't need to export symbols.
> 
> This seems really hacky :-(
> 
> In the PL cases, at least, it's not so much the PL itself that
> wants to export symbols as the underlying language library
> (libpython, libperl, etc).  You're assuming that an action on
> the PL .so will propagate to the underlying language .so, which

(If I understand you correctly) I'm not assuming that the
propagation happens or even the contray.  As can be seen from it
actually works, symbols of underlying libraries referred from
pl*.so are resoved using dependency list in pl*.so itself
independently of the RTLD flags given for it. The problem here is
while underlying language libraries cannot ferer symbols back in
the pl*.so when it is loaded with RTLD_LOCAL, RTLD_GLOBAL makes
some extensions unhappy.

I think that the assumption that RTLD_GLOBAL resolves all was
proved rather shaky. RTLD_DEEPBIND is not available on some
platforms so we need to choose between LOCAL and GLOBAL per
loading module.

> seems like a pretty shaky assumption.  I wonder also whether
> closing and reopening the DL handle will really do anything
> at all for already-loaded libraries ...

internal_load_library() avoids duplicate loading so I believe it
cannot happen.  Regarding duplicate dlopen, RTLD_GLOBAL just
promotes RTLD_LOCAL. In other words, the former wins. So it's not
a problem if the dlclose() there did nothing.

We could RTLD_NOLOAD for almost all platforms but win32 implement
needs additinal change to make work it as expected.

If you are saying hacky about that it uses only the symbol of
unused function, we can actually call it to get the flag.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: RTLD_GLOBAL (& JIT inlining)

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> With the attached patch, external modules are loaded RTLD_LOCAL
> by default. PG_MODULE_EXPORT_SYMBOL in a module source files let
> it loaded RTLD_GLOBAL. I suppose this is the change with the
> least impact on existing modules because I believe most of them
> don't need to export symbols.

This seems really hacky :-(

In the PL cases, at least, it's not so much the PL itself that
wants to export symbols as the underlying language library
(libpython, libperl, etc).  You're assuming that an action on
the PL .so will propagate to the underlying language .so, which
seems like a pretty shaky assumption.  I wonder also whether
closing and reopening the DL handle will really do anything
at all for already-loaded libraries ...

regards, tom lane

Re: RTLD_GLOBAL (& JIT inlining)

[Kyotaro HORIGUCHI]

Hello.

At Mon, 26 Feb 2018 23:50:41 +0200, Ants Aasma  wrote in 

> On Mon, Feb 26, 2018 at 11:28 PM, Andres Freund  wrote:
> > So RTLD_LOCAL is out of the question, but I think we can get a good bit
> > of the benefit by either specifying -Wl,-Bsymbolic at shlib build time,
> > or RTLD_DEEPBIND at dlopen() time.  Either leads to the opened shared
> > library effectively being put at the beginning of the search path,
> > therefore avoiding the issue that an earlier loaded shared library or
> > symbols from the main binary can accidentally overwrite things in the
> > shared library itself. Which incidentally also makes loading a bit
> > faster.
> 
> I think this would also fix oracle_fdw crashing when postgres is
> compiled with --with-ldap. At least RTLD_DEEPBIND helped. [1]
> 
> [1] 
> https://www.postgresql.org/message-id/CA%2BCSw_tPDYgnzCYW0S4oU0mTUoUhZ9pc7MRBPXVD-3Zbiwni9w%40mail.gmail.com

I saw several cases of the misbinding specifically among
extensions using different versions of the same library or a part
of which was just transplanted from another extension. Now I'm
seeing a case nearby again and found this thread.

Some pl-libraries (plpython does but plperl doesn't for me)
mandates to export their symbols for external modules. So
RTLD_GLOBAL is needed for such extensions but not needed in most
cases. We need a means to instruct whether a module wants to
expose symbols or not.

The extension control file doesn't seem the place. The most
appropriate way seems to be another magic data.

With the attached patch, external modules are loaded RTLD_LOCAL
by default. PG_MODULE_EXPORT_SYMBOL in a module source files let
it loaded RTLD_GLOBAL. I suppose this is the change with the
least impact on existing modules because I believe most of them
don't need to export symbols.

I think this works for all platforms that have dlopen or
shl_load. Windows doesn't the equivalent but anyway we should
explicitly declare using dllexport.

Any opinions or thoughts?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 114692655060bf74d01e0f5452e89f3bd332dfa1 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 24 Jan 2019 13:35:19 +0900
Subject: [PATCH] Add control on wheter symbols in external module is exported

We load exteral modlues with RTLD_GLOBAL, which is problematic when
two or more modules share the same symbol. On the other hand some
modules mandates to export its symbols. This patch enables modules
to decide whether to export them or not. They is defaultly hidden.

plpython module is modified along with as it needs to export symbols.
---
 doc/src/sgml/xfunc.sgml|  8 
 src/backend/utils/fmgr/dfmgr.c | 13 -
 src/include/fmgr.h |  8 
 src/pl/plpython/plpy_main.c|  1 +
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index e18272c33a..9a65e76eda 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -1875,6 +1875,14 @@ PG_MODULE_MAGIC;
 (Presently, unloads are disabled and will never occur, but this may
 change in the future.)

+   
+ The dynamic loader loads a file as its symbols are hidden from external
+ modules by default. A file is loaded exporting the symbols by writhing
+ this in one of the module source files.
+
+PG_MODULE_EXPORT_SYMBOL;
+
+   
 
   
 
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 456297a531..98ac1cc438 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -236,7 +236,7 @@ internal_load_library(const char *libname)
 #endif
 		file_scanner->next = NULL;
 
-		file_scanner->handle = dlopen(file_scanner->filename, RTLD_NOW | RTLD_GLOBAL);
+		file_scanner->handle = dlopen(file_scanner->filename, RTLD_NOW | RTLD_LOCAL);
 		if (file_scanner->handle == NULL)
 		{
 			load_error = dlerror();
@@ -281,6 +281,17 @@ internal_load_library(const char *libname)
 	 errhint("Extension libraries are required to use the PG_MODULE_MAGIC macro.")));
 		}
 
+		/* Check if the module wants to export symbols */
+		if (dlsym(file_scanner->handle, PG_MODULE_EXPORT_SYMBOL_NAME_STRING))
+		{
+			elog(DEBUG3,
+ "loaded dynamic-link library \"%s\" with RTLD_GLOBAL", libname);
+			/* want to export, reload it the way */
+			dlclose(file_scanner->handle);
+			file_scanner->handle = dlopen(file_scanner->filename,
+		  RTLD_NOW | RTLD_GLOBAL);
+		}
+
 		/*
 		 * If the library has a _PG_init() function, call it.
 		 */
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index ead17f0e44..c072850646 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -456,6 +456,14 @@ PG_MAGIC_FUNCTION_NAME(void) \
 } \
 extern int no_such_variable
 
+#define PG_MODULE_EXPORT_SYMBOL_NAME Pg_module_exrpot_symbols
+#define PG_MODULE_EXPORT_SYMBOL_NAME_STRING "Pg_module_exrpot_symbols"
+
+#define PG_MODULE_EXPORT_SYMBOL	\
+extern PGDLLEXPORT void 

Re: RTLD_GLOBAL (& JIT inlining)

[Ants Aasma]

On Mon, Feb 26, 2018 at 11:28 PM, Andres Freund  wrote:
> So RTLD_LOCAL is out of the question, but I think we can get a good bit
> of the benefit by either specifying -Wl,-Bsymbolic at shlib build time,
> or RTLD_DEEPBIND at dlopen() time.  Either leads to the opened shared
> library effectively being put at the beginning of the search path,
> therefore avoiding the issue that an earlier loaded shared library or
> symbols from the main binary can accidentally overwrite things in the
> shared library itself. Which incidentally also makes loading a bit
> faster.

I think this would also fix oracle_fdw crashing when postgres is
compiled with --with-ldap. At least RTLD_DEEPBIND helped. [1]

[1] 
https://www.postgresql.org/message-id/CA%2BCSw_tPDYgnzCYW0S4oU0mTUoUhZ9pc7MRBPXVD-3Zbiwni9w%40mail.gmail.com

Ants Aasma

Re: RTLD_GLOBAL (& JIT inlining)

[Andres Freund]

On 2018-02-23 11:05:23 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > I think using RTLD_LOCAL on most machines would be a much better
> > idea. I've not found proper explanations why GLOBAL is used. We started
> > using it ages ago, with [2], but that commit contains no explanation,
> > and a quick search didn't show up anything either. Peter?
> 
> https://www.postgresql.org/message-id/7142.122...@sss.pgh.pa.us
> 
> My position is the same as then: I'm happy to remove it if it doesn't
> break things anywhere ... but it seems like it would cause problems for
> plpython, unless their behavior has changed since 2001 which is
> surely possible.

It hasn't, and it's not just plpython, at least plperl is affected as
well. I'd guess most libraries that have their own dynamic loading
support are affected.

So RTLD_LOCAL is out of the question, but I think we can get a good bit
of the benefit by either specifying -Wl,-Bsymbolic at shlib build time,
or RTLD_DEEPBIND at dlopen() time.  Either leads to the opened shared
library effectively being put at the beginning of the search path,
therefore avoiding the issue that an earlier loaded shared library or
symbols from the main binary can accidentally overwrite things in the
shared library itself. Which incidentally also makes loading a bit
faster.

A bit of googling suggests -Bsymbolic is likely to be more portable.

A quick test confirms that under linux our tests pass with it, after
patching Makefile.shlib.

Greetings,

Andres Freund

Re: RTLD_GLOBAL (& JIT inlining)

[Andres Freund]

Hi,

On 2018-02-22 23:11:07 -0800, Andres Freund wrote:
> I think using RTLD_LOCAL on most machines would be a much better
> idea. I've not found proper explanations why GLOBAL is used. We started
> using it ages ago, with [2], but that commit contains no explanation,
> and a quick search didn't show up anything either. Peter?

Hm. No idea if that was the reason back then, but I think it's
unfortunately not easy to change.  The reason why some plperlu tests
fail, is that plperl extension libraries rely on plperl to be loaded
into the global namespace.

When plperl gets loaded with RTLD_LOCAL all the dependant shared
libaries (i.e. libperl.so) also get loaded with it. That works perfectly
for plperl itself, but once per loads an extension library is loaded, it
fails to resolve libperl.so symbols...  I can "fix" this by
dlopen(RTLD_GLOBAL) libperl.so, but that's obviously not a practial
solution.


FWIW, something in plperl's error handling appears to be busted. Instead
of a helpful error report we get:
ERROR:  22021: invalid byte sequence for encoding "UTF8": 0x00
which doesn't help much to nail down the issue.

With a breakpoint it becomes clear what the issue is:

#0  report_invalid_encoding (encoding=6, mbstr=0x557ae880416b "", len=252)
at /home/andres/src/postgresql/src/backend/utils/mb/wchar.c:1997
#1  0x557ae73d4473 in pg_verify_mbstr_len (encoding=6, mbstr=0x557ae880416b 
"", len=252, noError=0 '\000')
at /home/andres/src/postgresql/src/backend/utils/mb/wchar.c:1936
#2  0x557ae73d4354 in pg_verify_mbstr (encoding=6, 
mbstr=0x557ae8804080 "Can't load 
'/usr/lib/x86_64-linux-gnu/perl5/5.26/auto/List/Util/Util.so' for module 
List::Util: /usr/lib/x86_64-linux-gnu/perl5/5.26/auto/List/Util/Util.so: 
undefined symbol: PL_memory_wrap at /usr/share/perl/5.26/XSLoader.pm line 
96.\n", len=487, noError=0 '\000') at 
/home/andres/src/postgresql/src/backend/utils/mb/wchar.c:1879
#3  0x557ae73d119a in pg_any_to_server (
s=0x557ae8804080 "Can't load 
'/usr/lib/x86_64-linux-gnu/perl5/5.26/auto/List/Util/Util.so' for module 
List::Util: /usr/lib/x86_64-linux-gnu/perl5/5.26/auto/List/Util/Util.so: 
undefined symbol: PL_memory_wrap at /usr/share/perl/5.26/XSLoader.pm line 
96.\n", len=487, encoding=6) at 
/home/andres/src/postgresql/src/backend/utils/mb/mbutils.c:572
#4  0x7f4cabb6b123 in utf_u2e (
utf8_str=0x557ae8804080 "Can't load 
'/usr/lib/x86_64-linux-gnu/perl5/5.26/auto/List/Util/Util.so' for module 
List::Util: /usr/lib/x86_64-linux-gnu/perl5/5.26/auto/List/Util/Util.so: 
undefined symbol: PL_memory_wrap at /usr/share/perl/5.26/XSLoader.pm line 
96.\n", len=487) at 
/home/andres/src/postgresql/src/pl/plperl/plperl_helpers.h:16
#5  0x7f4cabb6b2f6 in sv2cstr (sv=0x557ae8769568) at 
/home/andres/src/postgresql/src/pl/plperl/plperl_helpers.h:96
#6  0x7f4cabb732dc in plperl_call_perl_func (desc=0x557ae86cf6b0, 
fcinfo=0x557ae875fc20)
at /home/andres/src/postgresql/src/pl/plperl/plperl.c:2250
#7  0x7f4cabb74a3b in plperl_func_handler (fcinfo=0x557ae875fc20)
at /home/andres/src/postgresql/src/pl/plperl/plperl.c:2438

I don't now perl apis at all, but it's clearly wrong that fram 3,4 show
len=487, which comes from SvPVutf8() in sv2cstr().  It appears that
somehow perl throws out two error messages separated by a null byte...

(gdb) p *(char[487]*)utf8_str
$42 = "Can't load '/usr/lib/x86_64-linux-gnu/perl5/5.26/auto/List/Util/Util.so' 
for module List::Util: 
/usr/lib/x86_64-linux-gnu/perl5/5.26/auto/List/Util/Util.so: undefined symbol: 
PL_memory_wrap at /usr/share/perl/5.26/XSLoader.pm line 96.\n\000 at 
/usr/lib/x86_64-linux-gnu/perl5/5.26/List/Util.pm line 23.\nCompilation failed 
in require at /usr/lib/x86_64-linux-gnu/perl5/5.26/Scalar/Util.pm line 
23.\nCompilation failed in require at 
/usr/lib/x86_64-linux-gnu/perl/5.26/Data/Dumper.pm line 303.\n"


Greetings,

Andres Freund

RTLD_GLOBAL (& JIT inlining)

[Andres Freund]

Hi Peter, All,


First question:

Why do we currently use RTLD_GLOBAL loading extension libraries, but
take pains ([1]) to make things work without RTLD_GLOBAL. It seems like
it'd be both safer to RTLD_LOCAL on platforms supporting it (or
equivalent), as well as less error-prone because we'd be less likely to
depend on it like we did e.g. during transforms development.


It seems error prone because on systmes I'm aware of (IOW linux ;))
conflicting symbols will not cause errors. Neither when there's a
conflict between a .so and the main binary, nor between different
shlibs.  My reading of glibc's ld is that the search order for
unresolved references in a .so is main binary, and then other
RTLD_GLOBAL shared libraries in order of time they're loaded.

Right now it appears that e.g. hstore_plperl has some issue, building
with RTLD_LOCAL makes its test fail. Which means it'll probably not work
on some platforms.


I think using RTLD_LOCAL on most machines would be a much better
idea. I've not found proper explanations why GLOBAL is used. We started
using it ages ago, with [2], but that commit contains no explanation,
and a quick search didn't show up anything either. Peter?


Secondly:

For JITing internal & C operators, and other functions referenced in
JITed code, can be inlined if the definition is available in a suitable
form. The details how that works don't matter much here.


For my jitting work I've so far treated symbols of binaries and all
referenced extensions as being part of a single namespace. Currently the
order in which symbols with multiple definitions were looked up was in
essentially in filesystem order.

While I'm not aware of any observable problems, reviewing that decision
I think that's a terrible idea. First and foremost it seems obviously
wrong to not load symbols from the main postgres binary first. But
leaving that aside, I'm uncomfortable doing inlining between shlibs for
JITing, because that can lead to symbols beeing reloaded differently
between non JITed code (where all external libraries loaded in the
current library are searched in load/usage order) and JITed code (where
the symbols would be resolved according to some static order).

I'm about to rewrite it so that functions are inlined just from the main
binary, or the basename of the shared library explicitly referenced in
the function definition.  That's still different, but at least easy to
understand.  But that still means there's inconsistent order between
different methods of execution, which isn't great.

Comments?

Greetings,

Andres Freund

[1] http://archives.postgresql.org/message-id/2652.1475512158%40sss.pgh.pa.us
[2] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c87bc779d4e9f109e92f8b8f1dfad5d6739f8e97