From ce1e7a2f716919652c280937087b24937677f8b3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 18 Feb 2022 11:37:27 -0500 Subject: [PATCH] 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 | 31 ++++++++++++------------ src/interfaces/libpq/fe-exec.c | 37 ++++++----------------------- src/interfaces/libpq/libpq-events.c | 14 ++++++----- 3 files changed, 31 insertions(+), 51 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index e0ab7cd5555..40c39feb7d0 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -6831,6 +6831,7 @@ PGresult *PQcopyResult(const PGresult *src, int flags); PG_COPYRES_EVENTS specifies copying the source result's events. (But any instance data associated with the source is not copied.) + The event procedures receive PGEVT_RESULTCOPY events. @@ -7126,7 +7127,7 @@ defaultNoticeProcessor(void *arg, const char *message) , , and - PQsetResultInstanceData functions. Note that + functions. Note that unlike the pass-through pointer, instance data of a PGconn is not automatically inherited by PGresults created from it. libpq 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 instanceData 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. typedef struct @@ -7261,11 +7262,11 @@ typedef struct conn is the connection used to generate the result. This is the ideal place to initialize any instanceData 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 - the result object for itself. When returning a - failure code, all cleanup must be performed as no - PGEVT_RESULTDESTROY 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 PGEVT_RESULTCOPY + or PGEVT_RESULTDESTROY events for this result or + results copied from it. @@ -7295,12 +7296,12 @@ typedef struct src result is what was copied while the dest result is the copy destination. This event can be used to provide a deep copy of instanceData, - since PQcopyResult cannot do that. If the event - procedure fails, the entire copy operation will fail and the - dest result will be cleared. When returning a - failure code, all cleanup must be performed as no - PGEVT_RESULTDESTROY event will be sent for the - destination result. + since PQcopyResult 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 PGEVT_RESULTCOPY + or PGEVT_RESULTDESTROY events for that result or + results copied from it. @@ -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 9afd4d88b44..c7c48d07dc7 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 7754c377488..1ec86b1d64c 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; } -- 2.30.2