Horton

Thread

Commits

  1. Don't let libpq PGEVT_CONNRESET callbacks break a PGconn.

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

  1. PGEventProcs must not be allowed to break libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-15T21:21:23Z

    I've been fooling around with the duplicated-error-text issue
discussed in [1], and I think I have a solution that is fairly
bulletproof ... except that it cannot cope with this little gem
at the bottom of PQgetResult:

            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;

Deciding to rearrange the error situation at this point doesn't
work well for what I have in mind, mainly because we'd have already
done the bookkeeping that determines which error text to emit.
But 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.

Just to add insult to injury, the "break" makes for some pretty
inconsistent semantics for other eventprocs that may be registered.
Not to mention that previously-called eventprocs might be very
surprised to find that a non-error PGresult has turned into an
error one underneath them.

I think the behavior for failing event procs ought to be that they're
just ignored henceforth, so we'd replace this bit with

            if (res->events[i].proc(PGEVT_RESULTCREATE, &evt,
                                    res->events[i].passThrough))
                res->events[i].resultInitialized = true;

and continue the policy that not-resultInitialized events don't get
PGEVT_RESULTDESTROY or PGEVT_RESULTCOPY calls.  (This'd also allow
the behavior of PQfireResultCreateEvents to be more closely synced
with PQgetResult.)

Likewise, I do not think it's acceptable to let PGEVT_RESULTCOPY
callbacks break PQcopyResult (just in our own code, that breaks
single-row mode).  So I'd drop the forced PQclear there, and say the
only consequence is to not set resultInitialized so that that event
won't get PGEVT_RESULTDESTROY later.

I also find the existing behavior that failing PGEVT_CONNRESET
events break the connection to be less than sane, and would propose
ignoring the function result in those calls too.  It's less critical
to my immediate problem though.

Thoughts?

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/ab4288f8-be5c-57fb-2400-e3e857f53e46@enterprisedb.com

  2. Re: PGEventProcs must not be allowed to break libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-16T18:11:50Z

    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

  3. Re: PGEventProcs must not be allowed to break libpq

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-02-17T14:42:27Z

    Not really related to this complaint and patch, but as far as I can see,
libpq events go completely untested in the core source.  Maybe we should
come up with a test module or something?

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

  4. Re: PGEventProcs must not be allowed to break libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-17T14:49:38Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Not really related to this complaint and patch, but as far as I can see,
> libpq events go completely untested in the core source.  Maybe we should
> come up with a test module or something?

Yeah, I suppose.  The libpq part of it is pretty simple, but still...

			regards, tom lane