Message-ID: <48D0DFBE.9050900@esilo.com>
Date: Wed, 17 Sep 2008 06:45:18 -0400
From: Andrew Chernow <ac@esilo.com>
User-Agent: Thunderbird 2.0.0.16 (Windows/20080708)
MIME-Version: 1.0
To: Tom Lane <tgl@sss.pgh.pa.us>
CC: Alvaro Herrera <alvherre@commandprompt.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Merlin Moncure <mmoncure@gmail.com>
Subject: Re: [PATCHES] libpq events patch (with sgml docs)
References: <48C18533.2000204@esilo.com>
	<20080905192215.GK4353@alvh.no-ip.org> <48C18B48.509@esilo.com>
	<20080905194853.GL4353@alvh.no-ip.org> <48C191A7.3000205@esilo.com>
	<20080905201504.GM4353@alvh.no-ip.org>
	<48C19759.90402@esilo.com> <27615.1221619326@sss.pgh.pa.us>
	<48D072E3.6010703@esilo.com> <28673.1221622817@sss.pgh.pa.us>
	<48D080B0.6070104@esilo.com> <454.1221626213@sss.pgh.pa.us>
In-Reply-To: <454.1221626213@sss.pgh.pa.us>
Content-Type: multipart/mixed; boundary="------------030906040802000004040506"

This is a multi-part message in MIME format.
--------------030906040802000004040506
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

 > Now, it's questionable how tense we need to be about that as long as
 > event proc failure aborts calling of later event procs.  That means
 > that procs have to be robust against getting DESTROY with no CREATE
 > calls in any case.  Should we try to make that less uncertain?
 >
 >

I attached a patch that adds a 'needDestroy' member to PGEvent  It is set when 
resultcreate or resultcopy succeeds and checked during a PQclear.  That *should* 
resolve the issue of "no resultcreate but gets a resultdestroy".

> The general question of symmetry between RESULTCREATE and RESULTDESTROY
> callbacks is still bothering me.  As committed, PQmakeEmptyPGresult
> will copy events into its result, but not fire RESULTCREATE for them
> ... but they'll get RESULTDESTROY when it's deleted.  Is that what we
> want?  

PQmakeEmptyPGResult was given thought.  The problem is every internal function 
that generates a result calls PQmakeEmptyPGResult, but those cases should not 
fire a resultcreate.  resultcreate should be called when the result is fully 
constructed (tuples and all) so the eventproc gets a useful PGresult.

One solution is to do something like the below:

PGresult *
PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
{
   return pqMakeEmptyPGresult(conn, status, TRUE);
}

PGresult *
pqMakeEmptyPGresult(PGconn *conn, ExecStatusType status, int fireEvents)
{
   // existing function, only change is handling fireEvents
}

I am willing to create a patch for this.  Is this an acceptable idea?

 > I don't have a lot of faith that PQgetResult is the only place
 > inside libpq that needs to fire RESULTCREATE, either.
 >

I looked again and I didn't see anything.  Is there something your thinking of? 
  ISTM that PQgetResult is called every where a result is created (outside of 
PQmakeEmptyPGresult).

-- 
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

--------------030906040802000004040506
Content-Type: text/x-patch;
 name="pgevent.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="pgevent.patch"

Index: src/interfaces/libpq/fe-exec.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v
retrieving revision 1.198
diff -C6 -r1.198 fe-exec.c
*** src/interfaces/libpq/fe-exec.c	17 Sep 2008 04:31:08 -0000	1.198
--- src/interfaces/libpq/fe-exec.c	17 Sep 2008 10:40:41 -0000
***************
*** 356,367 ****
--- 356,369 ----
  		if (!dest->events[i].proc(PGEVT_RESULTCOPY, &evt,
  								  dest->events[i].passThrough))
  		{
  			PQclear(dest);
  			return NULL;
  		}
+ 
+ 		dest->events[i].needDestroy = TRUE;
  	}
  
  	return dest;
  }
  
  /*
***************
*** 378,394 ****
  		return NULL;
  
  	newEvents = (PGEvent *) malloc(count * sizeof(PGEvent));
  	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].data = NULL;
  		newEvents[i].name = strdup(newEvents[i].name);
  		if (!newEvents[i].name)
  		{
  			while (--i >= 0)
  				free(newEvents[i].name);
--- 380,396 ----
  		return NULL;
  
  	newEvents = (PGEvent *) malloc(count * sizeof(PGEvent));
  	if (!newEvents)
  		return NULL;
  
  	for (i = 0; i < count; i++)
  	{
+ 		newEvents[i].proc = events[i].proc;
+ 		newEvents[i].passThrough = events[i].passThrough;
+ 		newEvents[i].needDestroy = FALSE;
  		newEvents[i].data = NULL;
  		newEvents[i].name = strdup(newEvents[i].name);
  		if (!newEvents[i].name)
  		{
  			while (--i >= 0)
  				free(newEvents[i].name);
***************
*** 663,679 ****
  
  	if (!res)
  		return;
  
  	for (i = 0; i < res->nEvents; i++)
  	{
! 		PGEventResultDestroy evt;
  
- 		evt.result = res;
- 		(void) res->events[i].proc(PGEVT_RESULTDESTROY, &evt,
- 								   res->events[i].passThrough);
  		free(res->events[i].name);
  	}
  
  	if (res->events)
  		free(res->events);
  
--- 665,685 ----
  
  	if (!res)
  		return;
  
  	for (i = 0; i < res->nEvents; i++)
  	{
! 		if(res->events[i].needDestroy)
! 		{
! 			PGEventResultDestroy evt;
! 
! 			evt.result = res;
! 			(void) res->events[i].proc(PGEVT_RESULTDESTROY, &evt,
! 									   res->events[i].passThrough);
! 		}
  
  		free(res->events[i].name);
  	}
  
  	if (res->events)
  		free(res->events);
  
***************
*** 1609,1620 ****
--- 1615,1628 ----
  								  libpq_gettext("PGEventProc \"%s\" failed during PGEVT_RESULTCREATE event\n"),
  								  res->events[i].name);
  				pqSetResultError(res, conn->errorMessage.data);
  				res->resultStatus = PGRES_FATAL_ERROR;
  				break;
  			}
+ 
+ 			res->events[i].needDestroy = TRUE;
  		}
  	}
  
  	return res;
  }
  
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.132
diff -C6 -r1.132 libpq-int.h
*** src/interfaces/libpq/libpq-int.h	17 Sep 2008 04:31:08 -0000	1.132
--- src/interfaces/libpq/libpq-int.h	17 Sep 2008 10:40:41 -0000
***************
*** 153,164 ****
--- 153,165 ----
  typedef struct PGEvent
  {
  	PGEventProc	proc;			/* the function to call on events */
  	char	   *name;			/* used only for error messages */
  	void	   *passThrough;	/* pointer supplied at registration time */
  	void	   *data;			/* optional state (instance) data */
+ 	int		   needDestroy; 	/* indicates a PGEVT_RESULTDESTROY is needed. */
  } PGEvent;
  
  struct pg_result
  {
  	int			ntups;
  	int			numAttributes;

--------------030906040802000004040506--