From: Andrew Chernow <ac@esilo.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@commandprompt.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Merlin Moncure <mmoncure@gmail.com>
Subject: Re: [PATCHES] libpq events patch (with sgml docs)
Date: Wed, 17 Sep 2008 06:45:18 -0400
Message-ID: <48D0DFBE.9050900@esilo.com> (raw)
In-Reply-To: <454.1221626213@sss.pgh.pa.us>
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>
> 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/Attachments:pgevent.patch (text/x-patch, 3.4K) download
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;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-patches@postgresql.org
Cc: ac@esilo.com, tgl@sss.pgh.pa.us, alvherre@commandprompt.com, pgsql-hackers@postgresql.org, mmoncure@gmail.com
Subject: Re: [PATCHES] libpq events patch (with sgml docs)
In-Reply-To: <48D0DFBE.9050900@esilo.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox