Re: [PATCHES] libpq patch for pqtypes hook api and PGresult creation
Kind of a long post, but if you take the time to read it we think it accurately
clarifies how we interrupt the current objections and how we see this working.
NOTE: any references to overhead are in regards to library size, not performance.
would be to insert hooks at library
_init() time, meaning that the mere linking of libpgtypes
Alvaro Herrera wrote:
Maybe there's a way we can have libpqtypes adding calls into some
hypothetical libpq hooks. So libpqtypes registers its hooks in _init()
or some such, and it gets picked up automatically by any app that links
to it.
the hook name concept
Not needed anymore if we do per-conn hooks. I was doing library wide hooks, it
felt natural to allow them to be removed (ability not needed per-conn). You can
only remove hooks if you have a means of referencing what you want to remove.
From that perspective, the names served a purpose - PQremoveObjectHooks(myhook);
you've got it holding a process-wide lock
Okay, easy change to install per-conn. I was trying to avoid having to set these
hooks on every connection.
There are some dirty details in regards to locking. Even if you remove the
locking from libpq hooks, you still incur a lock at every hook point inside
pqtypes. pqtypes has to map a conn and result (we'll call this a pqobj) to
pqtypes typeData. Adding a void* to the hook funcs doesn't help because non-hook
functions like getf, paramcreate, etc. only have a ptr to a pqobj: PQgetf(res,
..), PQparamCreate(conn, ..). Since the private storage of a pqobj is not
directly accessible, you have to either A) map pqobj addresses to typeData in a
pqtypes global array that must be locked or B) add two libpq API calls
PQtypeData(conn), PQresultTypeData(res).
libpgtypes calls PQinitTypes(PGconn *conn)
As this stands, it wouldn't work. You need some hook funcptr arguments. Without
them, there is no way to communicate with pqtypes.
Tom Lane wrote:
hooks that could be used by either libpgtypes or something that would like to
do something roughly similar
I don't think PQinitTypes, private type data per conn/result or the need for
PQtypeData(conn), PQresultTypeData(res) (to avoid locking in pqtypes) keeps
things in line with this requirement (generic hook api). Has this requirement
changed? BTW, Tom was not the only one to suggest generic design. That's why I
came up with object hooks - notifications of libpq object states. Best
name/concept I can come up with. PQinitTypes(conn) is really
PQaddObjectHook(conn, hook) -- addition of the conn argument -- to keep it generic.
In the end, the problem is that the wrong tool hooks is being used for
pqtypes. Hooks normally allow a completely unrelated library to receive events.
I think we are forcing a hook design on to something that is completely
related (libpqtypes is an extension of libpq, getvalue and getf are siblings).
There is no need for hooks. If we wanted to add
PQsetBillingMethod(PQconn*,PQbillingMethod*), then you could make a case for
hooks (obviously the billing api doesn't fit). But that is not the case for
PQgetf, PQputf, PQparamExec, PQparamSend,
The argument against pqtypes being part of libpq was library size overhead.
This was verbalized by many people (issues with redhat packaging were also
brought up). I never heard a complaint about the 10 API calls we wanted to add.
Only that those 10 API calls came at a 50K library bloat cost, and there were
no buyers.
This brings me back to the dlopen idea. If you want to use pqtypes,
PQtypesLoad(). The guts of the library are in libpqtypes.so so the only thing
left in libpq are simple functions like below:
// libpq
PQparamExec(...)
{
if(libpqtypes-paramExec)//typesLoad issued dlsym calls
// we are in libpq, access to private conn storage granted
return libpqtypes-paramExec(conn, conn-typeData, ...);
return library not loaded: call PQtypesLoad;
}
// end user
#include libpqtypes.h // pqtypes typedefs, includes libpq-fe.h
PQtypesLoad(); // call before using libpq
res = PQparamExec(conn, param, .);
The library size issue is resolved. I never heard any complaints about this
approach. Andrew Dunstan said Please make sure that any scheme you have along
these lines will work on Windows DLLs too., which didn't sound like a complaint
to me.
#ifdef WIN32
# define dlopen(libname, flags) LoadLibraryA(libname)
# define dlsym(handle, sym) GetProcAddress(handle, sym)
#endif
Tom also weighed in but he thought I was confused about his hook idea (as the
proposed dlopen is completely different):
Tom Wrote:
This is still 100% backwards. My idea of a libpq hook is something that
could be used by libpgtypes *and other things*. What you are proposing
is something where the entire API of the supposed add-on is hard-wired
into libpq.
He is 100% correct, the dlopen idea is 100% backwards from a hook concept. It
was not an implementation idea for the hooks concept, it was a different
approach
Re: [PATCHES] libpq patch for pqtypes hook api and PGresult creation
Merlin Moncure wrote: On Fri, Apr 11, 2008 at 1:47 PM, Andrew Chernow [EMAIL PROTECTED] wrote: Here are the changes to libpq. It adds the ability to register an Object Hook and create a home-grown result. Patch adds 4 functions. We changed the name of PQresultSetFieldValue to PQsetvalue, which better compliments PQgetvalue. If this patch is acceptable, we will move on to making the required changes to pqtypes; some work has already been done. Whoops! One missing thing here...we forgot to make pqResultAlloc pubilc...pqResultAlloc - PQresultAlloc (see discussion in -hackers). Also, we could use pqResultStrdup (or keep it private, in which case we can re-implement in libpqtypes). merlin The connCreate and resultCreate hooks are in the wrong place. I didn't realize this until I started to implement the hook functions in pqtypes. void (*connCreate)(const char *hookName, const PGconn *conn); The requirements for the connCreate hook are that the PGconn is ready for use. I am currently hooking in connectDBStart, which is dead wrong. After some poking around, it looks like the correct place to hook would be in PQconnectPoll ... does this sound correct? There are 3 places PQconnectPoll returns PGRES_POLLING_OK: one is at the top of the function and the other two are further down with We are open for business! comments. Would I be correct to hook in at these 3 places in PQconnectPoll? void (*resultCreate)(const char *hookName, const PGconn *conn, const PGresult *res); The requirements for resultCreate are that the result is fully constructed. I am currently hooked in PQmakeEmptyResult, again dead wrong. Does PQgetResult sound correct? Also, pqtypes is only interested in CONNECTION_OK and successfull results. But, these hooks are available to more than just pqtypes. What should the when to call connCreate and resultCreate policy be? Should the hook only be called when the conn or result was successfull or should the hooks be called for failed connections/commands as well? -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq patch for pqtypes hook api and PGresult creation
Should the hook only be called when the conn or result was
successfull or should the hooks be called for failed
connections/commands as well?
ISTM that the hooks should be called on success and error (doesn't
include cases where a NULL conn or result is returned). I think this
makes things more useful. If the hooker (pun intended) isn't interested
in error results or conns, it can easily ignore them.
connCreate: The best solution I found was to hook into PQconnectPoll.
This required making the current PQconnectPoll a static named
connectPoll and making PQconnectPoll a wrapper to it. The wrapper just
calls connectPoll and checks the status for hook points.
resultCreate: PQgetResult seemed like the best place. I only notify the
hooks if the resultStatus is NOT copyin or copyout.
I diff'd fe-connect.c and fe-exec.c against cvs which shows these changes.
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Index: fe-connect.c
===
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.357
diff -C6 -r1.357 fe-connect.c
*** fe-connect.c31 Mar 2008 02:43:14 - 1.357
--- fe-connect.c12 Apr 2008 13:22:30 -
***
*** 240,252
static int parseServiceInfo(PQconninfoOption *options,
PQExpBuffer errorMessage);
static char *pwdfMatchesString(char *buf, char *token);
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
char *username);
static void default_threadlock(int acquire);
!
/* global variable because fe-auth.c needs to access it */
pgthreadlock_t pg_g_threadlock = default_threadlock;
/*
--- 240,252
static int parseServiceInfo(PQconninfoOption *options,
PQExpBuffer errorMessage);
static char *pwdfMatchesString(char *buf, char *token);
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
char *username);
static void default_threadlock(int acquire);
! static void notifyConnCreateHooks(PGconn *conn);
/* global variable because fe-auth.c needs to access it */
pgthreadlock_t pg_g_threadlock = default_threadlock;
/*
***
*** 891,903
--- 891,907
connectDBComplete(PGconn *conn)
{
PostgresPollingStatusType flag = PGRES_POLLING_WRITING;
time_t finish_time = ((time_t) -1);
if (conn == NULL || conn-status == CONNECTION_BAD)
+ {
+ if(conn)
+ notifyConnCreateHooks(conn);
return 0;
+ }
/*
* Set up a time limit, if connect_timeout isn't zero.
*/
if (conn-connect_timeout != NULL)
{
***
*** 979,992
* o If your backend wants to use Kerberos authentication then you
must
*supply both a host name and a host address, otherwise this
function
*may block on gethostname.
*
*
*/
! PostgresPollingStatusType
! PQconnectPoll(PGconn *conn)
{
PGresult *res;
charsebuf[256];
if (conn == NULL)
return PGRES_POLLING_FAILED;
--- 983,997
* o If your backend wants to use Kerberos authentication then you
must
*supply both a host name and a host address, otherwise this
function
*may block on gethostname.
*
*
*/
!
! static PostgresPollingStatusType
! connectPoll(PGconn *conn)
{
PGresult *res;
charsebuf[256];
if (conn == NULL)
return PGRES_POLLING_FAILED;
***
*** 1875,1886
--- 1880,1908
* the connection structure must be freed).
*/
conn-status = CONNECTION_BAD;
return PGRES_POLLING_FAILED;
}
+ /* See connectPoll. All this does is wrap the real PQconnectPoll so
+ * we can trap PGRES_POLLING_OK or PGRES_POLLING_FAILED statuses. This
+ * could be done in the real connectPoll, but that requires littering the
+ * function with notifyConnCreateHooks calls because there are many
+ * places the function returns a status. This solution is much less
+ * obtrusive and avoids messing with the black magic of connectPoll.
+ */
+ PostgresPollingStatusType
+ PQconnectPoll(PGconn *conn)
+ {
+ PostgresPollingStatusType status = connectPoll(conn);
+
+ if(status == PGRES_POLLING_OK || status == PGRES_POLLING_FAILED)
+ notifyConnCreateHooks(conn);
+
+ return status;
+ }
/*
* makeEmptyPGconn
* - create a PGconn data structure with (as yet) no interesting data
*/
static PGconn *
***
*** 1970,1981
--- 1992,2015
* release data that is to be held for the life of the PGconn structure.
* If a
Re: [PATCHES] libpq patch for pqtypes hook api and PGresult creation
Andrew Chernow [EMAIL PROTECTED] writes: The requirements for the connCreate hook are that the PGconn is ready for use. I am currently hooking in connectDBStart, which is dead wrong. I looked at the object hooks patch and it looked like a complete mess. AFAICS the only way you could use it would be to insert hooks at library _init() time, meaning that the mere linking of libpgtypes into an executable would cause all your hook overhead to occur on every connection and every query ever made by that program. The thread locking you put in is completely horrid as well --- you've got it holding a process-wide lock over operations that are likely to include nontrivial database interactions. I think you need to consider something a bit less invasive. What I'd imagine is something more like this: a program that wishes to use libpgtypes calls PQinitTypes(PGconn *conn) immediately after establishing a connection, and that installs hooks into connection-local storage and does whatever per-connection setup it needs. No need for any global state nor any thread mutexes. Lastly, as far as the hook designs themselves: the hook name concept seems utterly useless, and what *is* needed is missing: every callback function needs a pass-through void * pointer so that it can get at private state. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq patch for pqtypes hook api and PGresult creation
On Fri, Apr 11, 2008 at 1:47 PM, Andrew Chernow [EMAIL PROTECTED] wrote: Here are the changes to libpq. It adds the ability to register an Object Hook and create a home-grown result. Patch adds 4 functions. We changed the name of PQresultSetFieldValue to PQsetvalue, which better compliments PQgetvalue. If this patch is acceptable, we will move on to making the required changes to pqtypes; some work has already been done. Whoops! One missing thing here...we forgot to make pqResultAlloc pubilc...pqResultAlloc - PQresultAlloc (see discussion in -hackers). Also, we could use pqResultStrdup (or keep it private, in which case we can re-implement in libpqtypes). merlin -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
