Thread: 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 discussedin the second thread, it still seems like a win for reducing boilerplate and accidental compiler warnings. So hereis a patch for consideration.
Attachment
Peter Eisentraut <peter_e@gmx.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 discussedin the second thread, it still seems like a win for reducing boilerplate and accidental compiler warnings. So hereis 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@gmx.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 discussedin the second thread, it still seems like a win for reducing boilerplate and accidental compiler warnings. So hereis 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@gmx.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@gmx.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@gmx.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@gmx.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); \ externPGDLLEXPORT 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@gmx.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@gmx.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@2ndquadrant.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@gmx.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.24046.2.camel@vanquo.pezone.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