Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

Tom Lane wrote:
>> Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

I wrote:
> Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

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


0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1-function-decl.patch
Description: 0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1-function-decl.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

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


0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1-function-decl.patch
Description: 0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1-function-decl.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Albe Laurenz  writes:
> The buildfarm log at
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=thrips=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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Craig Ringer  writes:
> On 18 October 2016 at 04:11, Tom Lane  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Craig Ringer  writes:
> On 18 October 2016 at 04:19, Andres Freund  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Pavel Stehule]

2016-10-18 5:48 GMT+02:00 Craig Ringer :

> On 18 October 2016 at 04:19, Andres Freund  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


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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Craig Ringer]

On 18 October 2016 at 04:11, Tom Lane  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Craig Ringer]

On 18 October 2016 at 04:19, Andres Freund  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Yury Zhuravlev]

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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Yury Zhuravlev]

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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Robert Haas]

On Mon, Oct 17, 2016 at 4:42 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Robert Haas  writes:
> On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Andres Freund]

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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Robert Haas]

On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz  
>> 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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Robert Haas  writes:
> On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz  
> 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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Robert Haas]

On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

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


0001-Add-PGDLLEXPORT-to-sample-C-function.patch
Description: 0001-Add-PGDLLEXPORT-to-sample-C-function.patch


0002-Add-instructions-for-building-C-functions-with-MSVC.patch
Description: 0002-Add-instructions-for-building-C-functions-with-MSVC.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Craig Ringer]

On 13 Oct. 2016 05:28, "Tom Lane"  wrote:
>
> I wrote:
> > Albe Laurenz  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.

https://www.postgresql.org/message-id/flat/27019.1397571477%40sss.pgh.pa.us#27019.1397571...@sss.pgh.pa.us

I think the discussion last time came down to you and I disagreeing about
Microsoft droppings too ;)

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

I wrote:
> Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

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


0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1.patch
Description: 0001-Add-PGDLLEXPORT-to-PG_FUNCTION_INFO_V1.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

Tom Lane wrote:
> Albe Laurenz  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

Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Tom Lane]

Albe Laurenz  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 _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

[HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1

[Albe Laurenz]

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


0001-Add-PGDLLEXPORT-to-function-declaration-in-PG_FUNCTI.patch
Description: 0001-Add-PGDLLEXPORT-to-function-declaration-in-PG_FUNCTI.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers