pgsql: Default to hidden visibility for extension libraries where possi

Hi,

On 2022-07-18 01:05:56 +0000, Andres Freund wrote:

Default to hidden visibility for extension libraries where possible

Looking at the odd failures, not sure what went wrong.

Greetings,

Andres Freund

On 2022-07-17 18:39:35 -0700, Andres Freund wrote:

On 2022-07-18 01:05:56 +0000, Andres Freund wrote:

Default to hidden visibility for extension libraries where possible

Looking at the odd failures, not sure what went wrong.

I don't know how the configure exec bit got removed, shell history doesn't
show anything odd in that regard. I do see a mistake in locally merging the
symbol-visibility branch (leading to a crucial commit being missed). I do see
in shell history that I ran check-world without a failure just before pushing,
which should have shown the failure, but didn't.

Definitely a brown paper bag moment.

Andres Freund <andres@anarazel.de> writes:

I don't know how the configure exec bit got removed, shell history doesn't
show anything odd in that regard. I do see a mistake in locally merging the
symbol-visibility branch (leading to a crucial commit being missed). I do see
in shell history that I ran check-world without a failure just before pushing,
which should have shown the failure, but didn't.

check-world would not have re-run configure even if it was out of date
(only config.status), so that part of it's not so surprising.

Definitely a brown paper bag moment.

We've all been there :-)

regards, tom lane

On Sun, Jul 17, 2022 at 07:01:55PM -0700, Andres Freund wrote:

On 2022-07-17 18:39:35 -0700, Andres Freund wrote:

On 2022-07-18 01:05:56 +0000, Andres Freund wrote:

Default to hidden visibility for extension libraries where possible

Looking at the odd failures, not sure what went wrong.

I don't know how the configure exec bit got removed, shell history doesn't

I think git reflog -p would help to answer that, but I suppose you already
looked.

--
Justin

Hi

po 18. 7. 2022 v 3:06 odesílatel Andres Freund <andres@anarazel.de> napsal:

Default to hidden visibility for extension libraries where possible

Until now postgres built extension libraries with global visibility, i.e.
exporting all symbols. On the one platform where that behavior is not
natively available, namely windows, we emulate it by analyzing the input
files
to the shared library and exporting all the symbols therein.

Not exporting all symbols is actually desirable, as it can improve loading
speed, reduces the likelihood of symbol conflicts and can improve intra
extension library function call performance. It also makes the non-windows
builds more similar to windows builds.

Additionally, with meson implementing the export-all-symbols behavior for
windows, turns out to be more verbose than desirable.

This patch adds support for hiding symbols by default and, to counteract
that,
explicit symbol visibility annotation for compilers that support
__attribute__((visibility("default"))) and -fvisibility=hidden. That is
expected to be most, if not all, compilers except msvc (for which we
already
support explicit symbol export annotations).

Now that extension library symbols are explicitly exported, we don't need
to
export all symbols on windows anymore, hence remove that behavior from
src/tools/msvc. The supporting code can't be removed, as we still need to
export all symbols from the main postgres binary.

Author: Andres Freund <andres@anarazel.de>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion:
/messages/by-id/20211101020311.av6hphdl6xbjbuif@alap3.anarazel.de

Unfortunately, this commit definitely breaks plpgsql_check. Can the
following routines be visible?

AssertVariableIsOfType(&plpgsql_build_datatype,
plpgsql_check__build_datatype_t);
plpgsql_check__build_datatype_p = (plpgsql_check__build_datatype_t)
LOAD_EXTERNAL_FUNCTION("$libdir/plpgsql", "plpgsql_build_datatype");

AssertVariableIsOfType(&plpgsql_compile, plpgsql_check__compile_t);
plpgsql_check__compile_p = (plpgsql_check__compile_t)
LOAD_EXTERNAL_FUNCTION("$libdir/plpgsql", "plpgsql_compile");

AssertVariableIsOfType(&plpgsql_parser_setup,
plpgsql_check__parser_setup_t);
plpgsql_check__parser_setup_p = (plpgsql_check__parser_setup_t)
LOAD_EXTERNAL_FUNCTION("$libdir/plpgsql", "plpgsql_parser_setup");

AssertVariableIsOfType(&plpgsql_stmt_typename,
plpgsql_check__stmt_typename_t);
plpgsql_check__stmt_typename_p = (plpgsql_check__stmt_typename_t)
LOAD_EXTERNAL_FUNCTION("$libdir/plpgsql", "plpgsql_stmt_typename");

AssertVariableIsOfType(&plpgsql_exec_get_datum_type,
plpgsql_check__exec_get_datum_type_t);
plpgsql_check__exec_get_datum_type_p =
(plpgsql_check__exec_get_datum_type_t)
LOAD_EXTERNAL_FUNCTION("$libdir/plpgsql",
"plpgsql_exec_get_datum_type");

AssertVariableIsOfType(&plpgsql_recognize_err_condition,
plpgsql_check__recognize_err_condition_t);
plpgsql_check__recognize_err_condition_p =
(plpgsql_check__recognize_err_condition_t)
LOAD_EXTERNAL_FUNCTION("$libdir/plpgsql",
"plpgsql_recognize_err_condition");

AssertVariableIsOfType(&plpgsql_ns_lookup, plpgsql_check__ns_lookup_t);
plpgsql_check__ns_lookup_p = (plpgsql_check__ns_lookup_t)
LOAD_EXTERNAL_FUNCTION("$libdir/plpgsql", "plpgsql_ns_lookup");

Regards

Pavel

On 2022-Jul-19, Pavel Stehule wrote:

po 18. 7. 2022 v 3:06 odesílatel Andres Freund <andres@anarazel.de> napsal:

Default to hidden visibility for extension libraries where possible

Unfortunately, this commit definitely breaks plpgsql_check. Can the
following routines be visible?

Do you just need to send a patch to add an exports.txt file to
src/pl/plpgsql/src/ for these functions?

plpgsql_build_datatype
plpgsql_compile
plpgsql_parser_setup
plpgsql_stmt_typename
plpgsql_exec_get_datum_type
plpgsql_recognize_err_condition
plpgsql_ns_lookup

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2022-Jul-19, Pavel Stehule wrote:

Unfortunately, this commit definitely breaks plpgsql_check. Can the
following routines be visible?

Do you just need to send a patch to add an exports.txt file to
src/pl/plpgsql/src/ for these functions?

The precedent of plpython says that PGDLLEXPORT markers are sufficient.
But yeah, we need a list of exactly which functions need to be
re-exposed. I imagine pldebugger has its own needs.

regards, tom lane

[ Redirecting thread to -hackers from -committers ]

On 2022-Jul-19, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Do you just need to send a patch to add an exports.txt file to
src/pl/plpgsql/src/ for these functions?

The precedent of plpython says that PGDLLEXPORT markers are sufficient.
But yeah, we need a list of exactly which functions need to be
re-exposed. I imagine pldebugger has its own needs.

A reasonable guess. I went as far as downloading pldebugger and
compiling it, but it doesn't have a test suite of its own, so I couldn't
verify anything about it. I did notice that plpgsql_check is calling
function load_external_function(), and that doesn't appear in
pldebugger. I wonder if the find_rendezvous_variable business is at
play.

Anyway, the minimal patch that makes plpgsql_check tests pass is
attached. This seems a bit random. Maybe it'd be better to have a
plpgsql_internal.h with functions that are exported only for plpgsql
itself, and keep plpgsql.h with a set of functions, all marked
PGDLLEXPORT, that are for external use.

... oh, and:

$ postmaster -c shared_preload_libraries=plugin_debugger
2022-07-19 16:27:24.006 CEST [742142] FATAL: cannot request additional shared memory outside shmem_request_hook

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

On Tue, Jul 19, 2022 at 04:28:07PM +0200, Alvaro Herrera wrote:

... oh, and:

$ postmaster -c shared_preload_libraries=plugin_debugger
2022-07-19 16:27:24.006 CEST [742142] FATAL: cannot request additional shared memory outside shmem_request_hook

This has been reported multiple times (including on one of my own projects),
see
/messages/by-id/81f82c00-8818-91f3-96fa-47976f94662b@pm.me
for the last report.

Hi

út 19. 7. 2022 v 16:28 odesílatel Alvaro Herrera <alvherre@alvh.no-ip.org>
napsal:

[ Redirecting thread to -hackers from -committers ]

On 2022-Jul-19, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Do you just need to send a patch to add an exports.txt file to
src/pl/plpgsql/src/ for these functions?

The precedent of plpython says that PGDLLEXPORT markers are sufficient.
But yeah, we need a list of exactly which functions need to be
re-exposed. I imagine pldebugger has its own needs.

A reasonable guess. I went as far as downloading pldebugger and
compiling it, but it doesn't have a test suite of its own, so I couldn't
verify anything about it. I did notice that plpgsql_check is calling
function load_external_function(), and that doesn't appear in
pldebugger. I wonder if the find_rendezvous_variable business is at
play.

Anyway, the minimal patch that makes plpgsql_check tests pass is
attached. This seems a bit random. Maybe it'd be better to have a
plpgsql_internal.h with functions that are exported only for plpgsql
itself, and keep plpgsql.h with a set of functions, all marked
PGDLLEXPORT, that are for external use.

I can confirm that the attached patch fixes plpgsql_check.

Thank you

Pavel

Hi,

On 2022-07-19 16:28:07 +0200, Alvaro Herrera wrote:

Anyway, the minimal patch that makes plpgsql_check tests pass is
attached. This seems a bit random. Maybe it'd be better to have a
plpgsql_internal.h with functions that are exported only for plpgsql
itself, and keep plpgsql.h with a set of functions, all marked
PGDLLEXPORT, that are for external use.

It does seem a bit random. But I think we probably should err on the side of
adding more declarations, rather than the oposite.

I like the plpgsql_internal.h idea, but probably done separately?

Greetings,

Andres Freund

Hi

út 19. 7. 2022 v 17:31 odesílatel Andres Freund <andres@anarazel.de> napsal:

Hi,

On 2022-07-19 16:28:07 +0200, Alvaro Herrera wrote:

Anyway, the minimal patch that makes plpgsql_check tests pass is
attached. This seems a bit random. Maybe it'd be better to have a
plpgsql_internal.h with functions that are exported only for plpgsql
itself, and keep plpgsql.h with a set of functions, all marked
PGDLLEXPORT, that are for external use.

It does seem a bit random. But I think we probably should err on the side
of
adding more declarations, rather than the oposite.

This list can be extended. I think plpgsql_check is maybe one extension
that uses code from another extension directly. This is really not common
usage.

I like the plpgsql_internal.h idea, but probably done separately?

can be

I have not any problem with it or with exports.txt file.

Hi,

On 2022-07-19 17:37:11 +0200, Pavel Stehule wrote:

�t 19. 7. 2022 v 17:31 odes�latel Andres Freund <andres@anarazel.de> napsal:

Hi,

On 2022-07-19 16:28:07 +0200, Alvaro Herrera wrote:

Anyway, the minimal patch that makes plpgsql_check tests pass is
attached. This seems a bit random. Maybe it'd be better to have a
plpgsql_internal.h with functions that are exported only for plpgsql
itself, and keep plpgsql.h with a set of functions, all marked
PGDLLEXPORT, that are for external use.

It does seem a bit random. But I think we probably should err on the side
of
adding more declarations, rather than the oposite.

This list can be extended. I think plpgsql_check is maybe one extension
that uses code from another extension directly. This is really not common
usage.

There's a few more use cases, e.g. transform modules. Hence exposing e.g. many
plpython helpers.

I have not any problem with it or with exports.txt file.

Just to be clear, there shouldn't be any use exports.txt here, just a few
PGDLLEXPORTs.

Greetings,

Andres Freund

Hi,

On 2022-07-19 16:28:07 +0200, Alvaro Herrera wrote:

Anyway, the minimal patch that makes plpgsql_check tests pass is
attached.

Do you want to commit that or should I?

Greetings,

Andres Freund

Hi,

On 2022-07-19 08:31:49 -0700, Andres Freund wrote:

But I think we probably should err on the side of adding more
declarations, rather than the oposite.

To see if there's other declarations that should be added, I used
https://codesearch.debian.net/search?q=load_external_function&amp;literal=1&amp;perpkg=1

which shows plpgsql_check and hstore_pllua. All the hstore symbols for
the latter are exported already.

Greetings,

Andres Freund

Hello

On 2022-Jul-19, Andres Freund wrote:

On 2022-07-19 16:28:07 +0200, Alvaro Herrera wrote:

Anyway, the minimal patch that makes plpgsql_check tests pass is
attached.

Do you want to commit that or should I?

Done.

No immediate plans for splitting plpgsql.h, so if anyone wants to take a
stab at that, be my guest.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Aprender sin pensar es inútil; pensar sin aprender, peligroso" (Confucio)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

No immediate plans for splitting plpgsql.h, so if anyone wants to take a
stab at that, be my guest.

ISTM that a comment pointing out that the functions marked PGDLLEXPORT
are meant to be externally accessible should be sufficient.

I'll try to do some research later today to identify anything else
we need to mark in plpgsql. I recall doing some work specifically
creating functions for pldebugger's use, but I'll need to dig.

regards, tom lane

On Wed, Jul 20, 2022 at 9:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

ISTM that a comment pointing out that the functions marked PGDLLEXPORT
are meant to be externally accessible should be sufficient.

The name PGDLLEXPORT is actually slightly misleading, now, because
there's no longer anything about it that is specific to DLLs.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2022-Jul-20, Tom Lane wrote:

I'll try to do some research later today to identify anything else
we need to mark in plpgsql. I recall doing some work specifically
creating functions for pldebugger's use, but I'll need to dig.

I suppose you're probably thinking of commit 53ef6c40f1e7; that didn't
expose functions directly, but through plpgsql_plugin_ptr. Maybe that
one does need to be made PGDLLEXPORT, since currently it isn't.

That was also reported by Pavel. He was concerned about plpgsql_check,
though, not pldebugger.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree. (Don Knuth)