Re: [PATCHES] libpq events patch (with sgml docs) - Mailing list pgsql-hackers

 > 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 -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;