Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
Currently, PG_FUNCTION_INFO_V1 is defined as
/*
* Macro to build an info function associated with the given function name.
* Win32 loadable functions usually link with 'dlltool --export-all', but it
* doesn't hurt to add PGDLLIMPORT in case they don't.
*/
#define PG_FUNCTION_INFO_V1(funcname) \
Datum funcname(PG_FUNCTION_ARGS); \
extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
const Pg_finfo_record * \
CppConcat(pg_finfo_,funcname) (void) \
{ \
static const Pg_finfo_record my_finfo = { 1 }; \
return &my_finfo; \
} \
extern int no_such_variable
Is there a good reason why the "funcname" declaration is not decorated
with PGDLLEXPORT?
It would do the right thing on Windows and save people the trouble of
either using an export definition file or re-declaring the function with
the PGDLLEXPORT decoration.
An SQL function that is not exported does not make much sense, right?
BTW, I admit I don't understand the comment. What does PGDLLIMPORT do
for a dynamically loaded function?
I propose the attached patch.
Yours,
Laurenz Albe
Attachments:
0001-Add-PGDLLEXPORT-to-function-declaration-in-PG_FUNCTI.patchapplication/octet-stream; name=0001-Add-PGDLLEXPORT-to-function-declaration-in-PG_FUNCTI.patchDownload
From 1535731a0b84cb4e5e72b233f792d5b5c6ec7fee Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@wien.gv.at>
Date: Wed, 12 Oct 2016 10:30:26 +0200
Subject: [PATCH] Add PGDLLEXPORT to function declaration in PG_FUNCTION_INFO_V1
That way functions declared with the version-1 calling convention
will automatically be exported on Windows.
---
src/include/fmgr.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 0491e2e..6e37a95 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -348,7 +348,7 @@ typedef const Pg_finfo_record *(*PGFInfoFunction) (void);
* doesn't hurt to add PGDLLIMPORT in case they don't.
*/
#define PG_FUNCTION_INFO_V1(funcname) \
-Datum funcname(PG_FUNCTION_ARGS); \
+PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS); \
extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
const Pg_finfo_record * \
CppConcat(pg_finfo_,funcname) (void) \
--
1.7.1
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Currently, PG_FUNCTION_INFO_V1 is defined as
/*
* Macro to build an info function associated with the given function name.
* Win32 loadable functions usually link with 'dlltool --export-all', but it
* doesn't hurt to add PGDLLIMPORT in case they don't.
*/
#define PG_FUNCTION_INFO_V1(funcname) \
Datum funcname(PG_FUNCTION_ARGS); \
extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
const Pg_finfo_record * \
CppConcat(pg_finfo_,funcname) (void) \
{ \
static const Pg_finfo_record my_finfo = { 1 }; \
return &my_finfo; \
} \
extern int no_such_variable
Is there a good reason why the "funcname" declaration is not decorated
with PGDLLEXPORT?
The lack of complaints about this suggest that it's not actually necessary
to do so. So what this makes me wonder is whether we can't drop the
DLLEXPORT on the finfo function too. I'd rather reduce the number of
Microsoft-isms in the code, not increase it.
BTW, I admit I don't understand the comment.
It seems like an obvious typo. Or, possibly, sloppy thinking that
contributed to failure to recognize that the keyword isn't needed.
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
Tom Lane wrote:
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Currently, PG_FUNCTION_INFO_V1 is defined as
[...]
Is there a good reason why the "funcname" declaration is not decorated
with PGDLLEXPORT?The lack of complaints about this suggest that it's not actually necessary
to do so. So what this makes me wonder is whether we can't drop the
DLLEXPORT on the finfo function too. I'd rather reduce the number of
Microsoft-isms in the code, not increase it.
I understand the sentiment.
But it seems to actually cause a problem on Windows, at least there was a
complaint here: http://stackoverflow.com/q/39964233
Adding PGDLLEXPORT solved the problem there.
I guess that there are not more complaints about that because
few people build C functions on Windows themselves (lack of PGXS)
and those who do are probably knowledgeable enough that they can
fix it themselves by sticking an extra declaration with PGDLLEXPORT
into their source file.
PostgreSQL itself seems to use export files that explicitly declare the
exported symbols, so it gets away without these decorations.
BTW, I admit I don't understand the comment.
It seems like an obvious typo. Or, possibly, sloppy thinking that
contributed to failure to recognize that the keyword isn't needed.
Looking through the history, it seems to be as follows:
- In 5fde8613, the comment was added (mistakenly) together with a DLLIMPORT decoration.
- In 77e50a61, the decoration was changed to PGDLLEXPORT, but the comment forgotten.
- In e7128e8d, the function prototype was added to the macro, but
the PGDLLEXPORT decoration was forgotten.
The dlltool mentioned is MinGW, so it doesn't apply to people building
with MSVC.
Maybe the comment should be like this:
/*
* Macro to build an info function associated with the given function name.
* Win32 loadable functions usually link with 'dlltool --export-all' or use
* a .DEF file, but it doesn't hurt to add PGDLLEXPORT in case they don't.
*/
Yours,
Laurenz Albe
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Tom Lane wrote:
The lack of complaints about this suggest that it's not actually necessary
to do so. So what this makes me wonder is whether we can't drop the
DLLEXPORT on the finfo function too. I'd rather reduce the number of
Microsoft-isms in the code, not increase it.
I understand the sentiment.
But it seems to actually cause a problem on Windows, at least there was a
complaint here: http://stackoverflow.com/q/39964233
Adding PGDLLEXPORT solved the problem there.
I guess that there are not more complaints about that because
few people build C functions on Windows themselves (lack of PGXS)
and those who do are probably knowledgeable enough that they can
fix it themselves by sticking an extra declaration with PGDLLEXPORT
into their source file.
PostgreSQL itself seems to use export files that explicitly declare the
exported symbols, so it gets away without these decorations.
Except that we don't. There aren't PGDLLEXPORT markings for any
functions exported from contrib modules, and we don't use dlltool
on them either. By your argument, none of contrib would work on
Windows builds at all, but we have a ton of buildfarm evidence and
successful field use to the contrary. How is that all working?
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
Tom Lane wrote:
PostgreSQL itself seems to use export files that explicitly declare the
exported symbols, so it gets away without these decorations.Except that we don't. There aren't PGDLLEXPORT markings for any
functions exported from contrib modules, and we don't use dlltool
on them either. By your argument, none of contrib would work on
Windows builds at all, but we have a ton of buildfarm evidence and
successful field use to the contrary. How is that all working?
I thought it was the job of src/tools/msvc/gendef.pl to generate
.DEF files? In the buildfarm logs I can find lines like:
Generating CITEXT.DEF from directory Release\citext, platform x64
Generating POSTGRES_FDW.DEF from directory Release\postgres_fdw, platform x64
which are emitted by gendef.pl.
Yours,
Laurenz Albe
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Tom Lane wrote:
Except that we don't. There aren't PGDLLEXPORT markings for any
functions exported from contrib modules, and we don't use dlltool
on them either. By your argument, none of contrib would work on
Windows builds at all, but we have a ton of buildfarm evidence and
successful field use to the contrary. How is that all working?
I thought it was the job of src/tools/msvc/gendef.pl to generate
.DEF files?
Hm, okay, so after further review and googling:
MSVC: gendef.pl is used to build a .DEF file, creating the equivalent
of --export-all-symbols behavior.
Mingw: we use handmade .DEF files for libpq and a few other libraries,
otherwise Makefile.shlib explicitly specifies -Wl,--export-all-symbols.
Cygwin: per https://sourceware.org/binutils/docs-2.17/ld/WIN32.html
--export-all-symbols is the default unless you use a .DEF file or have at
least one symbol marked __declspec(dllexport) --- but the Cygwin build
defines PGDLLEXPORT as empty, so there won't be any of the latter.
So it works for us, but the stackoverflow complainant is evidently using
some homebrew build process that does none of the above.
I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to explain
why this macro is providing an extern for the base function at all...)
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
Tom Lane wrote:
I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to explain
why this macro is providing an extern for the base function at all...)
Here is a patch for that, including an attempt to improve the comment.
Yours,
Laurenz Albe
Attachments:
0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1.patchapplication/octet-stream; name=0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1.patchDownload
From b0be28bb7d3d200d7497d782b0d2daf5bc03aa87 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@wien.gv.at>
Date: Wed, 12 Oct 2016 16:51:37 +0200
Subject: [PATCH] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
That way functions declared with the version-1 calling convention
will automatically be exported on Windows.
Note that this is not necessary for building PostgreSQL, as our
build process takes care of exporting the functions, but it will
make things simpler for third party code using a different build
process.
---
src/include/fmgr.h | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 0491e2e..0f04c3e 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -344,11 +344,13 @@ typedef const Pg_finfo_record *(*PGFInfoFunction) (void);
/*
* Macro to build an info function associated with the given function name.
- * Win32 loadable functions usually link with 'dlltool --export-all', but it
- * doesn't hurt to add PGDLLIMPORT in case they don't.
+ * The function must be exported, and our build process takes care of that
+ * so that PGDLLEXPORT would not be necessary for builing PostgreSQL.
+ * We add it nonetheless so that C functions built with MSVC with a
+ * different build process are guaranteed to be exported.
*/
#define PG_FUNCTION_INFO_V1(funcname) \
-Datum funcname(PG_FUNCTION_ARGS); \
+PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS); \
extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
const Pg_finfo_record * \
CppConcat(pg_finfo_,funcname) (void) \
--
1.7.1
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Tom Lane wrote:
I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to explain
why this macro is providing an extern for the base function at all...)
Here is a patch for that, including an attempt to improve the comment.
Pushed with some further twiddling of the comment.
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
I wrote:
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Tom Lane wrote:
I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to explain
why this macro is providing an extern for the base function at all...)
Here is a patch for that, including an attempt to improve the comment.
Pushed with some further twiddling of the comment.
Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.
We could plaster all those declarations with PGDLLEXPORT, but I'm rather
considerably tempted to go back to the way things were, instead. I do
not like patches that end up with Microsoft-droppings everywhere.
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
I wrote:
Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.
I take back the latter comment --- I was comparing HEAD source code
against errors reported on back branches, and at least some of the
discrepancies are explained by additions/removals of separate "extern"
declarations. But still, if we want to do this we're going to need to
put PGDLLEXPORT in every V1 function extern declaration. We might be
able to remove some of the externs as unnecessary instead, but any way
you slice it it's going to be messy.
For the moment I've reverted the change.
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
On 13 Oct. 2016 05:28, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
I wrote:
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Tom Lane wrote:
I'm okay with adding PGDLLEXPORT to the extern, but we should update
that comment to note that it's not necessary with any of our standard
Windows build processes. (For that matter, the comment fails to
explain
why this macro is providing an extern for the base function at all...)
Here is a patch for that, including an attempt to improve the comment.
Pushed with some further twiddling of the comment.
Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.
Pretty sure we discussed and did exactly this before around 9.4. Will check
archives...
Yeah. Here's the thread.
/messages/by-id/27019.1397571477@sss.pgh.pa.us
I think the discussion last time came down to you and I disagreeing about
Microsoft droppings too ;)
Tom Lane wrote:
Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.I take back the latter comment --- I was comparing HEAD source code
against errors reported on back branches, and at least some of the
discrepancies are explained by additions/removals of separate "extern"
declarations. But still, if we want to do this we're going to need to
put PGDLLEXPORT in every V1 function extern declaration. We might be
able to remove some of the externs as unnecessary instead, but any way
you slice it it's going to be messy.For the moment I've reverted the change.
Sorry for having caused the inconvenience.
Actually I would say that the correct solution is to remove the function
declarations from all the header files in contrib, since from commit e7128e8d
on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right?
Of course one would have to check first that the SQL functions don't try to
call each other, which would require extra forward declarations...
If you are willing to consider that, I'd be happy to prepare a patch.
But I'd understand if you think that this is too much code churn for too little
benefit, even if it could be considered a clean-up.
In that case, I'd argue that in the sample in doc/src/sgml/xfunc.sgml
the function definitions should be changed to read
PGDLLEXPORT Datum foo(PG_FUNCTION_ARGS)
instead of
Datum foo(PG_FUNCTION_ARGS)
because without that the sample fails if you try to build it with MSVC
like the stackoverflow question did.
I'd be happy to prepare a patch for that as well.
Yours,
Laurenz Albe
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
But I'd understand if you think that this is too much code churn for too little
benefit, even if it could be considered a clean-up.In that case, I'd argue that in the sample in doc/src/sgml/xfunc.sgml
the function definitions should be changed to readPGDLLEXPORT Datum foo(PG_FUNCTION_ARGS)
instead of
Datum foo(PG_FUNCTION_ARGS)
because without that the sample fails if you try to build it with MSVC
like the stackoverflow question did.
Since I didn't hear from you, I assume that you are not in favour of
removing the SQL function declarations from contrib.
So I went ahead and wrote a patch to add PGDLLEXPORT to the C function sample.
While at it, I noticed that there are no instructions for building and
linking C functions with MSVC, so I have added a patch for that as well.
Yours,
Laurenz Albe
Attachments:
0001-Add-PGDLLEXPORT-to-sample-C-function.patchapplication/octet-stream; name=0001-Add-PGDLLEXPORT-to-sample-C-function.patchDownload
From d676a8e6d8daa29b943d21ffb8fd556a99a8f8bd Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@wien.gv.at>
Date: Mon, 17 Oct 2016 16:42:33 +0200
Subject: [PATCH 1/2] Add PGDLLEXPORT to sample C function
This is needed for the sample to compile with MSVC.
---
doc/src/sgml/xfunc.sgml | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index de6a466..cb527c0 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -2323,7 +2323,7 @@ PG_MODULE_MAGIC;
/* by value */
-PG_FUNCTION_INFO_V1(add_one);
+PGDLLEXPORT PG_FUNCTION_INFO_V1(add_one);
Datum
add_one(PG_FUNCTION_ARGS)
@@ -2335,7 +2335,7 @@ add_one(PG_FUNCTION_ARGS)
/* by reference, fixed length */
-PG_FUNCTION_INFO_V1(add_one_float8);
+PGDLLEXPORT PG_FUNCTION_INFO_V1(add_one_float8);
Datum
add_one_float8(PG_FUNCTION_ARGS)
@@ -2346,7 +2346,7 @@ add_one_float8(PG_FUNCTION_ARGS)
PG_RETURN_FLOAT8(arg + 1.0);
}
-PG_FUNCTION_INFO_V1(makepoint);
+PGDLLEXPORT PG_FUNCTION_INFO_V1(makepoint);
Datum
makepoint(PG_FUNCTION_ARGS)
@@ -2364,7 +2364,7 @@ makepoint(PG_FUNCTION_ARGS)
/* by reference, variable length */
-PG_FUNCTION_INFO_V1(copytext);
+PGDLLEXPORT PG_FUNCTION_INFO_V1(copytext);
Datum
copytext(PG_FUNCTION_ARGS)
@@ -2384,7 +2384,7 @@ copytext(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(new_t);
}
-PG_FUNCTION_INFO_V1(concat_text);
+PGDLLEXPORT PG_FUNCTION_INFO_V1(concat_text);
Datum
concat_text(PG_FUNCTION_ARGS)
@@ -2641,7 +2641,7 @@ c_overpaid(HeapTupleHeader t, /* the current row of emp */
PG_MODULE_MAGIC;
#endif
-PG_FUNCTION_INFO_V1(c_overpaid);
+PGDLLEXPORT PG_FUNCTION_INFO_V1(c_overpaid);
Datum
c_overpaid(PG_FUNCTION_ARGS)
@@ -3056,7 +3056,7 @@ my_set_returning_function(PG_FUNCTION_ARGS)
A complete example of a simple <acronym>SRF</> returning a composite type
looks like:
<programlisting><![CDATA[
-PG_FUNCTION_INFO_V1(retcomposite);
+PGDLLEXPORT PG_FUNCTION_INFO_V1(retcomposite);
Datum
retcomposite(PG_FUNCTION_ARGS)
@@ -3210,7 +3210,7 @@ CREATE OR REPLACE FUNCTION retcomposite(IN integer, IN integer,
element of any type, and return a one-dimensional array of that type:
<programlisting>
-PG_FUNCTION_INFO_V1(make_array);
+PGDLLEXPORT PG_FUNCTION_INFO_V1(make_array);
Datum
make_array(PG_FUNCTION_ARGS)
{
--
1.7.1
0002-Add-instructions-for-building-C-functions-with-MSVC.patchapplication/octet-stream; name=0002-Add-instructions-for-building-C-functions-with-MSVC.patchDownload
From 77ccb3b64d8e2bccce1b2780ae1b0cca0cc1647b Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@wien.gv.at>
Date: Mon, 17 Oct 2016 16:44:00 +0200
Subject: [PATCH 2/2] Add instructions for building C functions with MSVC
---
doc/src/sgml/dfunc.sgml | 21 +++++++++++++++++++++
1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/doc/src/sgml/dfunc.sgml b/doc/src/sgml/dfunc.sgml
index 6a4b7d6..8234514 100644
--- a/doc/src/sgml/dfunc.sgml
+++ b/doc/src/sgml/dfunc.sgml
@@ -205,6 +205,27 @@ gcc -G -o foo.so foo.o
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>
+ <systemitem class="osname">Windows with MSVC</>
+ <indexterm><primary>Windows</><secondary>shared library</></>
+ </term>
+ <listitem>
+ <para>
+ Windows does not use <acronym>PIC</acronym>, so the shared library
+ has to be linked with the PostgreSQL server's import library
+ <filename>postgres.lib</filename>.
+ The object file will be named <filename>foo.obj</filename>
+ and the shared library will be named <filename>foo.dll</filename>.
+ The compiler flag to create a shared library is <option>/LD</option>.
+<programlisting>
+cl /c foo.c
+cl /LD /Fe:foo.dll foo.obj postgres.lib
+</programlisting>
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
<tip>
--
1.7.1
On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz <laurenz.albe@wien.gv.at> wrote:
Tom Lane wrote:
Well, the buildfarm doesn't like that even a little bit. It seems that
the MSVC compiler does not like seeing both "extern Datum foo(...)" and
"extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in
a .h file is failing. There is also quite a bit of phase-of-the-moon
behavior in here, because in some cases some functions are raising errors
and others that look entirely the same are not.I take back the latter comment --- I was comparing HEAD source code
against errors reported on back branches, and at least some of the
discrepancies are explained by additions/removals of separate "extern"
declarations. But still, if we want to do this we're going to need to
put PGDLLEXPORT in every V1 function extern declaration. We might be
able to remove some of the externs as unnecessary instead, but any way
you slice it it's going to be messy.For the moment I've reverted the change.
Sorry for having caused the inconvenience.
Actually I would say that the correct solution is to remove the function
declarations from all the header files in contrib, since from commit e7128e8d
on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right?
Right. Why isn't that already the case? Commit e7128e8d seems to
have tried. Did it just miss a bunch of cases?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz <laurenz.albe@wien.gv.at> wrote:
Actually I would say that the correct solution is to remove the function
declarations from all the header files in contrib, since from commit e7128e8d
on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right?
Right. Why isn't that already the case? Commit e7128e8d seems to
have tried. Did it just miss a bunch of cases?
That only works to the extent that there are no cross-file references to
those symbols within the extension. If that's true for 99% or more of
them then this is probably a good way to go. If it's only true for, say,
75%, I'm not sure we want to decimate the headers that way. We'd end
up with something doubly ugly: both haphazard-looking lists of only
some of the symbols, and PGDLLEXPORT marks on those that remain.
As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.
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
On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz <laurenz.albe@wien.gv.at> wrote:
Actually I would say that the correct solution is to remove the function
declarations from all the header files in contrib, since from commit e7128e8d
on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right?Right. Why isn't that already the case? Commit e7128e8d seems to
have tried. Did it just miss a bunch of cases?That only works to the extent that there are no cross-file references to
those symbols within the extension. If that's true for 99% or more of
them then this is probably a good way to go. If it's only true for, say,
75%, I'm not sure we want to decimate the headers that way. We'd end
up with something doubly ugly: both haphazard-looking lists of only
some of the symbols, and PGDLLEXPORT marks on those that remain.
I wouldn't think that cross-file references would be especially
common. Functions that take PG_FUNCTION_ARGS and return Datum aren't
a lot of fun to call from C. But maybe I'm wrong.
As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.
So, they'd need to generate export files somehow?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-10-17 16:16:37 -0400, Robert Haas wrote:
I wouldn't think that cross-file references would be especially
common. Functions that take PG_FUNCTION_ARGS and return Datum aren't
a lot of fun to call from C. But maybe I'm wrong.
There's a fair number of DirectFunctionCall$Ns over the backend.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.
So, they'd need to generate export files somehow?
My point is that that's a solved problem. Perhaps the issue is that
we haven't made our src/tools/msvc infrastructure available for outside
use in the way that we've exported our Unix build infrastructure through
PGXS. But if so, I should think that working on that is the thing to do.
[ wanders away wondering what cmake does with this... ]
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
On Mon, Oct 17, 2016 at 4:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.So, they'd need to generate export files somehow?
My point is that that's a solved problem. Perhaps the issue is that
we haven't made our src/tools/msvc infrastructure available for outside
use in the way that we've exported our Unix build infrastructure through
PGXS. But if so, I should think that working on that is the thing to do.
Yeah, I don't know. For my money, decorating the function definitions
in place seems easier than having to maintain a separate export list,
especially if it can be hidden under the carpet using the existing
stupid macro tricks. But I am not a Windows expert.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
17 октября 2016 г. 23:42:30 MSK, Tom Lane wrote:
[ wanders away wondering what cmake does with this... ]
CMake can export all symbols using only one setting -
WINDOWS_EXPORT_ALL_SYMBOLS for shared libraries and special for Postgres I
made "export all" for executable files. You can try this in
cmake-3.7.0-rc1.
Current postgres_cmake is using the modified gendef.pl (use only
stdin/stdout for objtool without any files).
regards,
Yury
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas wrote:
Yeah, I don't know. For my money, decorating the function definitions
in place seems easier than having to maintain a separate export list,
especially if it can be hidden under the carpet using the existing
stupid macro tricks. But I am not a Windows expert.
I suppose we should to establish politics for this case. Because any who
see this and who have experience in Windows surprised by this. For windows
any DLL it is like plugins which must use strict API for communications and
resolving symbols. The situation is that in Postgres we have not API for
extensions in the Windows terms.
In future CMake will hide all this troubles in itself but if tell in truth
I don't like this situation when any extension has access to any non-static
symbols. However time to time we meet static function that we want to
reusing in our extension and today I know only one decision - copy-paste.
Without strict politics in this case we will be time to time meet new
persons who ask this or similar question.
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 18 October 2016 at 04:19, Andres Freund <andres@anarazel.de> wrote:
On 2016-10-17 16:16:37 -0400, Robert Haas wrote:
I wouldn't think that cross-file references would be especially
common. Functions that take PG_FUNCTION_ARGS and return Datum aren't
a lot of fun to call from C. But maybe I'm wrong.There's a fair number of DirectFunctionCall$Ns over the backend.
It's only an issue if one contrib calls another contrib (or the core
backend code calls into a contrib, but that's unlikely) via
DirectFunctionCall .
If changed to use regular OidFunctionCall they'll go via the catalogs
and be fine, albeit at a small performance penalty. In many cases that
can be eliminated by caching the fmgr info and using FunctionCall
directly, but it requires taking a lock on the function or registering
for invalidations so it's often not worth it.
Personally I think it'd be cleaner to avoid one contrib referencing
another's headers directly and we have the fmgr infrastructure to make
it unnecessary. But I don't really think it's a problem that
especially needs solving either.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 18 October 2016 at 04:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:
As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.
Effectively, "PGXS for Windows".
I had a quick look at getting that going a while ago, but it was going
to leave me eyeballs-deep in Perl and I quickly found something more
interesting to do.
I think it's worthwhile, but only if we can agree in advance that the
necessary infrastructure will be backported to all supported branches
if at all viable. Otherwise it's a massive waste of time, since you
can't actually avoid needing your own homebrew build for 5+ years.
I've kind of been hoping the CMake work would make the whole mess of
Perl build stuff go away. CMake would solve this quite neatly since we
can bundle CMake parameters file for inclusion in other projects and
could also tell pg_config how to point to it. Extensions then become
trivial CMake projects.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2016-10-18 5:48 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:
On 18 October 2016 at 04:19, Andres Freund <andres@anarazel.de> wrote:
On 2016-10-17 16:16:37 -0400, Robert Haas wrote:
I wouldn't think that cross-file references would be especially
common. Functions that take PG_FUNCTION_ARGS and return Datum aren't
a lot of fun to call from C. But maybe I'm wrong.There's a fair number of DirectFunctionCall$Ns over the backend.
It's only an issue if one contrib calls another contrib (or the core
backend code calls into a contrib, but that's unlikely) via
DirectFunctionCall .If changed to use regular OidFunctionCall they'll go via the catalogs
and be fine, albeit at a small performance penalty. In many cases that
can be eliminated by caching the fmgr info and using FunctionCall
directly, but it requires taking a lock on the function or registering
for invalidations so it's often not worth it.Personally I think it'd be cleaner to avoid one contrib referencing
another's headers directly and we have the fmgr infrastructure to make
it unnecessary. But I don't really think it's a problem that
especially needs solving either.
Not all called functions has V1 interface. I understand so plpgsql_check is
not usual extension and it is a exception, but there is lot of cross calls.
I can use a technique used by Tom in last changes in python's extension,
but I am not able do check in linux gcc.
Regards
Pavel
Show quoted text
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Craig Ringer <craig@2ndquadrant.com> writes:
On 18 October 2016 at 04:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:
As for the core problem, I wonder why we aren't recommending that
third-party modules be built using the same infrastructure contrib
uses, rather than people ginning up their own infrastructure and
then finding out the hard way that that means they need PGDLLEXPORT
marks.
Effectively, "PGXS for Windows".
Yeah.
I've kind of been hoping the CMake work would make the whole mess of
Perl build stuff go away. CMake would solve this quite neatly since we
can bundle CMake parameters file for inclusion in other projects and
could also tell pg_config how to point to it. Extensions then become
trivial CMake projects.
Agreed, it'd be wise not to put any effort into that until we see
whether the CMake conversion succeeds.
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
Craig Ringer <craig@2ndquadrant.com> writes:
On 18 October 2016 at 04:19, Andres Freund <andres@anarazel.de> wrote:
On 2016-10-17 16:16:37 -0400, Robert Haas wrote:
I wouldn't think that cross-file references would be especially
common. Functions that take PG_FUNCTION_ARGS and return Datum aren't
a lot of fun to call from C. But maybe I'm wrong.
There's a fair number of DirectFunctionCall$Ns over the backend.
It's only an issue if one contrib calls another contrib (or the core
backend code calls into a contrib, but that's unlikely) via
DirectFunctionCall .
No, it's cross *file* references within a single contrib module that
would be likely to need extern declarations in a header file. That's
not especially weird IMO. I'm not sure how many cases there actually
are though.
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
I wrote:
No, it's cross *file* references within a single contrib module that
would be likely to need extern declarations in a header file. That's
not especially weird IMO. I'm not sure how many cases there actually
are though.
I poked at this a little bit. AFAICT, the only actual cross-file
references are in contrib/ltree/, which has quite a few. Maybe we
could hold our noses and attach PGDLLEXPORT to the declarations in
ltree.h.
hstore's HSTORE_POLLUTE macro would also need PGDLLEXPORT-ification,
but that's just within the macro so it wouldn't be too ugly.
The other problem is xml2's xml_is_well_formed(), which duplicates
the name of a core backend function. If we PGDLLEXPORT-ify that,
we get conflicts against the core's declaration in utils/xml.h.
I do not see any nice way to dodge that. Maybe we could decide that
six releases' worth of backwards compatibility is enough and
just drop it from xml2 --- AFAICS, that would only break applications
that had never updated to the extension-ified version of xml2 at all.
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
Tom Lane wrote:
No, it's cross *file* references within a single contrib module that
would be likely to need extern declarations in a header file. That's
not especially weird IMO. I'm not sure how many cases there actually
are though.
I went through the contrib moules and tried to look that up,
and now I am even more confused than before.
The buildfarm log at
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=thrips&dt=2016-10-12%2018%3A37%3A26
shows the build failing (among others) for contrib/tablefunc
(close to the bottom end of the log).
Now when I look at the source, there is only one C file, and the
failing functions are *not* declared anywhere, neither in the C file
nor in the (almost empty) header file.
So it must be something other than double declaration that makes
the build fail.
Could it be that the .DEF files have something to do with that?
Yours,
Laurenz Albe
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
The buildfarm log at
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=thrips&dt=2016-10-12%2018%3A37%3A26
shows the build failing (among others) for contrib/tablefunc
(close to the bottom end of the log).
That's a build of 9.6.
Now when I look at the source, there is only one C file, and the
failing functions are *not* declared anywhere, neither in the C file
nor in the (almost empty) header file.
The declarations in question seem to have been removed in
0665023b4435a469e42289d7065c436967a022b6, which is only in HEAD.
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
Tom Lane wrote:
I poked at this a little bit. AFAICT, the only actual cross-file
references are in contrib/ltree/, which has quite a few. Maybe we
could hold our noses and attach PGDLLEXPORT to the declarations in
ltree.h.hstore's HSTORE_POLLUTE macro would also need PGDLLEXPORT-ification,
but that's just within the macro so it wouldn't be too ugly.The other problem is xml2's xml_is_well_formed(), which duplicates
the name of a core backend function. If we PGDLLEXPORT-ify that,
we get conflicts against the core's declaration in utils/xml.h.
I do not see any nice way to dodge that. Maybe we could decide that
six releases' worth of backwards compatibility is enough and
just drop it from xml2 --- AFAICS, that would only break applications
that had never updated to the extension-ified version of xml2 at all.
I guess it would also be possible, but undesirable, to decorate the
declaration in utils/xml.h.
Anyway, I have prepared a patch along the lines you suggest.
Yours,
Laurenz Albe
Attachments:
0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1-function-decl.patchapplication/octet-stream; name=0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1-function-decl.patchDownload
From 53f1fe40400cf7174a95e50380ddb538edf03dcb Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@wien.gv.at>
Date: Mon, 24 Oct 2016 17:32:27 +0200
Subject: [PATCH] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1 function declaration
On Windows, the function and info function must be exported. Our normal
build processes take care of that via .DEF files or --export-all-symbols.
Module authors using a different build process might do it differently,
so we declare these functions PGDLLEXPORT for their convenience.
MSVC builds fail if there are several declarations of the same function,
some of which have PGDLLEXPORT and some don't, so we have to decorate
the declarations in the ltree and hstore contrib modules accordingly.
Also, remove xml2's xml_is_well_formed() since it would clash with the
function of the same name declared in utils/xml.h. This has been there
only to support users who didn't upgrade their C function definitions
since the function has been moved into core in 9.1 (commit a0b7b717a4).
---
contrib/hstore/hstore.h | 4 ++--
contrib/ltree/ltree.h | 40 ++++++++++++++++++++--------------------
contrib/xml2/xpath.c | 45 ---------------------------------------------
src/include/fmgr.h | 7 +++----
4 files changed, 25 insertions(+), 71 deletions(-)
diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
index 6bab08b..99f9eee 100644
--- a/contrib/hstore/hstore.h
+++ b/contrib/hstore/hstore.h
@@ -194,8 +194,8 @@ extern Pairs *hstoreArrayToPairs(ArrayType *a, int *npairs);
#if HSTORE_POLLUTE_NAMESPACE
#define HSTORE_POLLUTE(newname_,oldname_) \
PG_FUNCTION_INFO_V1(oldname_); \
- Datum newname_(PG_FUNCTION_ARGS); \
- Datum oldname_(PG_FUNCTION_ARGS) { return newname_(fcinfo); } \
+ PGDLLEXPORT Datum newname_(PG_FUNCTION_ARGS); \
+ PGDLLEXPORT Datum oldname_(PG_FUNCTION_ARGS) { return newname_(fcinfo); } \
extern int no_such_variable
#else
#define HSTORE_POLLUTE(newname_,oldname_) \
diff --git a/contrib/ltree/ltree.h b/contrib/ltree/ltree.h
index c604357..28bc2d7 100644
--- a/contrib/ltree/ltree.h
+++ b/contrib/ltree/ltree.h
@@ -130,30 +130,30 @@ typedef struct
/* use in array iterator */
-Datum ltree_isparent(PG_FUNCTION_ARGS);
-Datum ltree_risparent(PG_FUNCTION_ARGS);
-Datum ltq_regex(PG_FUNCTION_ARGS);
-Datum ltq_rregex(PG_FUNCTION_ARGS);
-Datum lt_q_regex(PG_FUNCTION_ARGS);
-Datum lt_q_rregex(PG_FUNCTION_ARGS);
-Datum ltxtq_exec(PG_FUNCTION_ARGS);
-Datum ltxtq_rexec(PG_FUNCTION_ARGS);
-Datum _ltq_regex(PG_FUNCTION_ARGS);
-Datum _ltq_rregex(PG_FUNCTION_ARGS);
-Datum _lt_q_regex(PG_FUNCTION_ARGS);
-Datum _lt_q_rregex(PG_FUNCTION_ARGS);
-Datum _ltxtq_exec(PG_FUNCTION_ARGS);
-Datum _ltxtq_rexec(PG_FUNCTION_ARGS);
-Datum _ltree_isparent(PG_FUNCTION_ARGS);
-Datum _ltree_risparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_isparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_risparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltq_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltq_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum lt_q_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum lt_q_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltxtq_exec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltxtq_rexec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltq_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltq_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _lt_q_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _lt_q_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltxtq_exec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltxtq_rexec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltree_isparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltree_risparent(PG_FUNCTION_ARGS);
/* Concatenation functions */
-Datum ltree_addltree(PG_FUNCTION_ARGS);
-Datum ltree_addtext(PG_FUNCTION_ARGS);
-Datum ltree_textadd(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_addltree(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_addtext(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_textadd(PG_FUNCTION_ARGS);
/* Util function */
-Datum ltree_in(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_in(PG_FUNCTION_ARGS);
bool ltree_execute(ITEM *curitem, void *checkval,
bool calcnot, bool (*chkcond) (void *checkval, ITEM *val));
diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index ac28996..28445be 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -81,51 +81,6 @@ pgxml_parser_init(PgXmlStrictness strictness)
}
-/*
- * Returns true if document is well-formed
- *
- * Note: this has been superseded by a core function. We still have to
- * have it in the contrib module so that existing SQL-level references
- * to the function won't fail; but in normal usage with up-to-date SQL
- * definitions for the contrib module, this won't be called.
- */
-
-PG_FUNCTION_INFO_V1(xml_is_well_formed);
-
-Datum
-xml_is_well_formed(PG_FUNCTION_ARGS)
-{
- text *t = PG_GETARG_TEXT_P(0); /* document buffer */
- bool result = false;
- int32 docsize = VARSIZE(t) - VARHDRSZ;
- xmlDocPtr doctree;
- PgXmlErrorContext *xmlerrcxt;
-
- xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
-
- PG_TRY();
- {
- doctree = xmlParseMemory((char *) VARDATA(t), docsize);
-
- result = (doctree != NULL);
-
- if (doctree != NULL)
- xmlFreeDoc(doctree);
- }
- PG_CATCH();
- {
- pg_xml_done(xmlerrcxt, true);
-
- PG_RE_THROW();
- }
- PG_END_TRY();
-
- pg_xml_done(xmlerrcxt, false);
-
- PG_RETURN_BOOL(result);
-}
-
-
/* Encodes special characters (<, >, &, " and \r) as XML entities */
PG_FUNCTION_INFO_V1(xml_encode_special_chars);
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 0878418..3668ac3 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -350,12 +350,11 @@ typedef const Pg_finfo_record *(*PGFInfoFunction) (void);
*
* On Windows, the function and info function must be exported. Our normal
* build processes take care of that via .DEF files or --export-all-symbols.
- * Module authors using a different build process might need to manually
- * declare the function PGDLLEXPORT. We do that automatically here for the
- * info function, since authors shouldn't need to be explicitly aware of it.
+ * Module authors using a different build process might do it differently,
+ * so we declare these functions PGDLLEXPORT for their convenience.
*/
#define PG_FUNCTION_INFO_V1(funcname) \
-extern Datum funcname(PG_FUNCTION_ARGS); \
+extern PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS); \
extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
const Pg_finfo_record * \
CppConcat(pg_finfo_,funcname) (void) \
--
1.7.1
I wrote:
Anyway, I have prepared a patch along the lines you suggest.
It occurred to me that the documentation still suggests that you should
add a declaration to a C function; I have fixed that too.
I'll add the patch to the next commitfest.
Yours,
Laurenz Albe
Attachments:
0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1-function-decl.patchapplication/octet-stream; name=0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1-function-decl.patchDownload
From 7559e7f746a4641011609f0f678daacef9429ca6 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@wien.gv.at>
Date: Tue, 25 Oct 2016 15:16:44 +0200
Subject: [PATCH] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1 function declaration
On Windows, the function and info function must be exported. Our normal
build processes take care of that via .DEF files or --export-all-symbols.
Module authors using a different build process might do it differently,
so we declare these functions PGDLLEXPORT for their convenience.
MSVC builds fail if there are several declarations of the same function,
some of which have PGDLLEXPORT and some don't, so we have to decorate
the declarations in the ltree and hstore contrib modules accordingly.
Also, remove xml2's xml_is_well_formed() since it would clash with the
function of the same name declared in utils/xml.h. This has been there
only to support users who didn't upgrade their C function definitions
since the function has been moved into core in 9.1 (commit a0b7b717a4).
Since commit e7128e8d the best practice is not to add a declaration to
an SQL function in an external module; modify the documentation to
reflect that.
---
contrib/hstore/hstore.h | 4 ++--
contrib/ltree/ltree.h | 40 ++++++++++++++++++++--------------------
contrib/xml2/xpath.c | 45 ---------------------------------------------
doc/src/sgml/xfunc.sgml | 6 +++---
src/include/fmgr.h | 7 +++----
5 files changed, 28 insertions(+), 74 deletions(-)
diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
index 6bab08b..99f9eee 100644
--- a/contrib/hstore/hstore.h
+++ b/contrib/hstore/hstore.h
@@ -194,8 +194,8 @@ extern Pairs *hstoreArrayToPairs(ArrayType *a, int *npairs);
#if HSTORE_POLLUTE_NAMESPACE
#define HSTORE_POLLUTE(newname_,oldname_) \
PG_FUNCTION_INFO_V1(oldname_); \
- Datum newname_(PG_FUNCTION_ARGS); \
- Datum oldname_(PG_FUNCTION_ARGS) { return newname_(fcinfo); } \
+ PGDLLEXPORT Datum newname_(PG_FUNCTION_ARGS); \
+ PGDLLEXPORT Datum oldname_(PG_FUNCTION_ARGS) { return newname_(fcinfo); } \
extern int no_such_variable
#else
#define HSTORE_POLLUTE(newname_,oldname_) \
diff --git a/contrib/ltree/ltree.h b/contrib/ltree/ltree.h
index c604357..28bc2d7 100644
--- a/contrib/ltree/ltree.h
+++ b/contrib/ltree/ltree.h
@@ -130,30 +130,30 @@ typedef struct
/* use in array iterator */
-Datum ltree_isparent(PG_FUNCTION_ARGS);
-Datum ltree_risparent(PG_FUNCTION_ARGS);
-Datum ltq_regex(PG_FUNCTION_ARGS);
-Datum ltq_rregex(PG_FUNCTION_ARGS);
-Datum lt_q_regex(PG_FUNCTION_ARGS);
-Datum lt_q_rregex(PG_FUNCTION_ARGS);
-Datum ltxtq_exec(PG_FUNCTION_ARGS);
-Datum ltxtq_rexec(PG_FUNCTION_ARGS);
-Datum _ltq_regex(PG_FUNCTION_ARGS);
-Datum _ltq_rregex(PG_FUNCTION_ARGS);
-Datum _lt_q_regex(PG_FUNCTION_ARGS);
-Datum _lt_q_rregex(PG_FUNCTION_ARGS);
-Datum _ltxtq_exec(PG_FUNCTION_ARGS);
-Datum _ltxtq_rexec(PG_FUNCTION_ARGS);
-Datum _ltree_isparent(PG_FUNCTION_ARGS);
-Datum _ltree_risparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_isparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_risparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltq_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltq_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum lt_q_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum lt_q_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltxtq_exec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltxtq_rexec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltq_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltq_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _lt_q_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _lt_q_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltxtq_exec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltxtq_rexec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltree_isparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum _ltree_risparent(PG_FUNCTION_ARGS);
/* Concatenation functions */
-Datum ltree_addltree(PG_FUNCTION_ARGS);
-Datum ltree_addtext(PG_FUNCTION_ARGS);
-Datum ltree_textadd(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_addltree(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_addtext(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_textadd(PG_FUNCTION_ARGS);
/* Util function */
-Datum ltree_in(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum ltree_in(PG_FUNCTION_ARGS);
bool ltree_execute(ITEM *curitem, void *checkval,
bool calcnot, bool (*chkcond) (void *checkval, ITEM *val));
diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index ac28996..28445be 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -81,51 +81,6 @@ pgxml_parser_init(PgXmlStrictness strictness)
}
-/*
- * Returns true if document is well-formed
- *
- * Note: this has been superseded by a core function. We still have to
- * have it in the contrib module so that existing SQL-level references
- * to the function won't fail; but in normal usage with up-to-date SQL
- * definitions for the contrib module, this won't be called.
- */
-
-PG_FUNCTION_INFO_V1(xml_is_well_formed);
-
-Datum
-xml_is_well_formed(PG_FUNCTION_ARGS)
-{
- text *t = PG_GETARG_TEXT_P(0); /* document buffer */
- bool result = false;
- int32 docsize = VARSIZE(t) - VARHDRSZ;
- xmlDocPtr doctree;
- PgXmlErrorContext *xmlerrcxt;
-
- xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
-
- PG_TRY();
- {
- doctree = xmlParseMemory((char *) VARDATA(t), docsize);
-
- result = (doctree != NULL);
-
- if (doctree != NULL)
- xmlFreeDoc(doctree);
- }
- PG_CATCH();
- {
- pg_xml_done(xmlerrcxt, true);
-
- PG_RE_THROW();
- }
- PG_END_TRY();
-
- pg_xml_done(xmlerrcxt, false);
-
- PG_RETURN_BOOL(result);
-}
-
-
/* Encodes special characters (<, >, &, " and \r) as XML entities */
PG_FUNCTION_INFO_V1(xml_encode_special_chars);
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index de6a466..9004063 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -2280,13 +2280,13 @@ CREATE FUNCTION concat_text(text, text) RETURNS text
of the complexity of passing arguments and results. The C declaration
of a version-1 function is always:
<programlisting>
-Datum funcname(PG_FUNCTION_ARGS)
+PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS)
</programlisting>
- In addition, the macro call:
+ This is declared automatically by the macro call:
<programlisting>
PG_FUNCTION_INFO_V1(funcname);
</programlisting>
- must appear in the same source file. (Conventionally, it's
+ which must appear in the same source file. (Conventionally, it's
written just before the function itself.) This macro call is not
needed for <literal>internal</>-language functions, since
<productname>PostgreSQL</> assumes that all internal functions
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 0878418..3668ac3 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -350,12 +350,11 @@ typedef const Pg_finfo_record *(*PGFInfoFunction) (void);
*
* On Windows, the function and info function must be exported. Our normal
* build processes take care of that via .DEF files or --export-all-symbols.
- * Module authors using a different build process might need to manually
- * declare the function PGDLLEXPORT. We do that automatically here for the
- * info function, since authors shouldn't need to be explicitly aware of it.
+ * Module authors using a different build process might do it differently,
+ * so we declare these functions PGDLLEXPORT for their convenience.
*/
#define PG_FUNCTION_INFO_V1(funcname) \
-extern Datum funcname(PG_FUNCTION_ARGS); \
+extern PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS); \
extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
const Pg_finfo_record * \
CppConcat(pg_finfo_,funcname) (void) \
--
1.7.1
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Anyway, I have prepared a patch along the lines you suggest.
Pushed, we'll see if the buildfarm likes this iteration any better.
It occurred to me that the documentation still suggests that you should
add a declaration to a C function; I have fixed that too.
I didn't like that and rewrote it a bit. It's not specific to V1
functions.
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
I wrote:
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Anyway, I have prepared a patch along the lines you suggest.
Pushed, we'll see if the buildfarm likes this iteration any better.
And the answer is "not very much". The Windows builds aren't actually
failing, but they are producing lots of warnings:
lquery_op.obj : warning LNK4197: export '_ltq_regex' specified multiple times; using first specification
lquery_op.obj : warning LNK4197: export '_ltq_rregex' specified multiple times; using first specification
lquery_op.obj : warning LNK4197: export '_lt_q_regex' specified multiple times; using first specification
lquery_op.obj : warning LNK4197: export '_lt_q_rregex' specified multiple times; using first specification
ltree_gist.obj : warning LNK4197: export '_ltree_compress' specified multiple times; using first specification
ltree_gist.obj : warning LNK4197: export '_ltree_same' specified multiple times; using first specification
ltree_gist.obj : warning LNK4197: export '_ltree_union' specified multiple times; using first specification
ltree_gist.obj : warning LNK4197: export '_ltree_penalty' specified multiple times; using first specification
ltree_gist.obj : warning LNK4197: export '_ltree_picksplit' specified multiple times; using first specification
ltree_gist.obj : warning LNK4197: export '_ltree_consistent' specified multiple times; using first specification
ltree_op.obj : warning LNK4197: export '_ltree_isparent' specified multiple times; using first specification
ltree_op.obj : warning LNK4197: export '_ltree_risparent' specified multiple times; using first specification
ltree_op.obj : warning LNK4197: export '_lca' specified multiple times; using first specification
ltxtquery_op.obj : warning LNK4197: export '_ltxtq_exec' specified multiple times; using first specification
ltxtquery_op.obj : warning LNK4197: export '_ltxtq_rexec' specified multiple times; using first specification
This is evidently from the places where there are two "extern"
declarations for a function, one in a header and one in
PG_FUNCTION_INFO_V1. The externs are identical now, but nonetheless
MSVC insists on whining about it.
I'm inclined to give this up as a bad job and go back to the
previous state. We have a solution that works and doesn't
produce warnings; third-party authors who don't want to use it
are on their own.
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
Tom Lane wrote:
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Anyway, I have prepared a patch along the lines you suggest.
Pushed, we'll see if the buildfarm likes this iteration any better.
And the answer is "not very much". The Windows builds aren't actually
failing, but they are producing lots of warnings:lquery_op.obj : warning LNK4197: export '_ltq_regex' specified multiple times; using first specification
[...]
This is evidently from the places where there are two "extern"
declarations for a function, one in a header and one in
PG_FUNCTION_INFO_V1. The externs are identical now, but nonetheless
MSVC insists on whining about it.
Funny -- it seems to mind a duplicate declaration only if there
is PGDLLEXPORT in at least one of them.
I'm inclined to give this up as a bad job and go back to the
previous state. We have a solution that works and doesn't
produce warnings; third-party authors who don't want to use it
are on their own.
I think you are right.
Thanks for the work and thought you expended on this!
Particularly since Windows is not your primary interest.
Yours,
Laurenz Albe
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Albe Laurenz <laurenz.albe@wien.gv.at> writes:
Tom Lane wrote:
I'm inclined to give this up as a bad job and go back to the
previous state. We have a solution that works and doesn't
produce warnings; third-party authors who don't want to use it
are on their own.
I think you are right.
Done. We can always un-undo it if somebody has another idea.
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