Re: libpqrcv_connect() leaks PGconn

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.