Thread: libpq patch for pqtypes hook api and PGresult creation
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. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: src/interfaces/libpq/exports.txt =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.19 diff -c -r1.19 exports.txt *** src/interfaces/libpq/exports.txt 19 Mar 2008 00:39:33 -0000 1.19 --- src/interfaces/libpq/exports.txt 11 Apr 2008 17:36:26 -0000 *************** *** 141,143 **** --- 141,147 ---- pg_valid_server_encoding_id 139 PQconnectionNeedsPassword 140 lo_import_with_oid 141 + PQaddObjectHook 142 + PQremoveObjectHook 143 + PQmakeResult 144 + PQsetvalue 145 \ No newline at end of file Index: src/interfaces/libpq/fe-connect.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.357 diff -c -r1.357 fe-connect.c *** src/interfaces/libpq/fe-connect.c 31 Mar 2008 02:43:14 -0000 1.357 --- src/interfaces/libpq/fe-connect.c 11 Apr 2008 17:36:26 -0000 *************** *** 866,872 **** --- 866,887 ---- * we are in PGRES_POLLING_WRITING connection state. */ if (PQconnectPoll(conn) == PGRES_POLLING_WRITING) + { + int objHooksCount; + const PGobjectHooks *objHooks; + + if((objHooksCount = pqGetObjectHooks(&objHooks)) > 0) + { + int i; + + for(i=0; i < objHooksCount; i++) + if(objHooks[i].connCreate) + objHooks[i].connCreate(objHooks[i].name, conn); + } + pqReleaseObjectHooks(); + return 1; + } connect_errReturn: if (conn->sock >= 0) *************** *** 1973,1978 **** --- 1988,2006 ---- static void freePGconn(PGconn *conn) { + int objHooksCount; + const PGobjectHooks *objHooks; + + if((objHooksCount = pqGetObjectHooks(&objHooks)) > 0) + { + int i; + + for(i=0; i < objHooksCount; i++) + if(objHooks[i].connDestroy) + objHooks[i].connDestroy(objHooks[i].name, conn); + } + pqReleaseObjectHooks(); + if (conn->pghost) free(conn->pghost); if (conn->pghostaddr) Index: src/interfaces/libpq/fe-exec.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v retrieving revision 1.194 diff -c -r1.194 fe-exec.c *** src/interfaces/libpq/fe-exec.c 1 Jan 2008 19:46:00 -0000 1.194 --- src/interfaces/libpq/fe-exec.c 11 Apr 2008 17:36:27 -0000 *************** *** 63,69 **** static PGresult *PQexecFinish(PGconn *conn); static int PQsendDescribe(PGconn *conn, char desc_type, const char *desc_target); ! /* ---------------- * Space management for PGresult. --- 63,69 ---- static PGresult *PQexecFinish(PGconn *conn); static int PQsendDescribe(PGconn *conn, char desc_type, const char *desc_target); ! static int check_field_number(const PGresult *res, int field_num); /* ---------------- * Space management for PGresult. *************** *** 139,144 **** --- 139,146 ---- PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status) { PGresult *result; + int objHooksCount; + const PGobjectHooks *objHooks; result = (PGresult *) malloc(sizeof(PGresult)); if (!result) *************** *** 192,200 **** --- 194,341 ---- result->client_encoding = PG_SQL_ASCII; } + if((objHooksCount = pqGetObjectHooks(&objHooks)) > 0) + { + int i; + + for(i=0; i < objHooksCount; i++) + if(objHooks[i].resultCreate) + objHooks[i].resultCreate(objHooks[i].name, conn, result); + } + pqReleaseObjectHooks(); + return result; } + PGresult *PQmakeResult( + PGconn *conn, + int numAttributes, + PGresAttDesc *attDescs) + { + int i; + PGresult *res; + + if(numAttributes <= 0 || !attDescs) + return NULL; + + res = PQmakeEmptyPGresult(conn, PGRES_TUPLES_OK); + if(!res) + return NULL; + + res->attDescs = (PGresAttDesc *) + pqResultAlloc(res, numAttributes * sizeof(PGresAttDesc), TRUE); + + if(!res->attDescs) + { + PQclear(res); + return NULL; + } + + res->numAttributes = numAttributes; + memcpy(res->attDescs, attDescs, numAttributes * sizeof(PGresAttDesc)); + + /* resultalloc the attribute names. */ + res->binary = 1; + for(i=0; i < numAttributes; i++) + { + if(attDescs[i].name) + res->attDescs[i].name = pqResultStrdup(res, attDescs[i].name); + else + res->attDescs[i].name = res->null_field; + + if(!res->attDescs[i].name) + { + PQclear(res); + return NULL; + } + + /* Although deprecated, because results can have text+binary columns, + * its easy enough to deduce so set it for completeness. + */ + if(res->attDescs[i].format == 0) + res->binary = 0; + } + + return res; + } + + int PQsetvalue( + PGresult *res, + int tup_num, + int field_num, + char *value, + int len) + { + PGresAttValue *attval; + + if(!check_field_number(res, field_num)) + return FALSE; + + /* Invalid tup_num, must be <= ntups */ + if(tup_num > res->ntups) + return FALSE; + + /* need to grow the tuple table */ + if(res->ntups >= res->tupArrSize) + { + int n = res->tupArrSize ? (res->tupArrSize*3)/2 : 64; + PGresAttValue **tups = (PGresAttValue **) + (res->tuples ? realloc(res->tuples, n*sizeof(PGresAttValue *)) : + malloc(n*sizeof(PGresAttValue *))); + + if(!tups) + return FALSE; + + res->tuples = tups; + res->tupArrSize = n; + } + + /* new to allocate a new tuple */ + if(tup_num == res->ntups && !res->tuples[tup_num]) + { + int i; + PGresAttValue *tup = (PGresAttValue *)pqResultAlloc( + res, res->numAttributes * sizeof(PGresAttValue), TRUE); + + if(!tup) + return FALSE; + + /* initialize each column to NULL */ + for(i=0; i < res->numAttributes; i++) + { + tup[i].len = NULL_LEN; + tup[i].value = res->null_field; + } + + res->tuples[tup_num] = tup; + res->ntups++; + } + + attval = &res->tuples[tup_num][field_num]; + + /* On top of NULL_LEN, treat a NULL value as a NULL field */ + if(len == NULL_LEN || value == NULL) + { + attval->len = NULL_LEN; + attval->value = res->null_field; + } + else + { + if(len < 0) + len = 0; + + attval->value = (char *)pqResultAlloc(res, len + 1, TRUE); + if(!attval->value) + return FALSE; + + attval->len = len; + memcpy(attval->value, value, len); + attval->value[len] = '\0'; + } + + return TRUE; + } + /* * pqResultAlloc - * Allocate subsidiary storage for a PGresult. *************** *** 353,362 **** --- 494,515 ---- PQclear(PGresult *res) { PGresult_data *block; + int objHooksCount; + const PGobjectHooks *objHooks; if (!res) return; + if((objHooksCount = pqGetObjectHooks(&objHooks)) > 0) + { + int i; + + for(i=0; i < objHooksCount; i++) + if(objHooks[i].resultDestroy) + objHooks[i].resultDestroy(objHooks[i].name, res); + } + pqReleaseObjectHooks(); + /* Free all the subsidiary blocks */ while ((block = res->curBlock) != NULL) { Index: src/interfaces/libpq/fe-misc.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-misc.c,v retrieving revision 1.133 diff -c -r1.133 fe-misc.c *** src/interfaces/libpq/fe-misc.c 1 Jan 2008 19:46:00 -0000 1.133 --- src/interfaces/libpq/fe-misc.c 11 Apr 2008 17:36:27 -0000 *************** *** 1155,1157 **** --- 1155,1332 ---- } #endif /* ENABLE_NLS */ + + + /* --------------------- + * ObjectHooks + */ + + #ifdef ENABLE_THREAD_SAFETY + # ifdef WIN32 + # include "pthread-win32.h" + static pthread_mutex_t objHookMutex = NULL; + # else + # include <pthread.h> + static pthread_mutex_t objHookMutex = PTHREAD_MUTEX_INITIALIZER; + # endif + #endif + + /* Object Hook array management variables. The hooks array is + * dynamically grown as needed. When removing an element, the + * element is memmove'd out. + */ + static int objHooksCount = 0; + static int objHooksArrSize = 0; + static PGobjectHooks *objHooks = NULL; + + int PQaddObjectHook(PGobjectHooks *hook, char **errMsg) + { + int i; + + if(!hook) + { + if(errMsg) + *errMsg = "ObjectHook pointer cannot be NULL"; + return FALSE; + } + + /* hook names must be provided */ + if(!hook->name || !*hook->name) + { + if(errMsg) + *errMsg = "ObjectHook name cannot be NULL or an empty string"; + return FALSE; + } + + /* At least one hook func must be provided (pointless otherwise) */ + if(!hook->connCreate && !hook->connDestroy && + !hook->resultCreate && !hook->resultDestroy) + { + if(errMsg) + *errMsg = "At least one ObjectHook function must be provided"; + return FALSE; + } + + (void)pqGetObjectHooks(NULL); + + /* hook names must be unique, case ignored. */ + for(i=0; i < objHooksCount; i++) + { + if(pg_strcasecmp(objHooks[i].name, hook->name)==0) + { + pqReleaseObjectHooks(); + if(errMsg) + *errMsg = "ObjectHook name already exists"; + return FALSE; + } + } + + /* grow objHooks array */ + if(objHooksCount == objHooksArrSize) + { + int n = objHooksArrSize ? (objHooksArrSize*3)/2 : 4; + + objHooks = (PGobjectHooks *)malloc(sizeof(PGobjectHooks) * n); + if(!objHooks) + { + if(errMsg) + *errMsg = "Out of memory error"; + return FALSE; + } + + objHooksArrSize = n; + } + + memcpy(objHooks + objHooksCount, hook, sizeof(PGobjectHooks)); + objHooks[objHooksCount++].name = strdup(hook->name); + + pqReleaseObjectHooks(); + + if(errMsg) + *errMsg = ""; + return TRUE; + } + + /* Removes a hook from the global hooks array. */ + void PQremoveObjectHook(const char *name) + { + int i; + + if(!name || !*name) + return; + + (void)pqGetObjectHooks(NULL); + + for(i=0; i < objHooksCount; i++) + { + if(pg_strcasecmp(objHooks[i].name, name)==0) + { + free((void *)objHooks[i].name); + memmove(objHooks + i, objHooks + i + 1, + (objHooksCount - i - 1) * sizeof(PGobjectHooks)); + objHooksCount--; + break; + } + } + + pqReleaseObjectHooks(); + } + + /* Gets the object hooks array, storing the array at *hooks and returning + * the number of elements in the array. The hooks array should be + * treated as read-only memory. + * + * When ENABLE_THREAD_SAFETY is set, this will lock a private mutex + * before returning. To unlock the hooks, call pqReleaseObjectHooks. + * Warning, you must call pqReleaseObjectHooks for each call to + * pqGetObjectHooks. + * + * const PGobjectHooks *hooks; + * int count = pqGetObjectHooks(&hooks); + * // ... do some work with hooks + * pqReleaseObjectHooks(); + * + * If the provided hooks argument is NULL, the internal mutex is still + * locked and the function returns the number of hooks registered. + * You still have to call pqReleaseObjectHooks. + * + * If there are no hooks registered, *hooks will be NULL and the + * function returns 0. + */ + int pqGetObjectHooks(const PGobjectHooks **hooks) + { + #ifdef ENABLE_THREAD_SAFETY + # ifdef WIN32 + static long initlock = 0; + + /* copied from init_ssl_system */ + if(objHookMutex == NULL) + { + while (InterlockedExchange(&initlock, 1) == 1) + /* loop, another thread own the lock */ ; + if (objHookMutex == NULL) + pthread_mutex_init(&objHookMutex, NULL); + InterlockedExchange(&initlock, 0); + } + # endif + + pthread_mutex_lock(&objHookMutex); + #endif + + if(hooks) + *hooks = (const PGobjectHooks *)objHooks; + return objHooksCount; + } + + /* Must be called after pqGetObjectHooks, so the mutex can be unlocked + * when ENABLE_THREAD_SAFETY is set. + */ + void pqReleaseObjectHooks(void) + { + #ifdef ENABLE_THREAD_SAFETY + pthread_mutex_unlock(&objHookMutex); + #endif + } + + + Index: src/interfaces/libpq/libpq-fe.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v retrieving revision 1.142 diff -c -r1.142 libpq-fe.h *** src/interfaces/libpq/libpq-fe.h 19 Mar 2008 00:39:33 -0000 1.142 --- src/interfaces/libpq/libpq-fe.h 11 Apr 2008 17:36:27 -0000 *************** *** 193,198 **** --- 193,236 ---- } PQArgBlock; /* ---------------- + * PGresAttDesc -- Data about a single attribute (column) of a query result + * ---------------- + */ + typedef struct pgresAttDesc + { + char *name; /* column name */ + Oid tableid; /* source table, if known */ + int columnid; /* source column, if known */ + int format; /* format code for value (text/binary) */ + Oid typid; /* type id */ + int typlen; /* type size */ + int atttypmod; /* type-specific modifier info */ + } PGresAttDesc; + + /* ---------------- + * PGobjectHooks -- structure for adding libpq object hooks + * Monitors the creation and deletion of objects. + * ---------------- + */ + + typedef struct + { + const char *name; + + /* Invoked on PQconnectdb, PQsetdbLogin, PQconnectStart or PQreset */ + int (*connCreate)(const char *hookName, PGconn *conn); + + /* Invoked on PQfinish. */ + int (*connDestroy)(const char *hookName, PGconn *conn); + + /* Invoked when PQmakeEmptyResult is called */ + int (*resultCreate)(const char *hookName, PGconn *conn, PGresult *result); + + /* Invoked when PQclear is called */ + int (*resultDestroy)(const char *hookName, PGresult *result); + } PGobjectHooks; + + /* ---------------- * Exported functions of libpq * ---------------- */ *************** *** 437,442 **** --- 475,500 ---- */ extern PGresult *PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status); + /* + * Makes a new result using the provided field descriptors. If conn is + * not NULL, some information will be copied to the new result. + * The returned result has zero tuples and a resultStatus of PGRES_TUPLES_OK. + * To add tuples and set field values, see PQsetvalue. + */ + extern PGresult *PQmakeResult(PGconn *conn, int numAttributes, + PGresAttDesc *attDescs); + + /* + * Sets the value for a tuple field. The tup_num must be less than or + * equal to PQntuples(res). This function will generate tuples as needed. + * A new tuple is generated when tup_num equals PQntuples(res) and there + * are no fields defined for that tuple. + * + * Returns a non-zero value for success and zero for failure. + */ + extern int PQsetvalue(PGresult *res, int tup_num, int field_num, + char *value, int len); + /* Quoting strings before inclusion in queries. */ extern size_t PQescapeStringConn(PGconn *conn, *************** *** 509,514 **** --- 567,581 ---- /* Get encoding id from environment variable PGCLIENTENCODING */ extern int PQenv2encoding(void); + /* Add a libpq global Object Hook. If errMsg is not NULL, it will + * be set to an error message if the function fails. This returns + * non-zero on success and zero on failure. + */ + extern int PQaddObjectHook(PGobjectHooks *hook, char **errMsg); + + /* Removes an Object Hook previously added with PQaddObjectHook */ + extern void PQremoveObjectHook(const char *name); + /* === in fe-auth.c === */ extern char *PQencryptPassword(const char *passwd, const char *user); Index: src/interfaces/libpq/libpq-int.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-int.h,v retrieving revision 1.129 diff -c -r1.129 libpq-int.h *** src/interfaces/libpq/libpq-int.h 1 Jan 2008 19:46:00 -0000 1.129 --- src/interfaces/libpq/libpq-int.h 11 Apr 2008 17:36:27 -0000 *************** *** 100,118 **** char space[1]; /* dummy for accessing block as bytes */ }; - /* Data about a single attribute (column) of a query result */ - - typedef struct pgresAttDesc - { - char *name; /* column name */ - Oid tableid; /* source table, if known */ - int columnid; /* source column, if known */ - int format; /* format code for value (text/binary) */ - Oid typid; /* type id */ - int typlen; /* type size */ - int atttypmod; /* type-specific modifier info */ - } PGresAttDesc; - /* Data about a single parameter of a prepared statement */ typedef struct pgresParamDesc { --- 100,105 ---- *************** *** 205,210 **** --- 192,198 ---- int spaceLeft; /* number of free bytes remaining in block */ }; + /* PGAsyncStatusType defines the state of the query-execution state machine */ typedef enum { *************** *** 524,529 **** --- 512,522 ---- extern int pqReadReady(PGconn *conn); extern int pqWriteReady(PGconn *conn); + /* === in objhooks.c === */ + + extern int pqGetObjectHooks(const PGobjectHooks **hooks); + extern void pqReleaseObjectHooks(void); + /* === in fe-secure.c === */ extern int pqsecure_initialize(PGconn *);
On Fri, Apr 11, 2008 at 1:47 PM, Andrew Chernow <ac@esilo.com> 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
Merlin Moncure wrote: > On Fri, Apr 11, 2008 at 1:47 PM, Andrew Chernow <ac@esilo.com> 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/
> > 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.c 31 Mar 2008 02:43:14 -0000 1.357 --- fe-connect.c 12 Apr 2008 13:22:30 -0000 *************** *** 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; char sebuf[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; char sebuf[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 value ought to be cleared/freed during PQreset(), do it there not here. */ static void freePGconn(PGconn *conn) { + int objHooksCount; + const PGobjectHooks *objHooks; + + if((objHooksCount = pqGetObjectHooks(&objHooks)) > 0) + { + int i; + + for(i=0; i < objHooksCount; i++) + if(objHooks[i].connDestroy) + objHooks[i].connDestroy(objHooks[i].name, (const PGconn *)conn); + } + pqReleaseObjectHooks(); if (conn->pghost) free(conn->pghost); if (conn->pghostaddr) free(conn->pghostaddr); if (conn->pgport) free(conn->pgport); *************** *** 3843,3854 **** --- 3877,3904 ---- pthread_mutex_lock(&singlethread_lock); else pthread_mutex_unlock(&singlethread_lock); #endif } + /* Calls the connCreate hook. */ + static void notifyConnCreateHooks(PGconn *conn) + { + int objHooksCount; + const PGobjectHooks *objHooks; + + if((objHooksCount = pqGetObjectHooks(&objHooks)) > 0) + { + int i; + for(i=0; i < objHooksCount; i++) + if(objHooks[i].connCreate) + objHooks[i].connCreate(objHooks[i].name, (const PGconn *)conn); + } + + pqReleaseObjectHooks(); + } pgthreadlock_t PQregisterThreadLock(pgthreadlock_t newhandler) { pgthreadlock_t prev = pg_g_threadlock; if (newhandler) Index: fe-exec.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v retrieving revision 1.194 diff -C6 -r1.194 fe-exec.c *** fe-exec.c 1 Jan 2008 19:46:00 -0000 1.194 --- fe-exec.c 12 Apr 2008 13:22:31 -0000 *************** *** 60,72 **** int resultFormat); static void parseInput(PGconn *conn); static bool PQexecStart(PGconn *conn); static PGresult *PQexecFinish(PGconn *conn); static int PQsendDescribe(PGconn *conn, char desc_type, const char *desc_target); ! /* ---------------- * Space management for PGresult. * * Formerly, libpq did a separate malloc() for each field of each tuple * returned by a query. This was remarkably expensive --- malloc/free --- 60,72 ---- int resultFormat); static void parseInput(PGconn *conn); static bool PQexecStart(PGconn *conn); static PGresult *PQexecFinish(PGconn *conn); static int PQsendDescribe(PGconn *conn, char desc_type, const char *desc_target); ! static int check_field_number(const PGresult *res, int field_num); /* ---------------- * Space management for PGresult. * * Formerly, libpq did a separate malloc() for each field of each tuple * returned by a query. This was remarkably expensive --- malloc/free *************** *** 192,203 **** --- 192,331 ---- result->client_encoding = PG_SQL_ASCII; } return result; } + PGresult *PQmakeResult( + PGconn *conn, + int numAttributes, + PGresAttDesc *attDescs) + { + int i; + PGresult *res; + + if(numAttributes <= 0 || !attDescs) + return NULL; + + res = PQmakeEmptyPGresult(conn, PGRES_TUPLES_OK); + if(!res) + return NULL; + + res->attDescs = (PGresAttDesc *) + pqResultAlloc(res, numAttributes * sizeof(PGresAttDesc), TRUE); + + if(!res->attDescs) + { + PQclear(res); + return NULL; + } + + res->numAttributes = numAttributes; + memcpy(res->attDescs, attDescs, numAttributes * sizeof(PGresAttDesc)); + + /* resultalloc the attribute names. */ + res->binary = 1; + for(i=0; i < numAttributes; i++) + { + if(attDescs[i].name) + res->attDescs[i].name = pqResultStrdup(res, attDescs[i].name); + else + res->attDescs[i].name = res->null_field; + + if(!res->attDescs[i].name) + { + PQclear(res); + return NULL; + } + + /* Although deprecated, because results can have text+binary columns, + * its easy enough to deduce so set it for completeness. + */ + if(res->attDescs[i].format == 0) + res->binary = 0; + } + + return res; + } + + int PQsetvalue( + PGresult *res, + int tup_num, + int field_num, + char *value, + int len) + { + PGresAttValue *attval; + + if(!check_field_number(res, field_num)) + return FALSE; + + /* Invalid tup_num, must be <= ntups */ + if(tup_num > res->ntups) + return FALSE; + + /* need to grow the tuple table */ + if(res->ntups >= res->tupArrSize) + { + int n = res->tupArrSize ? (res->tupArrSize*3)/2 : 64; + PGresAttValue **tups = (PGresAttValue **) + (res->tuples ? realloc(res->tuples, n*sizeof(PGresAttValue *)) : + malloc(n*sizeof(PGresAttValue *))); + + if(!tups) + return FALSE; + + res->tuples = tups; + res->tupArrSize = n; + } + + /* new to allocate a new tuple */ + if(tup_num == res->ntups && !res->tuples[tup_num]) + { + int i; + PGresAttValue *tup = (PGresAttValue *)pqResultAlloc( + res, res->numAttributes * sizeof(PGresAttValue), TRUE); + + if(!tup) + return FALSE; + + /* initialize each column to NULL */ + for(i=0; i < res->numAttributes; i++) + { + tup[i].len = NULL_LEN; + tup[i].value = res->null_field; + } + + res->tuples[tup_num] = tup; + res->ntups++; + } + + attval = &res->tuples[tup_num][field_num]; + + /* On top of NULL_LEN, treat a NULL value as a NULL field */ + if(len == NULL_LEN || value == NULL) + { + attval->len = NULL_LEN; + attval->value = res->null_field; + } + else + { + if(len < 0) + len = 0; + + attval->value = (char *)pqResultAlloc(res, len + 1, TRUE); + if(!attval->value) + return FALSE; + + attval->len = len; + memcpy(attval->value, value, len); + attval->value[len] = '\0'; + } + + return TRUE; + } /* * pqResultAlloc - * Allocate subsidiary storage for a PGresult. * * nBytes is the amount of space needed for the object. * If isBinary is true, we assume that we need to align the object on *************** *** 350,365 **** --- 478,504 ---- * free's the memory associated with a PGresult */ void PQclear(PGresult *res) { PGresult_data *block; + int objHooksCount; + const PGobjectHooks *objHooks; if (!res) return; + if((objHooksCount = pqGetObjectHooks(&objHooks)) > 0) + { + int i; + + for(i=0; i < objHooksCount; i++) + if(objHooks[i].resultDestroy) + objHooks[i].resultDestroy(objHooks[i].name, (const PGresult *)res); + } + pqReleaseObjectHooks(); /* Free all the subsidiary blocks */ while ((block = res->curBlock) != NULL) { res->curBlock = block->next; free(block); } *************** *** 1179,1190 **** --- 1318,1346 ---- parseInput(conn); /* PQgetResult will return immediately in all states except BUSY. */ return conn->asyncStatus == PGASYNC_BUSY; } + /* Used by PQgetResult */ + static void notifyResultCreateHooks(PGconn *conn, PGresult *res) + { + int objHooksCount; + const PGobjectHooks *objHooks; + + if((objHooksCount = pqGetObjectHooks(&objHooks)) > 0) + { + int i; + for(i=0; i < objHooksCount; i++) + if(objHooks[i].resultCreate) + objHooks[i].resultCreate(objHooks[i].name, (const PGconn *)conn, + (const PGresult *)res); + } + + pqReleaseObjectHooks(); + } /* * PQgetResult * Get the next PGresult produced by a query. Returns NULL if no * query work remains or an error has occurred (e.g. out of * memory). *************** *** 1227,1239 **** /* * conn->errorMessage has been set by pqWait or pqReadData. We * want to append it to any already-received error message. */ pqSaveErrorResult(conn); conn->asyncStatus = PGASYNC_IDLE; ! return pqPrepareAsyncResult(conn); } /* Parse it. */ parseInput(conn); } --- 1383,1403 ---- /* * conn->errorMessage has been set by pqWait or pqReadData. We * want to append it to any already-received error message. */ pqSaveErrorResult(conn); conn->asyncStatus = PGASYNC_IDLE; ! res = pqPrepareAsyncResult(conn); ! ! if(res && res->resultStatus != PGRES_COPY_IN && ! res->resultStatus != PGRES_COPY_OUT) ! { ! notifyResultCreateHooks(conn, res); ! } ! ! return res; } /* Parse it. */ parseInput(conn); } *************** *** 1265,1276 **** --- 1429,1446 ---- libpq_gettext("unexpected asyncStatus: %d\n"), (int) conn->asyncStatus); res = PQmakeEmptyPGresult(conn, PGRES_FATAL_ERROR); break; } + if(res && res->resultStatus != PGRES_COPY_IN && + res->resultStatus != PGRES_COPY_OUT) + { + notifyResultCreateHooks(conn, res); + } + return res; } /* * PQexec
Andrew Chernow <ac@esilo.com> 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
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 altogether. Instead of a Hooks API, just add the pqtypes API.
What are the objections to adding 10 API calls to libpq (each around 5-10 lines,
minimal typedefs needed) and being able to dlopen behind PQtypesLoad?
hooks vs. dlopen
Hooks:
Need 5 API calls
- PQinitTypes
- PQmakeResult
- PQsetvalue
- PQtypeData - avoid locks in pqtypes
- PQresultTypeData - avoid locks in pqtypes
Need hook points in several places
Need storage in conn and result
Need a couple typedefs
Adds minimal size overhead to libpq
Two funcs are useful to other apps, makeResult & setvalue
Must expose internal type, PGresAttDesc
dlopen
Need 10 API calls
Need storage in conn and result
Need a few typedefs
Adds minimal size overhead to libpq
All funcs are useful to other apps
No internals need to be exposed
The two are not much different. Although, the hooks idea is a bit awkward when
applied to pqtypes and adds a layer of abstraction not needed.
Another important question is: What's more useful to libpq apps, a hook api
allowing you to receive created/destroyed events about libpq objects OR the
features of pqtypes (put and get any type in binary including arrays & composites)?
Playing with the idea: PQtypesLoad could take an argument specifying a
libpqtypes version (for speaking to a particular backend). Could even add a
PQtypesUnload so that you can unload+load a different version within the same
process. For now, I am just proposing a load function that should be called
before using libpq.
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/