Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Andrew Chernow]

Are there any plans to commit these libpq-events changes this fest?

http://archives.postgresql.org/pgsql-hackers/2008-09/msg01109.php

I wanted to release an updated libpqtypes but am waiting on the above 
patch.  If not, I'll release it now.


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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Andrew Chernow [EMAIL PROTECTED] writes:
 Are there any plans to commit these libpq-events changes this fest?

Sorry about that, I got distracted.  Will look at it shortly.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Andrew Chernow [EMAIL PROTECTED] writes:
 To build on this analogy, PGEVT_CONNRESET is like a realloc.  Meaning, 
 the initial malloc PGEVT_REGISTER worked by the realloc 
 PGEVT_CONNRESET didn't ... you still have free PGEVT_CONNDESTROY the 
 initial.  Its documented that way.  Basically if a register succeeds, a 
 destroy will always be sent regardless of what happens with a reset.

 I attached the wrong patch.  I'm sorry.

I had a further thought about this: after applying this patch, it is
essentially useless for the exposed PQmakeEmptyPGresult function to
copy events into the result.  If it doesn't give them a RESULTCREATE
call, then they cannot receive RESULTCOPY or RESULTDESTROY either,
so they might as well not be there.

The argument for not having PQmakeEmptyPGresult fire RESULTCREATE still
makes sense, but I am thinking that maybe what we ought to do is expose
a new function named something like PQfireResultCreateEvents() that just
does that.  This would allow an application to exactly emulate what
PQgetResult does: make an empty PGresult, fill it, then fire the create
events.

I'll go ahead and apply this patch in a little bit, but if you concur
with the above reasoning, please put together a followon patch to add
such a function.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Andrew Chernow]

Tom Lane wrote:


I'll go ahead and apply this patch in a little bit, but if you concur
with the above reasoning, please put together a followon patch to add
such a function.

regards, tom lane




I attached a patch.  You have to copy the events in PQmakeEmptyPGResult 
because there is no where else to do this, other than copyresult but 
that is different because it copies from a result not a conn.


PQmakeEmptyPGResult - must copy events here
PQsetResultAttrs - set attributes
PQsetvalue - set tuple values
PQfireResultCreateEvents(conn,res) - now fire resultcreate event

PQgetResult now calls PQfireResultCreateEvents.

BTW, the event system might be an alternative solution for PGNoticeHooks 
(PGEVT_NOTICE).


--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Index: doc/src/sgml/libpq.sgml
===
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.262
diff -C6 -r1.262 libpq.sgml
*** doc/src/sgml/libpq.sgml 19 Sep 2008 16:40:40 -  1.262
--- doc/src/sgml/libpq.sgml 19 Sep 2008 18:05:55 -
***
*** 4592,4604 
parameterconn/parameter is not null and parameterstatus/
indicates an error, the current error message of the specified
connection is copied into the structnamePGresult/structname.
Also, if parameterconn/parameter is not null, any event handlers
registered in the connection are copied into the
structnamePGresult/structname (but they don't get
!   literalPGEVT_RESULTCREATE/ calls).
Note that functionPQclear/function should eventually be called
on the object, just as with a structnamePGresult/structname
returned by applicationlibpq/application itself.
   /para
  /listitem
 /varlistentry
--- 4592,4606 
parameterconn/parameter is not null and parameterstatus/
indicates an error, the current error message of the specified
connection is copied into the structnamePGresult/structname.
Also, if parameterconn/parameter is not null, any event handlers
registered in the connection are copied into the
structnamePGresult/structname (but they don't get
!   literalPGEVT_RESULTCREATE/ calls).  Although,
!   functionPQfireResultCreateEvents/function can be used to fire
!   literalPGEVT_RESULTCREATE/ events.
Note that functionPQclear/function should eventually be called
on the object, just as with a structnamePGresult/structname
returned by applicationlibpq/application itself.
   /para
  /listitem
 /varlistentry
***
*** 5242,5253 
--- 5244,5279 
  void *PQresultInstanceData(const PGresult *res, PGEventProc proc);
 /synopsis
/para
   /listitem
  /varlistentry
 /variablelist
+ 
+ varlistentry
+  term
+   functionPQfireResultCreateEvents/function
+   indexterm
+primaryPQfireResultCreateEvents/primary
+   /indexterm
+  /term
+  listitem
+   para
+Manually fires a literalPGEVT_RESULTCREATE/literal event.  This is
+useful for applications that create a result using
+functionPQmakeEmptyPGResult/function, which does not fire a
+literalPGEVT_RESULTCREATE/literal event.  It allows an application
+to create the result, fill it and then fire the creation event.  This
+returns non-zero for success and zero for failure.
+ 
+synopsis
+ int PQfireResultCreateEvents(const PGconn conn, PGresult *res);
+/synopsis
+   /para
+  /listitem
+ /varlistentry
+/variablelist
/sect2
  
sect2 id=libpq-events-example
 titleEvent Example/title
  
 para
Index: src/interfaces/libpq/exports.txt
===
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.20
diff -C6 -r1.20 exports.txt
*** src/interfaces/libpq/exports.txt17 Sep 2008 04:31:08 -  1.20
--- src/interfaces/libpq/exports.txt19 Sep 2008 18:05:55 -
***
*** 147,152 
--- 147,153 
  PQresultAlloc 145
  PQregisterEventProc   146
  PQinstanceData147
  PQsetInstanceData 148
  PQresultInstanceData  149
  PQresultSetInstanceData   150
+ PQfireResultCreateEvents  151
Index: src/interfaces/libpq/fe-exec.c
===
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v
retrieving revision 1.199
diff -C6 -r1.199 fe-exec.c
*** src/interfaces/libpq/fe-exec.c  19 Sep 2008 16:40:40 -  1.199
--- src/interfaces/libpq/fe-exec.c  19 Sep 2008 18:05:55 -
***
*** 1595,1630 
  
libpq_gettext(unexpected asyncStatus: %d\n),
 

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Merlin Moncure]

On Fri, Sep 19, 2008 at 2:14 PM, Andrew Chernow [EMAIL PROTECTED] wrote:

 BTW, the event system might be an alternative solution for PGNoticeHooks
 (PGEVT_NOTICE).


Another possible use of the event hooks -- just spitballing here -- is
to generate an event when a notification comes through (you would
still receive events the old way., by command or PQconsumeInput).
Maybe this would eventually replace the current notification interface
(wasn't this going to be changed anyways?)

merlin

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

BTW, why are all the conn parameters to events declared const?  Isn't
the reason for passing them in mainly to give the event proc a chance
to issue queries?

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Andrew Chernow]

Tom Lane wrote:

BTW, why are all the conn parameters to events declared const?  Isn't


Because it looked prettier?  Kidding.  No idea, do you want me to change 
that or do you want to?



the reason for passing them in mainly to give the event proc a chance
to issue queries?




Partly.  You also want to give the eventproc a chance to issue a 
PQinstanceData call, so it can copy stuff to the created result.


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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Andrew Chernow [EMAIL PROTECTED] writes:
 Tom Lane wrote:
 BTW, why are all the conn parameters to events declared const?  Isn't

 Because it looked prettier?  Kidding.  No idea, do you want me to change 
 that or do you want to?

I'll fix it; it's hardly worth passing a patch around for.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Andrew Chernow [EMAIL PROTECTED] writes:
 I attached a patch.  You have to copy the events in PQmakeEmptyPGResult 
 because there is no where else to do this, other than copyresult but 
 that is different because it copies from a result not a conn.

Applied ...

 PQgetResult now calls PQfireResultCreateEvents.

... except I didn't do that because the error handling didn't seem
appropriate.  Since PQmakeEmptyPGResult allows a null conn,
PQfireResultCreateEvents ought to as well.  So I just made it return
false on failure.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Merlin Moncure [EMAIL PROTECTED] writes:
 On Fri, Sep 19, 2008 at 2:14 PM, Andrew Chernow [EMAIL PROTECTED] wrote:
 BTW, the event system might be an alternative solution for PGNoticeHooks
 (PGEVT_NOTICE).

 Another possible use of the event hooks -- just spitballing here -- is
 to generate an event when a notification comes through (you would
 still receive events the old way., by command or PQconsumeInput).

Seems rather pointless; you'd just be adding multiple ways to do things
we can do perfectly well now.

 Maybe this would eventually replace the current notification interface
 (wasn't this going to be changed anyways?)

No, I don't think anyone was unhappy with the libpq API for it.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Andrew Chernow]

 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/
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 -	1.198
--- src/interfaces/libpq/fe-exec.c	17 Sep 2008 10:40:41 -
***
*** 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 -	1.132
--- src/interfaces/libpq/libpq-int.h	17 Sep 2008 10:40:41 -
***
*** 153,164 
--- 153,165 
  typedef struct PGEvent
  {
  	PGEventProc	proc;			/* the function 

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Andrew Chernow]

Andrew Chernow wrote:

  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.





Shoot.  I have a booboo in that last patch.  Attached is the corrected version.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
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 -	1.198
--- src/interfaces/libpq/fe-exec.c	17 Sep 2008 10:49:10 -
***
*** 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,396 
  		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);
  			free(newEvents);
  			return NULL;
--- 380,398 
  		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(events[i].name);
  		if (!newEvents[i].name)
  		{
  			while (--i = 0)
  free(newEvents[i].name);
  			free(newEvents);
  			return NULL;
***
*** 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 -	1.132
--- src/interfaces/libpq/libpq-int.h	17 Sep 2008 10:49:10 -
***
*** 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;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Andrew Chernow [EMAIL PROTECTED] writes:
 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.

Some thought would need to be given to how that interacts with
RESULTCOPY.  Presumably on the destination side, RESULTCOPY is the
equivalent of RESULTCREATE, ie, don't DESTROY if you didn't get COPY.
But what of the source side?  Arguably you shouldn't invoke COPY on
events that were never initialized in this object.

I also think that a little bit of thought should go into whether or
not to call DESTROY on an event that *did* get a CREATE and failed it.
You could argue that one either way: should a failing CREATE operation
be expected to fully clean up after itself, or should it be expected
to leave things in a state where DESTROY can clean up properly?
I'm not entirely sure, but option A might force duplication of code
between CREATE's failure path and DESTROY.  Whichever semantics we
choose needs to be documented.

Also, if we choose option B, then the same would hold for REGISTER
versus CONNDESTROY: failing REGISTER should still mean that you get
a CONNDESTROY.  So maybe that's an argument for option A, because
that's how REGISTER works now.
 
Lastly, the idea that was in the back of my mind was to resolve this
issue by unconditionally calling all the event procs at CREATE time,
not by cutting off the later ones if an earlier one failed.  Again,
I do not see a performance argument for skipping the extra steps,
and it seems to me that it might improve symmetry/robustness.
I'm not necessarily wedded to that approach but it bears thinking
about.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Andrew Chernow]

Tom Lane wrote:


Some thought would need to be given to how that interacts with
RESULTCOPY.  Presumably on the destination side, RESULTCOPY is the
equivalent of RESULTCREATE, ie, don't DESTROY if you didn't get COPY.
But what of the source side?  Arguably you shouldn't invoke COPY on
events that were never initialized in this object.



You are correct.  The resultcopy function needs to check if the src 
result eventproc was initialized.  BUT, that should not be possible 
unless someone is not checking return values.  Meaning, the only way to 
have an uninitialized eventproc in this instance is if something failed 
during a resultcreate.  Actually, if you call PQmakeEmptyPGResult then 
you can also run into this problem.  That can be solved by adding an 
argument to makeResult as I previously suggested.



I also think that a little bit of thought should go into whether or
not to call DESTROY on an event that *did* get a CREATE and failed it.
You could argue that one either way: should a failing CREATE operation
be expected to fully clean up after itself, or should it be expected
to leave things in a state where DESTROY can clean up properly?
I'm not entirely sure, but option A might force duplication of code
between CREATE's failure path and DESTROY.  Whichever semantics we
choose needs to be documented.



If a resultcreate fails, than I think that resultdestroy should not be 
delivered to that eventproc (same for resultcopy).  That is how register 
works and how I saw this patch working (eventhough it appears I have a 
few logical errors).  I don't think it makes sense to do it otherwise, 
it would be like calling free after a malloc failure.


The needDestroy member should be called resultInitialized.  Although the 
conn doesn't reference the 'resultInitialized' member, I should 
initialize it to FALSE.  I did not do this in the last patch ... 
register function.


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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Andrew Chernow [EMAIL PROTECTED] writes:
 Tom Lane wrote:
 Some thought would need to be given to how that interacts with
 RESULTCOPY.  Presumably on the destination side, RESULTCOPY is the
 equivalent of RESULTCREATE, ie, don't DESTROY if you didn't get COPY.
 But what of the source side?  Arguably you shouldn't invoke COPY on
 events that were never initialized in this object.

 You are correct.  The resultcopy function needs to check if the src 
 result eventproc was initialized.  BUT, that should not be possible 
 unless someone is not checking return values.  Meaning, the only way to 
 have an uninitialized eventproc in this instance is if something failed 
 during a resultcreate.  Actually, if you call PQmakeEmptyPGResult then 
 you can also run into this problem.  That can be solved by adding an 
 argument to makeResult as I previously suggested.

I still think it would be a good idea to expend a couple more lines in
PQcopyResult to cover the case --- the likelihood of there being code
paths that make an empty result and never invoke RESULTCREATE just seems
too high.  Defensive programming 'n all that.  In any case I'm not
convinced that we should force a RESULTCREATE cycle on external callers
of PQmakeEmptyPGResult.  If you guys didn't see a need for it in your
development then it probably doesn't exist.

 If a resultcreate fails, than I think that resultdestroy should not be 
 delivered to that eventproc (same for resultcopy).  That is how register 
 works and how I saw this patch working (eventhough it appears I have a 
 few logical errors).  I don't think it makes sense to do it otherwise, 
 it would be like calling free after a malloc failure.

I can live with that definition, but please document it.

 The needDestroy member should be called resultInitialized.

Yeah, probably, so that you can set it FALSE in conn events and continue
to use memcpy to move things over.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Andrew Chernow]

Andrew Chernow wrote:

New patch following our discussion with updated docs.

few logical errors).  I don't think it makes sense to do it 
otherwise, it would be like calling free after a malloc failure.


I can live with that definition, but please document it.




To build on this analogy, PGEVT_CONNRESET is like a realloc.  Meaning, 
the initial malloc PGEVT_REGISTER worked by the realloc 
PGEVT_CONNRESET didn't ... you still have free PGEVT_CONNDESTROY the 
initial.  Its documented that way.  Basically if a register succeeds, a 
destroy will always be sent regardless of what happens with a reset.





I attached the wrong patch.  I'm sorry.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Index: doc/src/sgml/libpq.sgml
===
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.261
diff -C6 -r1.261 libpq.sgml
*** doc/src/sgml/libpq.sgml 17 Sep 2008 04:31:08 -  1.261
--- doc/src/sgml/libpq.sgml 17 Sep 2008 14:19:29 -
***
*** 4911,4923 
 When a literalPGEVT_REGISTER/literal event is received, the
 parameterevtInfo/parameter pointer should be cast to a
 structnamePGEventRegister */structname.  This structure contains a
 structnamePGconn/structname that should be in the
 literalCONNECTION_OK/literal status; guaranteed if one calls
 functionPQregisterEventProc/function right after obtaining a good
!structnamePGconn/structname.
/para
   /listitem
  /varlistentry
  
  varlistentry
   termliteralPGEVT_CONNRESET/literal/term
--- 4911,4925 
 When a literalPGEVT_REGISTER/literal event is received, the
 parameterevtInfo/parameter pointer should be cast to a
 structnamePGEventRegister */structname.  This structure contains a
 structnamePGconn/structname that should be in the
 literalCONNECTION_OK/literal status; guaranteed if one calls
 functionPQregisterEventProc/function right after obtaining a good
!structnamePGconn/structname.  When returning a failure code, all
!cleanup must be performed as no literalPGEVT_CONNDESTROY/literal
!event will be sent.
/para
   /listitem
  /varlistentry
  
  varlistentry
   termliteralPGEVT_CONNRESET/literal/term
***
*** 4941,4953 
  
 When a literalPGEVT_CONNRESET/literal event is received, the
 parameterevtInfo/parameter pointer should be cast to a
 structnamePGEventConnReset */structname.  Although the contained
 structnamePGconn/structname was just reset, all event data remains
 unchanged.  This event should be used to reset/reload/requery any
!associated literalinstanceData/literal.
/para
   /listitem
  /varlistentry
  
  varlistentry
   termliteralPGEVT_CONNDESTROY/literal/term
--- 4943,4956 
  
 When a literalPGEVT_CONNRESET/literal event is received, the
 parameterevtInfo/parameter pointer should be cast to a
 structnamePGEventConnReset */structname.  Although the contained
 structnamePGconn/structname was just reset, all event data remains
 unchanged.  This event should be used to reset/reload/requery any
!associated literalinstanceData/literal.  A PGEVT_CONNDESTROY event
!is always sent, regardless of the event procedure's return value.
/para
   /listitem
  /varlistentry
  
  varlistentry
   termliteralPGEVT_CONNDESTROY/literal/term
***
*** 5000,5023 
 structnamePGEventResultCreate */structname.  The
 parameterconn/parameter is the connection used to generate the
 result.  This is the ideal place to initialize any
 literalinstanceData/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
!functionPQclear/ the result object for itself.
/para
   /listitem
  /varlistentry
  
  varlistentry
   termliteralPGEVT_RESULTCOPY/literal/term
   listitem
para
 The result copy event is fired in response to
 functionPQcopyResult/function.  This event will only be fired after
!the copy is complete.
  
synopsis
  typedef struct
  {
  const PGresult *src;
  PGresult *dest;
--- 5003,5030 
 structnamePGEventResultCreate */structname.  The
 parameterconn/parameter is the connection used to generate the
 result.  This is the ideal place to initialize any
 literalinstanceData/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
!

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Andrew Chernow [EMAIL PROTECTED] writes:
 Missed that one.  Good catch :)  Update attached.

Looking at this now.  Question: why does PQgetResult invoke the
RESULTCREATE event only for non-error results?  This seems a rather
odd asymmetry, particularly in view of the fact that a RESULTDESTROY
event will occur for error results.  And surely we do not need to
micro-optimize error cases for speed.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Andrew Chernow]

Tom Lane wrote:

Andrew Chernow [EMAIL PROTECTED] writes:

Missed that one.  Good catch :)  Update attached.


Looking at this now.  Question: why does PQgetResult invoke the
RESULTCREATE event only for non-error results?  


It didn't seem useful to have the eventproc implementation allocate its private 
storage (or do whatever prep work it does), just to have it PQclear'd once the 
user gets the dead result back.  I guess we figured an error result typically 
has a short life-span, not very useful outside of indicating an error.


Is there a use case you think requires the opposite behavior?


odd asymmetry, particularly in view of the fact that a RESULTDESTROY
event will occur for error results.  And surely we do not need to
micro-optimize error cases for speed.



It is odd.  Actually, it looks like a bug to me.  PQgetResult is the behavior we 
were after, don't trigger the event if the creation failed.  Same thing occurs 
during a conn reset.  Looks like PQclear needs to check resultStatus before it 
triggers RESULTDESTROY events.


But before I fix this and put a patch up, please let me know if you prefer the 
opposite behavior ... trigger events on success or failure (including connreset).


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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Andrew Chernow [EMAIL PROTECTED] writes:
 Tom Lane wrote:
 Looking at this now.  Question: why does PQgetResult invoke the
 RESULTCREATE event only for non-error results?  

 It didn't seem useful to have the eventproc implementation allocate its 
 private 
 storage (or do whatever prep work it does), just to have it PQclear'd once 
 the 
 user gets the dead result back.  I guess we figured an error result typically 
 has a short life-span, not very useful outside of indicating an error.

Well, you could do a PQresultStatus and skip any prep work if you
actually saw a need to micro-optimize such cases.

 It is odd.  Actually, it looks like a bug to me.  PQgetResult is the
 behavior we were after, don't trigger the event if the creation
 failed.  Same thing occurs during a conn reset.  Looks like PQclear
 needs to check resultStatus before it triggers RESULTDESTROY events.

If you did that, then you WOULD have a bug.  Consider case where you
successfully init two events, and the third one fails.  You'll now
change the result's status to ERROR.  If you don't RESULTDESTROY
then the first two events will leak whatever storage they allocated.

The reason I suggested not being conditional about the init call was
to reduce the probability of bugs of similar ilks associated with
an event proc assuming that it could only get a DESTROY call for
something it had seen CREATEd --- for example, if it were frobbing
a reference count for some long-lived data, it could very easily
screw up the reference count that way.  I suppose that the risk of
an earlier event proc failing means it has to cope with that case
anyway, but I don't think it's a particularly good idea to have
arbitrary asymmetries increasing the odds of a problem.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Tom Lane]

Andrew Chernow [EMAIL PROTECTED] writes:
 Yeah.  Good point.  I fixed it to always fire events.

Applied with editorial revisions --- I don't think I changed any
functionality, but I fixed a number of corner-case bugs and
editorialized on style a bit.

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?  I don't have a lot of faith that PQgetResult is the only place
inside libpq that needs to fire RESULTCREATE, either.

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?

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Alvaro Herrera]

Andrew Chernow escribió:

   /*
* PQmakeEmptyPGresult
*   returns a newly allocated, initialized PGresult with given status.
*   If conn is not NULL and status indicates an error, the conn's
*   errorMessage is copied.
*
* Note this is exported --- you wouldn't think an application would need
* to build its own PGresults, but this has proven useful in both libpgtcl
* and the Perl5 interface, so maybe it's not so unreasonable.
 +  *
 +  * Updated April 2008 - If conn is not NULL, event states will be copied
 +  * from the PGconn to the created PGresult.
*/

Don't do this either.  We don't really need to know when the thing was
changed; the comment should just state what the function does.  I had
folded the last paragraph into the introductory one, but I think you
lost that part of my change.

 + /* resultalloc the attribute names.  The above memcpy has the attr
 +  * names pointing at the callers provided attDescs memory.
 +  */

resultalloc?  Why not just allocate?

* PQclear -
*free's the memory associated with a PGresult
*/

I'd add a comment here stating why the event name is not deallocated;
otherwise it just looks like it's being leaked.


-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCHES] libpq events patch (with sgml docs)

[Alvaro Herrera]

Andrew Chernow escribió:
 Alvaro Herrera wrote:
 Andrew Chernow escribió:

* PQclear -
*  free's the memory associated with a PGresult
*/

 I'd add a comment here stating why the event name is not deallocated;
 otherwise it just looks like it's being leaked.

 The event name is being deallocated.

Doh!  Sorry, you're right.  In that case, you're missing a NULL result
check from strdup() in dupEvents() ;-)

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers