Thread: libpq-events windows gotcha
Just noticed that the last libpqtypes release was broken on windows when
dynamically linking. The problem is that windows has two addresses for
functions, the import library uses a stub "ordinal" address while the
DLL itself is using the real address; yet another m$ annoyance. This
breaks the PQEventProc being used as a unique lookup value.
Be aware that there is nothing wrong with the libpq-events API. One
just has to be very careful on windows with DLLs.
libpqtypes fixed this issue by no longer publically exposing its
PQEventProc, didn't need to do this anyways. libpqtypes instanceData is
meant to be private. Version 1.2b conatins this fix and will be online
soon.
// this used to be a macro
int PQtypesRegister(PGconn *conn)
{ return PQregisterEventProc(conn, pqt_eventproc, "pqtypes", NULL);
}
// used to be a public function named PQtypesEventproc, now internal
int pqt_eventproc(PGEventId id, void *info, void *passThrough)
This is a big gotcha for any libpq-events implementors. It should
probably be documented in some fashion. Other implementations may want
to expose the PGEventProc so its API users can access the instanceData
... so they can get the lookup key. For this case, a public function
that returns the address of the private "static not dllexport"
PGEventProc would be required.
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
> Just noticed that the last libpqtypes release was broken on windows when
> dynamically linking. The problem is that windows has two addresses for
> functions, the import library uses a stub "ordinal" address while the
> DLL itself is using the real address; yet another m$ annoyance. This
> breaks the PQEventProc being used as a unique lookup value.
> This is a big gotcha for any libpq-events implementors. It should
> probably be documented in some fashion.
Hmm. Well, it's not too late to reconsider the use of the function
address as a lookup key ... but what else would we use instead?
regards, tom lane
Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: >> Just noticed that the last libpqtypes release was broken on windows when >> dynamically linking. The problem is that windows has two addresses for >> functions, the import library uses a stub "ordinal" address while the >> DLL itself is using the real address; yet another m$ annoyance. This >> breaks the PQEventProc being used as a unique lookup value. >> This is a big gotcha for any libpq-events implementors. It should >> probably be documented in some fashion. > > Hmm. Well, it's not too late to reconsider the use of the function > address as a lookup key ... but what else would we use instead? > > regards, tom lane > > Here are the options we see: 1. PQregisterEventProc returns a handle, synchronized counter incremented by libpq. A small table could map handle value to proc address, so register always returns the same handle for a provided eventproc. Only register would take an eventproc, instanceData functions would take this magical handle. 2. string key, has collision issues (already been ruled out) 3. have implementors return a static variable address (doesn't seem all that different from returning a static funcaddr) 4. what we do now, but document loudly that PGEventProc must be static. If it can't be referenced outside the DLL directlythen the issue can't arise. If you need the function address for calls to PQinstanceData, an implementor can create a public function that returns their private PGEventProc address. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow <ac@esilo.com> writes:
>> Here are the options we see:
>>
>> 1. PQregisterEventProc returns a handle, synchronized counter
>> incremented by libpq. A small table could map handle value to proc
>> address, so register always returns the same handle for a provided
>> eventproc. Only register would take an eventproc, instanceData
>> functions would take this magical handle.
>>
>> 2. string key, has collision issues (already been ruled out)
>>
>> 3. have implementors return a static variable address (doesn't seem all
>> that different from returning a static funcaddr)
>>
>> 4. what we do now, but document loudly that PGEventProc must be static.
>> If it can't be referenced outside the DLL directly then the issue can't
>> arise. If you need the function address for calls to PQinstanceData, an
>> implementor can create a public function that returns their private
>> PGEventProc address.
> Do you have a preference form the list above, or an another idea? If
> not, we would probably implement #1. Although, the simplest thing is #4
> which leaves everything as is and updates the sgml docs with a strong
> warning to implementors.
I think #1 is mostly going to complicate life for both libpq and callers
--- libpq has to deal with generating the handles (threading issues
here) and callers have to remember them somewhere. And it's not even
clear to me that it fixes the problem: wouldn't you get two different
handles if you supplied the internal and external addresses of an
eventproc?
On the whole I vote for #4 out of these. I was just wondering whether
there were any better alternatives, and it seems there's not.
regards, tom lane
Andrew Chernow wrote: > Tom Lane wrote: >> Andrew Chernow <ac@esilo.com> writes: >>> Just noticed that the last libpqtypes release was broken on windows >>> when dynamically linking. The problem is that windows has two >>> addresses for functions, the import library uses a stub "ordinal" >>> address while the DLL itself is using the real address; yet another >>> m$ annoyance. This breaks the PQEventProc being used as a unique >>> lookup value. >>> This is a big gotcha for any libpq-events implementors. It should >>> probably be documented in some fashion. >> >> Hmm. Well, it's not too late to reconsider the use of the function >> address as a lookup key ... but what else would we use instead? >> >> regards, tom lane >> >> > > Here are the options we see: > > 1. PQregisterEventProc returns a handle, synchronized counter > incremented by libpq. A small table could map handle value to proc > address, so register always returns the same handle for a provided > eventproc. Only register would take an eventproc, instanceData > functions would take this magical handle. > > 2. string key, has collision issues (already been ruled out) > > 3. have implementors return a static variable address (doesn't seem all > that different from returning a static funcaddr) > > 4. what we do now, but document loudly that PGEventProc must be static. > If it can't be referenced outside the DLL directly then the issue can't > arise. If you need the function address for calls to PQinstanceData, an > implementor can create a public function that returns their private > PGEventProc address. > Do you have a preference form the list above, or an another idea? If not, we would probably implement #1. Although, the simplest thing is #4 which leaves everything as is and updates the sgml docs with a strong warning to implementors. I am not sure which is best. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Tom Lane wrote: > > And it's not even > clear to me that it fixes the problem: wouldn't you get two different > handles if you supplied the internal and external addresses of an > eventproc? > Both #1 and #4 suffer from this issue, internal & external register methods. They also require the same WARNING in the docs. But, #1 solves the instancedata lookup issue. I am not trying to make a case for #1 but it does appear to have a narrower failure window. > libpq has to deal with generating the handles Well lock; if(not_in_map) handle = ++counter; unlock surely won't be to difficult ;-) > (threading issues here) There is no unregister, so the idea won't lock/unlock in high traffic routines. > On the whole I vote for #4 out of these. > Okay. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
>>> >>> 4. what we do now, but document loudly that PGEventProc must be static. >>> If it can't be referenced outside the DLL directly then the issue can't >>> arise. If you need the function address for calls to PQinstanceData, an >>> implementor can create a public function that returns their private >>> PGEventProc address. > >> Do you have a preference form the list above, or an another idea? If >> not, we would probably implement #1. Although, the simplest thing is #4 >> which leaves everything as is and updates the sgml docs with a strong >> warning to implementors. > > On the whole I vote for #4 out of these. > I attached a patch for the docs. Its documented as a NOTE to the PGEventProc. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: doc/src/sgml/libpq.sgml =================================================================== RCS file: /projects/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.269 diff -C6 -r1.269 libpq.sgml *** doc/src/sgml/libpq.sgml 13 Nov 2008 09:45:24 -0000 1.269 --- doc/src/sgml/libpq.sgml 14 Nov 2008 12:08:07 -0000 *************** *** 5252,5263 **** --- 5252,5275 ---- <para> A particular event procedure can be registered only once in any <structname>PGconn</>. This is because the address of the procedure is used as a lookup key to identify the associated instance data. </para> + + <note> + <para> + On windows, functions can have two different addresses: one accessed + from outside a DLL, obtained from the import library, and another from + inside a DLL. For this reason, implementors should never directly expose + an event procedure. If the event procedure is needed by an API user, + then its address should be returned by a library function; ie. + <literal>PGEventProc MyGetEventProc(void);</literal>. This ensures that + the application and DLL are always using the same address. + </para> + </note> </listitem> </varlistentry> </variablelist> </sect2> <sect2 id="libpq-events-funcs">
Andrew Chernow <ac@esilo.com> writes:
>> On the whole I vote for #4 out of these.
> I attached a patch for the docs. Its documented as a NOTE to the
> PGEventProc.
Applied, but I editorialized on the wording a bit. Let me know if you
think this is wrong ...
regards, tom lane
Index: libpq.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.269
diff -c -r1.269 libpq.sgml
*** libpq.sgml 13 Nov 2008 09:45:24 -0000 1.269
--- libpq.sgml 14 Nov 2008 22:57:05 -0000
***************
*** 5255,5260 ****
--- 5255,5273 ---- <structname>PGconn</>. This is because the address of the procedure is used as a
lookupkey to identify the associated instance data. </para>
+
+ <caution>
+ <para>
+ On Windows, functions can have two different addresses: one visible
+ from outside a DLL and another visible from inside the DLL. One
+ should be careful that only one of these addresses is used with
+ <application>libpq</>'s event-procedure functions, else confusion will
+ result. The simplest rule for writing code that will work is to
+ ensure that event procedures are declared <literal>static</>. If the
+ procedure's address must be available outside its own source file,
+ expose a separate function to return the address.
+ </para>
+ </caution> </listitem> </varlistentry> </variablelist>
Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: >>> On the whole I vote for #4 out of these. > >> I attached a patch for the docs. Its documented as a NOTE to the >> PGEventProc. > > Applied, but I editorialized on the wording a bit. Let me know if you > think this is wrong ... > > It's correct, the wording is clearer now. I wasn't aware there was a caution tag, which is better than using <note>. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/