Re: PGEventProcs must not be allowed to break libpq

I wrote:
> ... more generally, it seems to me that allowing a failing PGEventProc
> to cause this to happen is just not sane.  It breaks absolutely
> every guarantee you might think we have about how libpq will behave.
> As an example that seems very plausible currently, if an event proc
> doesn't know what a PGRES_PIPELINE_SYNC result is and fails on it,
> will the application see behavior that's even a little bit sane?
> I don't think so --- it will think the error results are server
> failures, and then be very confused when answers arrive anyway.

Attached are two proposed patches addressing this.  The first one
turns RESULTCREATE and RESULTCOPY events into pure observers,
ie failure of an event procedure doesn't affect the overall
processing of a PGresult.  I think this is necessary if we want
to be able to reason at all about how libpq behaves.  Event
procedures do still have the option to report failure out to the
application in some out-of-band way, such as via their passThrough
argument.  But they can't break what libpq itself does.

The second patch turns CONNRESET events into pure observers.  While
I'm slightly less hot about making that change, the existing behavior
seems very poorly thought-out, not to mention untested.  Notably,
the code there changes conn->status to CONNECTION_BAD without
closing the socket, which is unlike any other post-connection failure
path; so I wonder just how well that'd work if it were exercised in
anger.

Comments, objections?

            regards, tom lane

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index e0ab7cd555..40c39feb7d 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/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
-       the failure will be propagated.  The event procedure must not try to
-       <xref linkend="libpq-PQclear"/> the result object for itself.  When returning a
-       failure code, all cleanup must be performed as no
-       <literal>PGEVT_RESULTDESTROY</literal> event will be sent.
+       result.  If an event procedure fails (returns zero), that event
+       procedure will be ignored for the remaining lifetime of the result;
+       that is, it will not receive <literal>PGEVT_RESULTCOPY</literal>
+       or <literal>PGEVT_RESULTDESTROY</literal> events for this result or
+       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
-       procedure fails, the entire copy operation will fail and the
-       <parameter>dest</parameter> result will be cleared.   When returning a
-       failure code, all cleanup must be performed as no
-       <literal>PGEVT_RESULTDESTROY</literal> event will be sent for the
-       destination result.
+       since <literal>PQcopyResult</literal> cannot do that.  If an event
+       procedure fails (returns zero), that event procedure will be
+       ignored for the remaining lifetime of the new result; that is, it
+       will not receive <literal>PGEVT_RESULTCOPY</literal>
+       or <literal>PGEVT_RESULTDESTROY</literal> events for that result or
+       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;
         }

diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 9afd4d88b4..c7c48d07dc 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/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,
-                                      dest->events[i].passThrough))
-            {
-                PQclear(dest);
-                return NULL;
-            }
-            dest->events[i].resultInitialized = true;
+            if (dest->events[i].proc(PGEVT_RESULTCOPY, &evt,
+                                     dest->events[i].passThrough))
+                dest->events[i].resultInitialized = true;
         }
     }

@@ -2124,29 +2121,9 @@ PQgetResult(PGconn *conn)
             break;
     }

-    if (res)
-    {
-        int            i;
-
-        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;
-        }
-    }
+    /* Time to fire PGEVT_RESULTCREATE events, if there are any */
+    if (res && res->nEvents > 0)
+        (void) PQfireResultCreateEvents(conn, res);

     return res;
 }
diff --git a/src/interfaces/libpq/libpq-events.c b/src/interfaces/libpq/libpq-events.c
index 7754c37748..1ec86b1d64 100644
--- a/src/interfaces/libpq/libpq-events.c
+++ b/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,
-                                     res->events[i].passThrough))
-                return false;
-
-            res->events[i].resultInitialized = true;
+            if (res->events[i].proc(PGEVT_RESULTCREATE, &evt,
+                                    res->events[i].passThrough))
+                res->events[i].resultInitialized = true;
+            else
+                result = false;
         }
     }

-    return true;
+    return result;
 }
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 40c39feb7d..64e17401cd 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -7183,12 +7183,11 @@ typedef struct
       <para>
        The connection reset event is fired on completion of
        <xref linkend="libpq-PQreset"/> or <function>PQresetPoll</function>.  In
-       both cases, the event is only fired if the reset was successful.  If
-       the event procedure fails, the entire connection reset will fail; the
-       <structname>PGconn</structname> is put into
-       <literal>CONNECTION_BAD</literal> status and
-       <function>PQresetPoll</function> will return
-       <literal>PGRES_POLLING_FAILED</literal>.
+       both cases, the event is only fired if the reset was successful.
+       The return value of the event procedure is ignored
+       in <productname>PostgreSQL</productname> v15 and later.
+       With earlier versions, however, it's important to return success
+       (nonzero) or the connection will be aborted.

 <synopsis>
 typedef struct
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 30d6b7b377..9c9416e8ff 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4276,8 +4276,7 @@ PQreset(PGconn *conn)
         if (connectDBStart(conn) && connectDBComplete(conn))
         {
             /*
-             * Notify event procs of successful reset.  We treat an event proc
-             * failure as disabling the connection ... good idea?
+             * Notify event procs of successful reset.
              */
             int            i;

@@ -4286,15 +4285,8 @@ PQreset(PGconn *conn)
                 PGEventConnReset evt;

                 evt.conn = conn;
-                if (!conn->events[i].proc(PGEVT_CONNRESET, &evt,
-                                          conn->events[i].passThrough))
-                {
-                    conn->status = CONNECTION_BAD;
-                    appendPQExpBuffer(&conn->errorMessage,
-                                      libpq_gettext("PGEventProc \"%s\" failed during PGEVT_CONNRESET event\n"),
-                                      conn->events[i].name);
-                    break;
-                }
+                (void) conn->events[i].proc(PGEVT_CONNRESET, &evt,
+                                            conn->events[i].passThrough);
             }
         }
     }
@@ -4336,8 +4328,7 @@ PQresetPoll(PGconn *conn)
         if (status == PGRES_POLLING_OK)
         {
             /*
-             * Notify event procs of successful reset.  We treat an event proc
-             * failure as disabling the connection ... good idea?
+             * Notify event procs of successful reset.
              */
             int            i;

@@ -4346,15 +4337,8 @@ PQresetPoll(PGconn *conn)
                 PGEventConnReset evt;

                 evt.conn = conn;
-                if (!conn->events[i].proc(PGEVT_CONNRESET, &evt,
-                                          conn->events[i].passThrough))
-                {
-                    conn->status = CONNECTION_BAD;
-                    appendPQExpBuffer(&conn->errorMessage,
-                                      libpq_gettext("PGEventProc \"%s\" failed during PGEVT_CONNRESET event\n"),
-                                      conn->events[i].name);
-                    return PGRES_POLLING_FAILED;
-                }
+                (void) conn->events[i].proc(PGEVT_CONNRESET, &evt,
+                                            conn->events[i].passThrough);
             }
         }