On Fri, Jan 20, 2023 at 06:50:37PM -0800, Andres Freund wrote: > On 2023-01-20 17:12:37 -0800, Andres Freund wrote: > > We have code like this in libpqrcv_connect(): > > > > conn = palloc0(sizeof(WalReceiverConn)); > > conn->streamConn = PQconnectStartParams(keys, vals, > > > > /* expand_dbname = */ true); > > if (PQstatus(conn->streamConn) == CONNECTION_BAD) > > { > > *err = pchomp(PQerrorMessage(conn->streamConn)); > > return NULL; > > } > > > > [try to establish connection] > > > > if (PQstatus(conn->streamConn) != CONNECTION_OK) > > { > > *err = pchomp(PQerrorMessage(conn->streamConn)); > > return NULL; > > } > > > > > > Am I missing something, or are we leaking the libpq connection in case of > > errors? > > > > It doesn't matter really for walreceiver, since it will exit anyway, but we > > also use libpqwalreceiver for logical replication, where it might? > > > > > > Seems pretty clear that we should do a PQfinish() before returning NULL? I > > lean towards thinking that this isn't worth backpatching given the current > > uses of libpq, but I could easily be convinced otherwise. > > > > It's bit worse than I earlier thought: We use walrv_connect() during CREATE > SUBSCRIPTION. One can easily exhaust file descriptors right now. So I think > we need to fix this. > > I also noticed the following in libpqrcv_connect, added in 11da97024abb:
> The attached patch fixes both issues. Looks good. I'm not worried about a superuser hosing their own session via CREATE SUBSCRIPTION failures in a loop. At the same time, this fix is plenty safe to back-patch. > I seems we don't have any tests for creating a subscription that fails during > connection establishment? That doesn't seem optimal - I guess there may have > been concern around portability of the error messages? Perhaps. We have various (non-subscription) tests using "\set VERBOSITY sqlstate" for that problem. If even the sqlstate varies, a DO block is the next level of error swallowing. > I think we can control > for that in a tap test, by failing to connect due to a non-existant database, > then the error is under our control. Whereas e.g. an invalid hostname would > contain an error from gai_strerror(). That sounds fine.