Re: [HACKERS] [COMMITTERS] pgsql: Add transforms feature
Peter Eisentraut pete...@gmx.net writes: On 5/3/15 2:27 PM, Tom Lane wrote: 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. I agree that it's probably impractical to persuade toolchains to do this if they don't want to. But that doesn't mean that it's a good idea to remove the whole class of error checks on platforms where it *is* possible to make the check. The entire reason that we have a buildfarm is, in essence, to get the union of all checks that are possible on any supported platform. 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. On many platforms, undefined symbols will only be complained of if you actually try to call them, so I think this position places undue faith in the code coverage of our buildfarm tests. (As a counterexample, consider SSL, which I don't think we're exercising at all in the buildfarm because of the difficulty of setup. And we have had bugs of this sort versus OpenSSL, cf commit c9e1ad7fa.) 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. 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. Sure it is. Look at what plpgsql uses it for: to export a PLpgSQL_plugin struct containing function pointers that might be used by other extensions. That seems to me to be exactly the same case as what we're talking about here. 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.) Only with very poor coding style. Properly declared function pointers are pretty type-safe. 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. Well, that actually gets to the core of my point: if a module author exports a struct full of function pointers then he has put a stake in the ground as to exactly what his exported API *is*. With what you've done for transforms, what is the exported API of hstore or plpython or plperl? Conceivably, any non-static function in those libraries is now fair game for somebody else to call. We have this problem already with respect to the core code, and it's nasty. We should not be encouraging the same situation to develop for extensions. I do not understand the second part of your point. Surely, if an extension author expects other extensions to call function X, he's got the same testing problem regardless of how the other extensions would reach X exactly. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [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
Re: [HACKERS] [COMMITTERS] pgsql: Add transforms feature
Peter Eisentraut pete...@gmx.net writes: On 4/26/15 12:36 PM, Tom Lane wrote: I don't know why this patch is fooling around with compile/link flags, but it's broken at least prairiedog The addition of the link flag -undefined dynamic_lookup is so that plugins can refer to symbols from other plugins. This is how other platforms work by default, and it is standard for OS X in GNU libtool and many other projects. I'm confused why prairiedog is complaining about that. We could look into a fix for prairiedog's situation, but TBH I think that that is going in 100% the wrong direction. I'm pretty desperately unhappy with the design choice you've made here, for two reasons: 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. 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. 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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add transforms feature
On Thu, Apr 30, 2015 at 3:30 AM, Andrew Dunstan and...@dunslane.net wrote:
On 04/28/2015 04:10 PM, Andrew Dunstan wrote:
On 04/28/2015 12:03 PM, Andrew Dunstan wrote:
[switching to -hackers]
On 04/28/2015 11:51 AM, Andrew Dunstan wrote:
On 04/28/2015 09:04 AM, Michael Paquier wrote:
On Tue, Apr 28, 2015 at 10:01 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Tue, Apr 28, 2015 at 9:55 AM, Andrew Dunstan wrote:
w.r.t MSVC builds, it looks like we need entries in
$contrib_extraincludes
in src/tools/msvc/Mkvcbuild.pm at the very least.
If our goal is to put back to green the Windows nodes as quick as
possible, we could bypass their build this way , and we'll
additionally need to update the install script and
vcregress.pl/contribcheck to bypass those modules accordingly. Now I
don't think that we should make the things produced inconsistent.
OK, attached are two patches to put back the buildfarm nodes using MSVC
to green
- 0001 adds support for build and installation of the new transform
modules: hstore_plperl, hstore_plpython and ltree_plpython. Note that
this patch is enough to put back the buildfarm to a green state for
MSVC, but it disables the regression tests for those modules,
something addressed in the next patch...
- 0002 adds support for regression tests for the new modules. The
thing is that we need to check the major version version of Python
available in configuration and choose what are the extensions to
preload before the tests run. It is a little bit hacky... But it has
the merit to work, and I am not sure we could have a cleaner solution
by looking at the Makefiles of the transform modules that use
currently conditions based on $(python_majorversion).
Regards,
I have pushed the first of these with some tweaks.
I'm looking at the second.
Regarding this second patch - apart from the buggy python version test
which I just fixed in the first patch, I see this:
+ if ($pyver eq 2)
+ {
+ push @opts, --load-extension=plpythonu;
+ push @opts, '--load-extension=' . $module . 'u';
+ }
But what do we do for Python3?
Do we actually have a Windows machine building with Python3?
The answer seems to be probably not. When I tried enabling this with
bowerbird I got a ton of errors like these:
plpy_cursorobject.obj : error LNK2001: unresolved external symbol
PyObject_SelfIter [C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj]
plpy_cursorobject.obj : error LNK2019: unresolved external symbol
__imp_PyType_Ready referenced in function PLy_cursor_init_type
[C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj]
Something else to fix I guess.
OK, I fixed this as Michael suggested by installing the 64 bit python3 (it's
a pity that python.org didn't offer that to me as a download on their front
page - that's a bit ugly).
However, when it came to running these tests that was a miserable failure.
And indeed, we don't run any regression tests for plpython3 on MSVC.
I arrived to the same conclusion. Perhaps we shall write a TODO in the
code or on the wiki to not forget about that..
So I committed this with just python2 tests enabled. All the buildfarm MSVC
hosts seem to be using python2 anyway.
OK, thanks.
Meanwhile, we have some work to do on the mingw/gcc side.
These changes help make some progress - they let compilation succeed for
hstore_plperl but I still get linkage failures. I suspect we need some
things marked for export that we haven't to be marked needed before.
[...]
I'll try to have a look at that a bit if possible...
Regards,
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add transforms feature
On 04/28/2015 12:03 PM, Andrew Dunstan wrote:
[switching to -hackers]
On 04/28/2015 11:51 AM, Andrew Dunstan wrote:
On 04/28/2015 09:04 AM, Michael Paquier wrote:
On Tue, Apr 28, 2015 at 10:01 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Tue, Apr 28, 2015 at 9:55 AM, Andrew Dunstan wrote:
w.r.t MSVC builds, it looks like we need entries in
$contrib_extraincludes
in src/tools/msvc/Mkvcbuild.pm at the very least.
If our goal is to put back to green the Windows nodes as quick as
possible, we could bypass their build this way , and we'll
additionally need to update the install script and
vcregress.pl/contribcheck to bypass those modules accordingly. Now I
don't think that we should make the things produced inconsistent.
OK, attached are two patches to put back the buildfarm nodes using
MSVC to green
- 0001 adds support for build and installation of the new transform
modules: hstore_plperl, hstore_plpython and ltree_plpython. Note that
this patch is enough to put back the buildfarm to a green state for
MSVC, but it disables the regression tests for those modules,
something addressed in the next patch...
- 0002 adds support for regression tests for the new modules. The
thing is that we need to check the major version version of Python
available in configuration and choose what are the extensions to
preload before the tests run. It is a little bit hacky... But it has
the merit to work, and I am not sure we could have a cleaner solution
by looking at the Makefiles of the transform modules that use
currently conditions based on $(python_majorversion).
Regards,
I have pushed the first of these with some tweaks.
I'm looking at the second.
Regarding this second patch - apart from the buggy python version test
which I just fixed in the first patch, I see this:
+ if ($pyver eq 2)
+ {
+ push @opts, --load-extension=plpythonu;
+ push @opts, '--load-extension=' . $module . 'u';
+ }
But what do we do for Python3?
Do we actually have a Windows machine building with Python3?
The answer seems to be probably not. When I tried enabling this with
bowerbird I got a ton of errors like these:
plpy_cursorobject.obj : error LNK2001: unresolved external symbol
PyObject_SelfIter [C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj]
plpy_cursorobject.obj : error LNK2019: unresolved external symbol
__imp_PyType_Ready referenced in function PLy_cursor_init_type
[C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj]
Something else to fix I guess.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add transforms feature
On 4/28/15 4:10 PM, Andrew Dunstan wrote: Do we actually have a Windows machine building with Python3? The answer seems to be probably not. When I tried enabling this with bowerbird I got a ton of errors like these: plpy_cursorobject.obj : error LNK2001: unresolved external symbol PyObject_SelfIter [C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj] plpy_cursorobject.obj : error LNK2019: unresolved external symbol __imp_PyType_Ready referenced in function PLy_cursor_init_type [C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj] Something else to fix I guess. I think at least at one point the community Windows binaries built by EnterpriseDB were built against Python 3 (which upset some users). But they might not be using the msvc toolchain. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add transforms feature
On Wed, Apr 29, 2015 at 5:15 AM, Peter Eisentraut pete...@gmx.net wrote: On 4/28/15 4:10 PM, Andrew Dunstan wrote: Do we actually have a Windows machine building with Python3? The answer seems to be probably not. When I tried enabling this with bowerbird I got a ton of errors like these: plpy_cursorobject.obj : error LNK2001: unresolved external symbol PyObject_SelfIter [C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj] plpy_cursorobject.obj : error LNK2019: unresolved external symbol __imp_PyType_Ready referenced in function PLy_cursor_init_type [C:\prog\bf\root\HEAD\pgsql\plpython3.vcxproj] Something else to fix I guess. I think at least at one point the community Windows binaries built by EnterpriseDB were built against Python 3 (which upset some users). But they might not be using the msvc toolchain. Er, aren't you simply trying to link with 32b libraries while building in a 64b environment? I am able to make the build work with python 3. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add transforms feature
Oops, thought I'd changed the address line.
[switching to -hackers]
On 04/28/2015 11:51 AM, Andrew Dunstan wrote:
On 04/28/2015 09:04 AM, Michael Paquier wrote:
On Tue, Apr 28, 2015 at 10:01 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Tue, Apr 28, 2015 at 9:55 AM, Andrew Dunstan wrote:
w.r.t MSVC builds, it looks like we need entries in
$contrib_extraincludes
in src/tools/msvc/Mkvcbuild.pm at the very least.
If our goal is to put back to green the Windows nodes as quick as
possible, we could bypass their build this way , and we'll
additionally need to update the install script and
vcregress.pl/contribcheck to bypass those modules accordingly. Now I
don't think that we should make the things produced inconsistent.
OK, attached are two patches to put back the buildfarm nodes using
MSVC to green
- 0001 adds support for build and installation of the new transform
modules: hstore_plperl, hstore_plpython and ltree_plpython. Note that
this patch is enough to put back the buildfarm to a green state for
MSVC, but it disables the regression tests for those modules,
something addressed in the next patch...
- 0002 adds support for regression tests for the new modules. The
thing is that we need to check the major version version of Python
available in configuration and choose what are the extensions to
preload before the tests run. It is a little bit hacky... But it has
the merit to work, and I am not sure we could have a cleaner solution
by looking at the Makefiles of the transform modules that use
currently conditions based on $(python_majorversion).
Regards,
I have pushed the first of these with some tweaks.
I'm looking at the second.
Regarding this second patch - apart from the buggy python version test
which I just fixed in the first patch, I see this:
+ if ($pyver eq 2)
+ {
+ push @opts, --load-extension=plpythonu;
+ push @opts, '--load-extension=' . $module . 'u';
+ }
But what do we do for Python3?
Do we actually have a Windows machine building with Python3?
cheers
andrew
--
Sent via pgsql-committers mailing list (pgsql-committ...@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
