Here is an updated version of the 'libpq-side' changes required to
implement the libpqtypes proposal.  This fixes the complaints Tom had,
particularly the process lock on the various functions that kept
private state linked up with the PGconn.  We did this in a slightly
different way than Tom proposed...by keeping a void* in the
PGobjectHooks struct.

While we think this approach works, it is possibly to complicated for
what it does.  This could possibly be said of the hooking approach in
general.  We can't think of a good use case for other applications to
use the hooks.  The hook system feels awkward on both sides (in libpq,
and libpqtypes).  Why keep code abstract when it only has one real
purpose?

Possibly, a better approach than the hooking system is a wholesale
wrapping of libpq, introducing a extended PGconn struct (PQTconn for
example), with the private state stored in there and passed to all the
libpqtypes functions.  This would mean a complete new API, something
we wanted to avoid.  We see our proposal as a natural extension of
libpq, library code, not application code.  This difference of
perception is perhaps what the impasse is.

We think our other proposed strategy is better...namely to load the
libpqtypes functionality with dlopen and keep 'stub' functions inside
libpq which raise a runtime error if the library is not loaded.  This
resolves a major raised complaint, library size, and allows the stub
functions to better handle certain things that should be in libpq, for
example error handling.

We understand this raises the bar for libpqtypes in terms of
consensus...since it pushes certain aspects of libpqtypes into libpq,
the whole thing has to be 'signed off' on, and there has naturally
been a certain amount of resistance so far.  At this point, function
stubs (PQparamExec and friends) are all we are proposing to add to
libpq.  The remainder of the code can exist in core, pgfoundry, etc.
This is much more direct and useful to libpq developers than a
abstract hooking interface.

merlin
Index: exports.txt
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.19
diff -C6 -r1.19 exports.txt
*** exports.txt	19 Mar 2008 00:39:33 -0000	1.19
--- exports.txt	14 Apr 2008 14:27:10 -0000
***************
*** 138,143 ****
--- 138,149 ----
  PQsendDescribePortal      136
  lo_truncate               137
  PQconnectionUsedPassword  138
  pg_valid_server_encoding_id 139
  PQconnectionNeedsPassword 140
  lo_import_with_oid		  141
+ PQsetObjectHooks          142
+ PQhookData                143
+ PQresultHookData          144
+ PQmakeResult              145
+ PQsetvalue                146
+ PQresultAlloc             147
\ No newline at end of file
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	14 Apr 2008 14:27:11 -0000
***************
*** 1970,1981 ****
--- 1970,1984 ----
   * 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)
  {
+ 	if(conn->objHooks.connDestroy)
+ 		conn->objHooks.connDestroy((const PGconn *)conn);
+ 
  	if (conn->pghost)
  		free(conn->pghost);
  	if (conn->pghostaddr)
  		free(conn->pghostaddr);
  	if (conn->pgport)
  		free(conn->pgport);
***************
*** 2152,2164 ****
--- 2155,2171 ----
  {
  	if (conn)
  	{
  		closePGconn(conn);
  
  		if (connectDBStart(conn))
+ 		{
  			(void) connectDBComplete(conn);
+ 			if(conn->objHooks.connReset)
+ 				conn->objHooks.connReset((const PGconn *)conn);
+ 		}
  	}
  }
  
  
  /*
   * PQresetStart:
***************
*** 2186,2198 ****
   * closes the existing connection and makes a new one
   */
  PostgresPollingStatusType
  PQresetPoll(PGconn *conn)
  {
  	if (conn)
! 		return PQconnectPoll(conn);
  
  	return PGRES_POLLING_FAILED;
  }
  
  /*
   * PQcancelGet: get a PGcancel structure corresponding to a connection.
--- 2193,2215 ----
   * closes the existing connection and makes a new one
   */
  PostgresPollingStatusType
  PQresetPoll(PGconn *conn)
  {
  	if (conn)
! 	{
! 		PostgresPollingStatusType status = PQconnectPoll(conn);
! 
! 		if((status == PGRES_POLLING_OK || status == PGRES_POLLING_FAILED) &&
! 			 conn->objHooks.connReset)
! 		{
! 			conn->objHooks.connReset((const PGconn *)conn);
! 		}
! 
! 		return status;
! 	}
  
  	return PGRES_POLLING_FAILED;
  }
  
  /*
   * PQcancelGet: get a PGcancel structure corresponding to a connection.
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	14 Apr 2008 14:27:11 -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
***************
*** 163,174 ****
--- 163,176 ----
  
  	if (conn)
  	{
  		/* copy connection data we might need for operations on PGresult */
  		result->noticeHooks = conn->noticeHooks;
  		result->client_encoding = conn->client_encoding;
+ 		memcpy(&result->objHooks, &conn->objHooks, sizeof(PGobjectHooks));
+ 		result->objHooks.data = NULL;
  
  		/* consider copying conn's errorMessage */
  		switch (status)
  		{
  			case PGRES_EMPTY_QUERY:
  			case PGRES_COMMAND_OK:
***************
*** 187,203 ****
--- 189,352 ----
  		/* defaults... */
  		result->noticeHooks.noticeRec = NULL;
  		result->noticeHooks.noticeRecArg = NULL;
  		result->noticeHooks.noticeProc = NULL;
  		result->noticeHooks.noticeProcArg = NULL;
  		result->client_encoding = PG_SQL_ASCII;
+ 		memset(&result->objHooks, 0, sizeof(PGobjectHooks));
  	}
  
  	return result;
  }
  
+ PGresult *PQmakeResult(
+   const PGconn *conn,
+   int numAttributes,
+   PGresAttDesc *attDescs,
+   PGobjectHooks *hooks)
+ {
+ 	int i;
+ 	PGresult *res;
+ 
+ 	if(numAttributes <= 0 || !attDescs)
+ 		return NULL;
+ 
+ 	res = PQmakeEmptyPGresult((PGconn *)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;
+ 	}
+ 
+ 	if(hooks)
+ 	{
+ 		memcpy(&res->objHooks, hooks, sizeof(PGobjectHooks));
+ 		res->objHooks.data = NULL;
+ 	}
+ 
+ 	if(res->objHooks.resultCreate)
+ 		res->objHooks.data = res->objHooks.resultCreate(
+ 			conn, (const PGresult *)res);
+ 
+ 	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;
+ }
+ 
+ void *
+ PQresultAlloc(PGresult *res, size_t nBytes)
+ {
+ 	return pqResultAlloc(res, nBytes, 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
***************
*** 354,365 ****
--- 503,517 ----
  {
  	PGresult_data *block;
  
  	if (!res)
  		return;
  
+ 	if(res->objHooks.resultDestroy)
+ 		res->objHooks.resultDestroy((const PGresult *)res);
+ 
  	/* Free all the subsidiary blocks */
  	while ((block = res->curBlock) != NULL)
  	{
  		res->curBlock = block->next;
  		free(block);
  	}
***************
*** 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);
  	}
  
--- 1379,1401 ----
  			/*
  			 * 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->objHooks.resultCreate &&
! 				 res->resultStatus != PGRES_COPY_IN &&
! 				 res->resultStatus != PGRES_COPY_OUT)
! 			{
! 				res->objHooks.data = res->objHooks.resultCreate(
! 					(const PGconn *)conn, (const PGresult *)res);
! 			}
! 
! 			return res;
  		}
  
  		/* Parse it. */
  		parseInput(conn);
  	}
  
***************
*** 1265,1276 ****
--- 1427,1446 ----
  							  libpq_gettext("unexpected asyncStatus: %d\n"),
  							  (int) conn->asyncStatus);
  			res = PQmakeEmptyPGresult(conn, PGRES_FATAL_ERROR);
  			break;
  	}
  
+ 	if(res && res->objHooks.resultCreate &&
+ 		 res->resultStatus != PGRES_COPY_IN &&
+ 		 res->resultStatus != PGRES_COPY_OUT)
+ 	{
+ 		res->objHooks.data = res->objHooks.resultCreate(
+ 			(const PGconn *)conn, (const PGresult *)res);
+ 	}
+ 
  	return res;
  }
  
  
  /*
   * PQexec
Index: fe-misc.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-misc.c,v
retrieving revision 1.133
diff -C6 -r1.133 fe-misc.c
*** fe-misc.c	1 Jan 2008 19:46:00 -0000	1.133
--- fe-misc.c	14 Apr 2008 14:27:11 -0000
***************
*** 1152,1157 ****
--- 1152,1194 ----
  	}
  
  	return dgettext("libpq", msgid);
  }
  
  #endif   /* ENABLE_NLS */
+ 
+ 
+ /* ---------------------
+  * ObjectHooks
+  */
+ 
+ int PQsetObjectHooks(PGconn *conn, PGobjectHooks *hooks)
+ {
+ 	if(conn && hooks)
+ 	{
+ 		memcpy(&conn->objHooks, hooks, sizeof(PGobjectHooks));
+ 		conn->objHooks.data = NULL;
+ 
+ 		if(conn->objHooks.initHookData)
+ 			conn->objHooks.data = conn->objHooks.initHookData(
+ 				(const PGconn *)conn);
+ 
+ 		return TRUE;
+ 	}
+ 
+ 	return FALSE;
+ }
+ 
+ void *PQhookData(const PGconn *conn)
+ {
+ 	if(conn)
+ 		return conn->objHooks.data;
+ 	return NULL;
+ }
+ 
+ void *PQresultHookData(const PGresult *res)
+ {
+ 	if(res)
+ 		return res->objHooks.data;
+ 	return NULL;
+ }
+ 
Index: libpq-fe.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.142
diff -C6 -r1.142 libpq-fe.h
*** libpq-fe.h	19 Mar 2008 00:39:33 -0000	1.142
--- libpq-fe.h	14 Apr 2008 14:27:12 -0000
***************
*** 190,201 ****
--- 190,246 ----
  		int		   *ptr;		/* can't use void (dec compiler barfs)	 */
  		int			integer;
  	}			u;
  } 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
+ {
+ 	void *data;
+ 
+ 	/* Invoked when PQsetObjectHook is called.  The pointer returned
+ 	 * by the hook implementation is stored in the private storage of
+ 	 * the PGconn, accessible via PQhookData(PGconn*).  If no
+ 	 * storage is needed, return NULL or set this hook to NULL.
+ 	 */
+ 	void *(*initHookData)(const PGconn *conn);
+ 
+ 	/* Invoked on PQreset and PQresetPoll */
+ 	void (*connReset)(const PGconn *conn);
+ 
+ 	/* Invoked on PQfinish. */
+ 	void (*connDestroy)(const PGconn *conn);
+ 
+ 	/* Invoked on PQgetResult, internally called by all exec funcs */
+ 	void *(*resultCreate)(const PGconn *conn, const PGresult *result);
+ 
+ 	/* Invoked when PQclear is called */
+ 	void (*resultDestroy)(const PGresult *result);
+ } PGobjectHooks;
+ 
+ /* ----------------
   * Exported functions of libpq
   * ----------------
   */
  
  /* ===	in fe-connect.c === */
  
***************
*** 427,445 ****
--- 472,512 ----
  #define PQfreeNotify(ptr) PQfreemem(ptr)
  
  /* Error when no password was given. */
  /* Note: depending on this is deprecated; use PQconnectionNeedsPassword(). */
  #define PQnoPasswordSupplied	"fe_sendauth: no password supplied\n"
  
+ extern void *PQresultAlloc(PGresult *res, size_t nBytes);
+ 
  /*
   * Make an empty PGresult with given status (some apps find this
   * useful). If conn is not NULL and status indicates an error, the
   * conn's errorMessage is copied.
   */
  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(const PGconn *conn, int numAttributes,
+   PGresAttDesc *attDescs, PGobjectHooks *hooks);
+ 
+ /*
+  * 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,
  				   char *to, const char *from, size_t length,
  				   int *error);
  extern unsigned char *PQescapeByteaConn(PGconn *conn,
***************
*** 506,517 ****
--- 573,589 ----
  /* Determine display length of multibyte encoded char at *s */
  extern int	PQdsplen(const char *s, int encoding);
  
  /* Get encoding id from environment variable PGCLIENTENCODING */
  extern int	PQenv2encoding(void);
  
+ /* Set the object hooks for a given connection. */
+ extern int PQsetObjectHooks(PGconn *conn, PGobjectHooks *hooks);
+ extern void *PQhookData(const PGconn *conn);
+ extern void *PQresultHookData(const PGresult *res);
+ 
  /* === in fe-auth.c === */
  
  extern char *PQencryptPassword(const char *passwd, const char *user);
  
  /* === in encnames.c === */
  
Index: libpq-int.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.129
diff -C6 -r1.129 libpq-int.h
*** libpq-int.h	1 Jan 2008 19:46:00 -0000	1.129
--- libpq-int.h	14 Apr 2008 14:27:12 -0000
***************
*** 97,121 ****
  union pgresult_data
  {
  	PGresult_data *next;		/* link to next block, or NULL */
  	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
  {
  	Oid			typid;			/* type id */
  } PGresParamDesc;
  
--- 97,108 ----
***************
*** 180,191 ****
--- 167,179 ----
  	/*
  	 * These fields are copied from the originating PGconn, so that operations
  	 * on the PGresult don't have to reference the PGconn.
  	 */
  	PGNoticeHooks noticeHooks;
  	int			client_encoding;	/* encoding id */
+ 	PGobjectHooks objHooks;   /* Object Hooks */
  
  	/*
  	 * Error information (all NULL if not an error result).  errMsg is the
  	 * "overall" error message returned by PQresultErrorMessage.  If we have
  	 * per-field info then it is stored in a linked list.
  	 */
***************
*** 300,311 ****
--- 288,302 ----
  	/* Optional file to write trace info to */
  	FILE	   *Pfdebug;
  
  	/* Callback procedures for notice message processing */
  	PGNoticeHooks noticeHooks;
  
+ 	/* Object Hooks */
+ 	PGobjectHooks objHooks;
+ 
  	/* Status indicators */
  	ConnStatusType status;
  	PGAsyncStatusType asyncStatus;
  	PGTransactionStatusType xactStatus; /* never changes to ACTIVE */
  	PGQueryClass queryclass;
  	char	   *last_query;		/* last SQL command, or NULL if unknown */
-- 
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Reply via email to