Improve the recently-added libpq events code to provide more consistent

guarantees about whether event procedures will receive DESTROY events.
They no longer need to defend themselves against getting a DESTROY
without a successful prior CREATE.

Andrew Chernow

Author: tglsfdc

doc/src/sgml/libpq.sgml

@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/libpq.sgml,v 1.261 2008/09/17 04:31:08 tgl Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/libpq.sgml,v 1.262 2008/09/19 16:40:40 tgl Exp $ -->
 
 <chapter id="libpq">
  <title><application>libpq</application> - C Library</title>
@@ -4914,7 +4914,9 @@ typedef struct
        <structname>PGconn</structname> that should be in the
        <literal>CONNECTION_OK</literal> status; guaranteed if one calls
        <function>PQregisterEventProc</function> right after obtaining a good
-       <structname>PGconn</structname>.
+       <structname>PGconn</structname>.  When returning a failure code, all
+       cleanup must be performed as no <literal>PGEVT_CONNDESTROY</literal>
+       event will be sent.
       </para>
      </listitem>
     </varlistentry>
@@ -4944,7 +4946,10 @@ typedef struct
        <structname>PGEventConnReset *</structname>.  Although the contained
        <structname>PGconn</structname> was just reset, all event data remains
        unchanged.  This event should be used to reset/reload/requery any
-       associated <literal>instanceData</literal>.
+       associated <literal>instanceData</literal>.  Note that even if the
+       event procedure fails to process <literal>PGEVT_CONNRESET</>, it will
+       still receive a <literal>PGEVT_CONNDESTROY</> event when the connection
+       is closed.
       </para>
      </listitem>
     </varlistentry>
@@ -5003,7 +5008,9 @@ typedef struct
        <literal>instanceData</literal> that needs to be associated with the
        result.  If the event procedure fails, the result will be cleared and
        the failure will be propagated.  The event procedure must not try to
-       <function>PQclear</> the result object for itself.
+       <function>PQclear</> the result object for itself.  When returning a
+       failure code, all cleanup must be performed as no
+       <literal>PGEVT_RESULTDESTROY</literal> event will be sent.
       </para>
      </listitem>
     </varlistentry>
@@ -5014,7 +5021,10 @@ typedef struct
       <para>
        The result copy event is fired in response to
        <function>PQcopyResult</function>.  This event will only be fired after
-       the copy is complete.
+       the copy is complete.  Only event procedures that have
+       successfully handled the <literal>PGEVT_RESULTCREATE</literal>
+       or <literal>PGEVT_RESULTCOPY</literal> event for the source result
+       will receive <literal>PGEVT_RESULTCOPY</literal> events.
 
       <synopsis>
 typedef struct
@@ -5032,7 +5042,10 @@ typedef struct
        can be used to provide a deep copy of <literal>instanceData</literal>,
        since <literal>PQcopyResult</literal> cannot do that.  If the event
        procedure fails, the entire copy operation will fail and the
-       <parameter>dest</parameter> result will be cleared.
+       <parameter>dest</parameter> result will be cleared.   When returning a
+       failure code, all cleanup must be performed as no
+       <literal>PGEVT_RESULTDESTROY</literal> event will be sent for the
+       destination result.
       </para>
      </listitem>
     </varlistentry>

src/interfaces/libpq/fe-exec.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/interfaces/libpq/fe-exec.c,v 1.198 2008/09/17 04:31:08 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/interfaces/libpq/fe-exec.c,v 1.199 2008/09/19 16:40:40 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -331,10 +331,7 @@ PQcopyResult(const PGresult *src, int flags)
 	if (flags & PG_COPYRES_NOTICEHOOKS)
 		dest->noticeHooks = src->noticeHooks;
 
-	/*
-	 * Wants to copy PGEvents?  NB: this should be last, as we don't want
-	 * to trigger RESULTDESTROY events on a useless PGresult.
-	 */
+	/* Wants to copy PGEvents? */
 	if ((flags & PG_COPYRES_EVENTS) && src->nEvents > 0)
 	{
 		dest->events = dupEvents(src->events, src->nEvents);
@@ -349,24 +346,29 @@ PQcopyResult(const PGresult *src, int flags)
 	/* Okay, trigger PGEVT_RESULTCOPY event */
 	for (i = 0; i < dest->nEvents; i++)
 	{
-		PGEventResultCopy evt;
-
-		evt.src = src;
-		evt.dest = dest;
-		if (!dest->events[i].proc(PGEVT_RESULTCOPY, &evt,
-								  dest->events[i].passThrough))
+		if (src->events[i].resultInitialized)
 		{
-			PQclear(dest);
-			return NULL;
+			PGEventResultCopy evt;
+
+			evt.src = src;
+			evt.dest = dest;
+			if (!dest->events[i].proc(PGEVT_RESULTCOPY, &evt,
+									  dest->events[i].passThrough))
+			{
+				PQclear(dest);
+				return NULL;
+			}
+			dest->events[i].resultInitialized = TRUE;
 		}
 	}
 
 	return dest;
 }
 
 /*
- * Copy an array of PGEvents (with no extra space for more)
- * Does not duplicate the event instance data, sets this to NULL
+ * Copy an array of PGEvents (with no extra space for more).
+ * Does not duplicate the event instance data, sets this to NULL.
+ * Also, the resultInitialized flags are all cleared.
  */
 static PGEvent *
 dupEvents(PGEvent *events, int count)
@@ -381,13 +383,13 @@ dupEvents(PGEvent *events, int count)
 	if (!newEvents)
 		return NULL;
 
-	memcpy(newEvents, events, count * sizeof(PGEvent));
-
-	/* NULL out the data pointers and deep copy names */
 	for (i = 0; i < count; i++)
 	{
+		newEvents[i].proc = events[i].proc;
+		newEvents[i].passThrough = events[i].passThrough;
 		newEvents[i].data = NULL;
-		newEvents[i].name = strdup(newEvents[i].name);
+		newEvents[i].resultInitialized = FALSE;
+		newEvents[i].name = strdup(events[i].name);
 		if (!newEvents[i].name)
 		{
 			while (--i >= 0)
@@ -666,11 +668,15 @@ PQclear(PGresult *res)
 
 	for (i = 0; i < res->nEvents; i++)
 	{
-		PGEventResultDestroy evt;
+		/* only send DESTROY to successfully-initialized event procs */
+		if (res->events[i].resultInitialized)
+		{
+			PGEventResultDestroy evt;
 
-		evt.result = res;
-		(void) res->events[i].proc(PGEVT_RESULTDESTROY, &evt,
-								   res->events[i].passThrough);
+			evt.result = res;
+			(void) res->events[i].proc(PGEVT_RESULTDESTROY, &evt,
+									   res->events[i].passThrough);
+		}
 		free(res->events[i].name);
 	}
 
@@ -1612,6 +1618,7 @@ PQgetResult(PGconn *conn)
 				res->resultStatus = PGRES_FATAL_ERROR;
 				break;
 			}
+			res->events[i].resultInitialized = TRUE;
 		}
 	}
 

src/interfaces/libpq/libpq-events.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/interfaces/libpq/libpq-events.c,v 1.1 2008/09/17 04:31:08 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/interfaces/libpq/libpq-events.c,v 1.2 2008/09/19 16:40:40 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -76,6 +76,7 @@ PQregisterEventProc(PGconn *conn, PGEventProc proc,
 		return FALSE;
 	conn->events[conn->nEvents].passThrough = passThrough;
 	conn->events[conn->nEvents].data = NULL;
+	conn->events[conn->nEvents].resultInitialized = FALSE;
 	conn->nEvents++;
 
 	regevt.conn = conn;

src/interfaces/libpq/libpq-int.h

@@ -12,7 +12,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/interfaces/libpq/libpq-int.h,v 1.132 2008/09/17 04:31:08 tgl Exp $
+ * $PostgreSQL: pgsql/src/interfaces/libpq/libpq-int.h,v 1.133 2008/09/19 16:40:40 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -156,6 +156,7 @@ typedef struct PGEvent
 	char	   *name;			/* used only for error messages */
 	void	   *passThrough;	/* pointer supplied at registration time */
 	void	   *data;			/* optional state (instance) data */
+	bool		resultInitialized;	/* T if RESULTCREATE/COPY succeeded */
 } PGEvent;
 
 struct pg_result