Обсуждение: 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

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

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

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

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

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

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

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

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

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

<p dir="ltr">On 13 Oct. 2016 05:28, "Tom Lane" <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>
wrote:<br/> ><br /> > I wrote:<br /> > > Albe Laurenz <<a
href="mailto:laurenz.albe@wien.gv.at">laurenz.albe@wien.gv.at</a>>writes:<br /> > >> Tom Lane wrote:<br />
>>>> I'm okay with adding PGDLLEXPORT to the extern, but we should update<br /> > >>> that
commentto note that it's not necessary with any of our standard<br /> > >>> Windows build processes.  (For
thatmatter, the comment fails to explain<br /> > >>> why this macro is providing an extern for the base
functionat all...)<br /> ><br /> > >> Here is a patch for that, including an attempt to improve the
comment.<br/> ><br /> > > Pushed with some further twiddling of the comment.<br /> ><br /> > Well, the
buildfarmdoesn't like that even a little bit.  It seems that<br /> > the MSVC compiler does not like seeing both
"externDatum foo(...)" and<br /> > "extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in<br /> >
a.h file is failing.  There is also quite a bit of phase-of-the-moon<br /> > behavior in here, because in some cases
somefunctions are raising errors<br /> > and others that look entirely the same are not.<p dir="ltr">Pretty sure we
discussedand did exactly this before around 9.4. Will check archives...<p dir="ltr">Yeah. Here's the thread.<p
dir="ltr"><a
href="https://www.postgresql.org/message-id/flat/27019.1397571477%40sss.pgh.pa.us#27019.1397571477@sss.pgh.pa.us">https://www.postgresql.org/message-id/flat/27019.1397571477%40sss.pgh.pa.us#27019.1397571477@sss.pgh.pa.us</a><p
dir="ltr">Ithink 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

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 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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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