Create function prototype as part of PG_FUNCTION_INFO_V1

This idea has appeared at least twice now, in
http://www.postgresql.org/message-id/1386301050.2743.17.camel@vanquo.pezone.net and http://www.postgresql.org/message-id/52D25AA2.50108@2ndquadrant.com . Even if it doesn't help with Windows issues, as discussed in the second thread, it still seems like a win for reducing boilerplate and accidental compiler warnings. So here is a patch for consideration.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> This idea has appeared at least twice now, in
> http://www.postgresql.org/message-id/1386301050.2743.17.camel@vanquo.pezone.net and http://www.postgresql.org/message-id/52D25AA2.50108@2ndquadrant.com . Even if it doesn't help with Windows issues, as discussed in the second thread, it still seems like a win for reducing boilerplate and accidental compiler warnings. So here is a patch for consideration.

Meh. I don't think that extension authors are really going to appreciate
changing from "thou shalt declare all thy functions" to "thou shalt
declare none of them". If the code were such that it wouldn't matter
whether a manual declaration were provided too, then that wouldn't be a
big deal --- but you seem to be ignoring the discussion in the one thread
cited above about PGDLLEXPORT.

Also, surely it is 100% bogus for fmgr.h to be declaring functions not
actually provided by fmgr.c. That will create about as many failure
modes as it removes, not to mention being conceptually wrong.

The latter point might possibly be worked around by putting the externs
for _PG_init and _PG_fini into the PG_MODULE_MAGIC macro, though I'm not
sure how well that works for multi-source-file extensions; the init
functions might be in some other file than the PG_MODULE_MAGIC call.

regards, tom lane

Hi,

On 2014-01-15 00:41:41 -0500, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > This idea has appeared at least twice now, in
> > http://www.postgresql.org/message-id/1386301050.2743.17.camel@vanquo.pezone.net and http://www.postgresql.org/message-id/52D25AA2.50108@2ndquadrant.com . Even if it doesn't help with Windows issues, as discussed in the second thread, it still seems like a win for reducing boilerplate and accidental compiler warnings. So here is a patch for consideration.
>
> Meh. I don't think that extension authors are really going to appreciate
> changing from "thou shalt declare all thy functions" to "thou shalt
> declare none of them". If the code were such that it wouldn't matter
> whether a manual declaration were provided too, then that wouldn't be a
> big deal --- but you seem to be ignoring the discussion in the one thread
> cited above about PGDLLEXPORT.
>
> Also, surely it is 100% bogus for fmgr.h to be declaring functions not
> actually provided by fmgr.c. That will create about as many failure
> modes as it removes, not to mention being conceptually wrong.
>
> The latter point might possibly be worked around by putting the externs
> for _PG_init and _PG_fini into the PG_MODULE_MAGIC macro, though I'm not
> sure how well that works for multi-source-file extensions; the init
> functions might be in some other file than the PG_MODULE_MAGIC call.

Based on those comments and the lack of counter arguments after a month
I am going to mark the patch as rejected.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2/15/14, 8:51 AM, Andres Freund wrote:
> Based on those comments and the lack of counter arguments after a month
> I am going to mark the patch as rejected.

Actually, I was waiting for that PGDLLIMPORT thread to sort itself out
before tackling this.

On 1/15/14, 12:41 AM, Tom Lane wrote:
> Meh. I don't think that extension authors are really going to appreciate
> changing from "thou shalt declare all thy functions" to "thou shalt
> declare none of them".

This patch does no such thing.

> If the code were such that it wouldn't matter
> whether a manual declaration were provided too, then that wouldn't be a
> big deal --- but you seem to be ignoring the discussion in the one thread
> cited above about PGDLLEXPORT.

In that thread, you argue that requiring PGDLLEXPORT is not acceptable,
so that makes this argument irrelevant.

> Also, surely it is 100% bogus for fmgr.h to be declaring functions not
> actually provided by fmgr.c.

That's the arrangement if you don't have dynamic loading. For dynamic
loading, the question isn't necessarily who provides them, but who
determines the interface for them. There has to be a way for the
postgres backend to say to extension module authors, I'm going to try to
load this function, and this is what it should look like. This case
wasn't envisioned when they designed C in 1970. If you have a better
idea, I'm listening.

> That will create about as many failure
> modes as it removes, not to mention being conceptually wrong.

What failure modes?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 1/15/14, 12:41 AM, Tom Lane wrote:
>> Meh. I don't think that extension authors are really going to appreciate
>> changing from "thou shalt declare all thy functions" to "thou shalt
>> declare none of them".

> This patch does no such thing.

Yes it does; people who fail to remove their manual externs will get
Windows-only build failures (or at least warnings; it's not very clear
which declaration will win).

regards, tom lane

On 2/15/14, 10:22 AM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> On 1/15/14, 12:41 AM, Tom Lane wrote:
>>> Meh. I don't think that extension authors are really going to appreciate
>>> changing from "thou shalt declare all thy functions" to "thou shalt
>>> declare none of them".
>
>> This patch does no such thing.
>
> Yes it does; people who fail to remove their manual externs will get
> Windows-only build failures (or at least warnings; it's not very clear
> which declaration will win).

The manual externs and the automatically provided ones are exactly the
same. Why would that fail?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 2/15/14, 10:22 AM, Tom Lane wrote:
>> Yes it does; people who fail to remove their manual externs will get
>> Windows-only build failures (or at least warnings; it's not very clear
>> which declaration will win).

> The manual externs and the automatically provided ones are exactly the
> same. Why would that fail?

Maybe I'm remembering the wrong patch. I thought what this patch was
intending was to put PGDLLEXPORT into the automatically-provided externs.

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > On 2/15/14, 10:22 AM, Tom Lane wrote:
> >> Yes it does; people who fail to remove their manual externs will get
> >> Windows-only build failures (or at least warnings; it's not very clear
> >> which declaration will win).
>
> > The manual externs and the automatically provided ones are exactly the
> > same. Why would that fail?
>
> Maybe I'm remembering the wrong patch. I thought what this patch was
> intending was to put PGDLLEXPORT into the automatically-provided externs.

This hunk is the essence of this patch:

#define PG_FUNCTION_INFO_V1(funcname) \
+Datum funcname(PG_FUNCTION_ARGS); \
extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \

Note that PGDLLEXPORT is already there. This patch is just about
additionally providing the prototype.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-02-17 10:30:16 -0300, Alvaro Herrera wrote:
> Tom Lane wrote:
> > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > > On 2/15/14, 10:22 AM, Tom Lane wrote:
> > >> Yes it does; people who fail to remove their manual externs will get
> > >> Windows-only build failures (or at least warnings; it's not very clear
> > >> which declaration will win).
> >
> > > The manual externs and the automatically provided ones are exactly the
> > > same. Why would that fail?
> >
> > Maybe I'm remembering the wrong patch. I thought what this patch was
> > intending was to put PGDLLEXPORT into the automatically-provided externs.
>
> This hunk is the essence of this patch:
>
> #define PG_FUNCTION_INFO_V1(funcname) \
> +Datum funcname(PG_FUNCTION_ARGS); \
> extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
>
>
> Note that PGDLLEXPORT is already there. This patch is just about
> additionally providing the prototype.

The PGDLLEXPORT is attached to the variable, no the function tho. If
somebody previously tried to do the correct thing and attached
PGDLLEXPORT to their own *function* prototoype, it would cause problems
now.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 4/4/14, 10:07 AM, Andres Freund wrote:
> If
> somebody previously tried to do the correct thing and attached
> PGDLLEXPORT to their own *function* prototoype, it would cause problems
> now.

What is the difference (on affected platforms) between

Datum funcname(PG_FUNCTION_ARGS);

and writing (effectively)

PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS);
Datum funcname(PG_FUNCTION_ARGS);

or for that matter

Datum funcname(PG_FUNCTION_ARGS);
PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS);

If there isn't a difference, then my patch is fine. Otherwise, it might
be good to document the issues for extension authors.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> What is the difference (on affected platforms) between

> Datum funcname(PG_FUNCTION_ARGS);

> and writing (effectively)

> PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS);
> Datum funcname(PG_FUNCTION_ARGS);

> or for that matter

> Datum funcname(PG_FUNCTION_ARGS);
> PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS);

According to
http://www.postgresql.org/message-id/52D25AA2.50108@2ndquadrant.com

the latter fails outright. Which is problematic because that'd be the
more common case (particularly if you put manually-written externs in a
header file). So while we could do this, and it'd probably be an
improvement in the very long run, it'd definitely cause pain in the short
run. Not sure if it's worth it.

I still wish we could get rid of this problem by fixing the Windows build
recipes so that the PGDLLEXPORT marking wasn't needed. We proved to
ourselves recently that getting rid of PGDLLIMPORT on global variables
wouldn't work, but I'm not sure that the function end of it was really
investigated.

regards, tom lane

On 04/15/2014 03:39 AM, Tom Lane wrote:
> I still wish we could get rid of this problem by fixing the Windows build
> recipes so that the PGDLLEXPORT marking wasn't needed. We proved to
> ourselves recently that getting rid of PGDLLIMPORT on global variables
> wouldn't work, but I'm not sure that the function end of it was really
> investigated.

My understanding is that we *can* drop PGDLLEXPORT on functions without
actively breaking anything. But we probably shouldn't.

If we omit PGDLLEXPORT, the linker of the DLL/executable that imports
the extern function will generate a thunk from the .LIB file for the
target DLL during linkage; this thunk within the DLL/EXE with the
undefined extern then jumps to the real address within the defining DLL/EXE.

Reference: http://msdn.microsoft.com/en-us/library/zw3za17w.aspx

So in other words, it makes calls across DLL boundaries less efficient
by adding a layer of indirection. (No idea how this works in the
presence of link time base address randomization either).

I actually think we should *add* a LIBPQEXPORT that handles this for
libpq, much like PGDLLEXPORT does for postgres(.exe). And in the
process, rename PGDLLEXPORT to POSTGRESEXPORT or PGSERVEREXPORT or
something.

PGDLLEXPORT is probably less important overall - it'll mainly impact
extensions (like hstore, intarray, etc) that call into the server.

I wonder if this thunking still really mattres with modern CPU
architecures' smart branch prediction, TLB caches, etc. I haven't found
much info on the real world impact.

It would probably be reasonable to add PGDLLEXPORT within postgres.exe
only on functions we've intentionally exposed for use by extensions,
where those functions are likely to get called a lot and don't have
bigget costs like disk I/O, network I/O, expensive memory allocations,
etc, that make call time overheads irrelevant.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> On 04/15/2014 03:39 AM, Tom Lane wrote:
>> I still wish we could get rid of this problem by fixing the Windows build
>> recipes so that the PGDLLEXPORT marking wasn't needed. We proved to
>> ourselves recently that getting rid of PGDLLIMPORT on global variables
>> wouldn't work, but I'm not sure that the function end of it was really
>> investigated.

> My understanding is that we *can* drop PGDLLEXPORT on functions without
> actively breaking anything. But we probably shouldn't.

> If we omit PGDLLEXPORT, the linker of the DLL/executable that imports
> the extern function will generate a thunk from the .LIB file for the
> target DLL during linkage; this thunk within the DLL/EXE with the
> undefined extern then jumps to the real address within the defining DLL/EXE.

TBH, if the only argument for this is a small efficiency difference,
then to my mind it barely requires discussion. I don't give one hoot
about micro-optimization for the Windows platform; I'm satisfied if
it works at all there. And I seriously doubt that a couple more cycles to
call any function implemented in a loadable module would matter anyway.

> I actually think we should *add* a LIBPQEXPORT that handles this for
> libpq, much like PGDLLEXPORT does for postgres(.exe). And in the
> process, rename PGDLLEXPORT to POSTGRESEXPORT or PGSERVEREXPORT or
> something.

My reaction to that is "not bloody likely". I remarked on this upthread
already, but there is absolutely no way that I want to clutter our source
code with platform-specific markings like that.

Perhaps somebody could try a Windows build with PGDLLEXPORT defined to
empty, and verify that it works, and if so do a pgbench comparison
against a build done the existing way?

regards, tom lane

On 4/14/14, 3:28 PM, Peter Eisentraut wrote:
> On 4/4/14, 10:07 AM, Andres Freund wrote:
>> If
>> somebody previously tried to do the correct thing and attached
>> PGDLLEXPORT to their own *function* prototoype, it would cause problems
>> now.
>
> What is the difference (on affected platforms) between
>
> Datum funcname(PG_FUNCTION_ARGS);
>
> and writing (effectively)
>
> PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS);
> Datum funcname(PG_FUNCTION_ARGS);
>
> or for that matter
>
> Datum funcname(PG_FUNCTION_ARGS);
> PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS);
>
>
> If there isn't a difference, then my patch is fine. Otherwise, it might
> be good to document the issues for extension authors.

Let me point out again that my patch doesn't actually do anything about
PGDLLEXPORT or the like. It just adds automatic prototypes into
PG_FUNCTION_INFO_V1, to reduce compiler warnings in extensions and
reduce some boilerplate in general.

If it turns out that this might help someone optimize the Windows build,
then great. But I gather it won't, so so what. Or maybe it can be made
to work, in which case extension authors will get compiler errors about
places they need to clean up. So it could still help that way, but who
knows, that's not the point.

If there are still concerns in this area, we could commit just the part
that adds the prototype to PG_FUNCTION_INFO_V1 and let that sit for a
while, and then remove the (now redundant) explicit prototypes in a
later release, so if there is a need to revert it, it won't be so big.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Let me point out again that my patch doesn't actually do anything about
> PGDLLEXPORT or the like. It just adds automatic prototypes into
> PG_FUNCTION_INFO_V1, to reduce compiler warnings in extensions and
> reduce some boilerplate in general.

Hmm ... for some reason I had gotten it in my head that you were adding
PGDLLEXPORT to the autogenerated extern declarations, but at least the
version of the patch in <1389762012(dot)24046(dot)2(dot)camel(at)vanquo(dot)pezone(dot)net>
doesn't do that, so the point is moot.

I still object to the aspect of the patch that moves the externs for
_PG_init/_PG_fini into fmgr.h: that is conceptually wrong and will create
more confusion than the trivial code savings is worth. But I won't
complain if you commit the PG_FUNCTION_INFO_V1 changes.

regards, tom lane

On 04/15/2014 10:17 PM, Tom Lane wrote:
>> > I actually think we should *add* a LIBPQEXPORT that handles this for
>> > libpq, much like PGDLLEXPORT does for postgres(.exe). And in the
>> > process, rename PGDLLEXPORT to POSTGRESEXPORT or PGSERVEREXPORT or
>> > something.
> My reaction to that is "not bloody likely". I remarked on this upthread
> already, but there is absolutely no way that I want to clutter our source
> code with platform-specific markings like that.
>
> Perhaps somebody could try a Windows build with PGDLLEXPORT defined to
> empty, and verify that it works, and if so do a pgbench comparison
> against a build done the existing way?

Good idea.

Personally, I don't care about Windows enough. I want it to work, but
performance optimisation is beyond what I'm bothered with.

Another useful test would be to modify libpq as described above, so its
headers set __declspec(dllexport) on its exports during compilation and
its headers set __declspec(dllimport) when included while compiling
other binaries that will link to libpq. Then use *that* in pgbench too
and see if it makes any meaningful difference.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services