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.