Don't let libpq "event" procs break the state of PGresult objects.

As currently implemented, failure of a PGEVT_RESULTCREATE callback
causes the PGresult to be converted to an error result.  This is
intellectually inconsistent (shouldn't a failing callback likewise
prevent creation of the error result? what about side-effects on the
behavior seen by other event procs? why does PQfireResultCreateEvents
act differently from PQgetResult?), but more importantly it destroys
any promises we might wish to make about the behavior of libpq in
nontrivial operating modes, such as pipeline mode.  For example,
it's not possible to promise that PGRES_PIPELINE_SYNC results will
be returned if an event callback fails on those.  With this
definition, expecting applications to behave sanely in the face of
possibly-failing callbacks seems like a very big lift.

Hence, redefine the result of a callback failure as being simply
that that event procedure won't be called any more for this PGresult
(which was true already).  Event procedures can still signal failure
back to the application through out-of-band mechanisms, for example
via their passthrough arguments.

Similarly, don't let failure of a PGEVT_RESULTCOPY callback prevent
PQcopyResult from succeeding.  That definition allowed a misbehaving
event proc to break single-row mode (our sole internal use of
PQcopyResult), and it probably had equally deleterious effects for
outside uses.

Discussion: https://postgr.es/m/3185105.1644960083@sss.pgh.pa.us

doc/src/sgml/libpq.sgml

@@ -6831,6 +6831,7 @@ PGresult *PQcopyResult(const PGresult *src, int flags);
       <literal>PG_COPYRES_EVENTS</literal> specifies copying the source
       result's events.  (But any instance data associated with the source
       is not copied.)
+      The event procedures receive <literal>PGEVT_RESULTCOPY</literal> events.
      </para>
     </listitem>
    </varlistentry>
@@ -7126,7 +7127,7 @@ defaultNoticeProcessor(void *arg, const char *message)
    <xref linkend="libpq-PQinstanceData"/>,
    <xref linkend="libpq-PQsetInstanceData"/>,
    <xref linkend="libpq-PQresultInstanceData"/> and
-   <function>PQsetResultInstanceData</function> functions.  Note that
+   <xref linkend="libpq-PQresultSetInstanceData"/> functions.  Note that
    unlike the pass-through pointer, instance data of a <structname>PGconn</structname>
    is not automatically inherited by <structname>PGresult</structname>s created from
    it.  <application>libpq</application> does not know what pass-through
@@ -7154,7 +7155,7 @@ defaultNoticeProcessor(void *arg, const char *message)
        is called.  It is the ideal time to initialize any
        <literal>instanceData</literal> an event procedure may need.  Only one
        register event will be fired per event handler per connection.  If the
-       event procedure fails, the registration is aborted.
+       event procedure fails (returns zero), the registration is cancelled.
 
 <synopsis>
 typedef struct
@@ -7261,11 +7262,11 @@ typedef struct
        <parameter>conn</parameter> is the connection used to generate the
        result.  This is the ideal place to initialize any
        <literal>instanceData</literal> that needs to be associated with the
-       result.  If the event procedure fails, the result will be cleared and
+       result.  If an event procedure fails (returns zero), that event
-       the failure will be propagated.  The event procedure must not try to
+       procedure will be ignored for the remaining lifetime of the result;
-       <xref linkend="libpq-PQclear"/> the result object for itself.  When returning a
+       that is, it will not receive <literal>PGEVT_RESULTCOPY</literal>
-       failure code, all cleanup must be performed as no
+       or <literal>PGEVT_RESULTDESTROY</literal> events for this result or
-       <literal>PGEVT_RESULTDESTROY</literal> event will be sent.
+       results copied from it.
       </para>
      </listitem>
     </varlistentry>
@@ -7295,12 +7296,12 @@ typedef struct
        <parameter>src</parameter> result is what was copied while the
        <parameter>dest</parameter> result is the copy destination.  This event
        can be used to provide a deep copy of <literal>instanceData</literal>,
-       since <literal>PQcopyResult</literal> cannot do that.  If the event
+       since <literal>PQcopyResult</literal> cannot do that.  If an event
-       procedure fails, the entire copy operation will fail and the
+       procedure fails (returns zero), that event procedure will be
-       <parameter>dest</parameter> result will be cleared.   When returning a
+       ignored for the remaining lifetime of the new result; that is, it
-       failure code, all cleanup must be performed as no
+       will not receive <literal>PGEVT_RESULTCOPY</literal>
-       <literal>PGEVT_RESULTDESTROY</literal> event will be sent for the
+       or <literal>PGEVT_RESULTDESTROY</literal> events for that result or
-       destination result.
+       results copied from it.
       </para>
      </listitem>
     </varlistentry>
@@ -7618,7 +7619,7 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
             mydata *res_data = dup_mydata(conn_data);
 
             /* associate app specific data with result (copy it from conn) */
-            PQsetResultInstanceData(e->result, myEventProc, res_data);
+            PQresultSetInstanceData(e->result, myEventProc, res_data);
             break;
         }
 
@@ -7629,7 +7630,7 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
             mydata *dest_data = dup_mydata(src_data);
 
             /* associate app specific data with result (copy it from a result) */
-            PQsetResultInstanceData(e->dest, myEventProc, dest_data);
+            PQresultSetInstanceData(e->dest, myEventProc, dest_data);
             break;
         }
 

src/interfaces/libpq/fe-exec.c

@@ -363,19 +363,16 @@ PQcopyResult(const PGresult *src, int flags)
 	/* Okay, trigger PGEVT_RESULTCOPY event */
 	for (i = 0; i < dest->nEvents; i++)
 	{
+		/* We don't fire events that had some previous failure */
 		if (src->events[i].resultInitialized)
 		{
 			PGEventResultCopy evt;
 
 			evt.src = src;
 			evt.dest = dest;
-			if (!dest->events[i].proc(PGEVT_RESULTCOPY, &evt,
+			if (dest->events[i].proc(PGEVT_RESULTCOPY, &evt,
-									  dest->events[i].passThrough))
+									 dest->events[i].passThrough))
-			{
+				dest->events[i].resultInitialized = true;
-				PQclear(dest);
-				return NULL;
-			}
-			dest->events[i].resultInitialized = true;
 		}
 	}
 
@@ -2124,29 +2121,9 @@ PQgetResult(PGconn *conn)
 			break;
 	}
 
-	if (res)
+	/* Time to fire PGEVT_RESULTCREATE events, if there are any */
-	{
+	if (res && res->nEvents > 0)
-		int			i;
+		(void) PQfireResultCreateEvents(conn, res);
-
-		for (i = 0; i < res->nEvents; i++)
-		{
-			PGEventResultCreate evt;
-
-			evt.conn = conn;
-			evt.result = res;
-			if (!res->events[i].proc(PGEVT_RESULTCREATE, &evt,
-									 res->events[i].passThrough))
-			{
-				appendPQExpBuffer(&conn->errorMessage,
-								  libpq_gettext("PGEventProc \"%s\" failed during PGEVT_RESULTCREATE event\n"),
-								  res->events[i].name);
-				pqSetResultError(res, &conn->errorMessage);
-				res->resultStatus = PGRES_FATAL_ERROR;
-				break;
-			}
-			res->events[i].resultInitialized = true;
-		}
-	}
 
 	return res;
 }

src/interfaces/libpq/libpq-events.c

@@ -184,6 +184,7 @@ PQresultInstanceData(const PGresult *result, PGEventProc proc)
 int
 PQfireResultCreateEvents(PGconn *conn, PGresult *res)
 {
+	int			result = true;
 	int			i;
 
 	if (!res)
@@ -191,19 +192,20 @@ PQfireResultCreateEvents(PGconn *conn, PGresult *res)
 
 	for (i = 0; i < res->nEvents; i++)
 	{
+		/* It's possible event was already fired, if so don't repeat it */
 		if (!res->events[i].resultInitialized)
 		{
 			PGEventResultCreate evt;
 
 			evt.conn = conn;
 			evt.result = res;
-			if (!res->events[i].proc(PGEVT_RESULTCREATE, &evt,
+			if (res->events[i].proc(PGEVT_RESULTCREATE, &evt,
-									 res->events[i].passThrough))
+									res->events[i].passThrough))
-				return false;
+				res->events[i].resultInitialized = true;
-
+			else
-			res->events[i].resultInitialized = true;
+				result = false;
 		}
 	}
 
-	return true;
+	return result;
 }