Re: libpqrcv_connect() leaks PGconn

Hi,

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:

if (logical)
{
PGresult *res;

res = libpqrcv_PQexec(conn->streamConn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
if (PQresultStatus(res) != PGRES_TUPLES_OK)
{
PQclear(res);
ereport(ERROR,
(errmsg("could not clear search path: %s",
pchomp(PQerrorMessage(conn->streamConn)))));
}
PQclear(res);
}

Which doesn't seem quite right? The comment for the function says:

* Returns NULL on error and fills the err with palloc'ed error message.

which this doesn't do. Of course we don't expect this to fail, but network
issues etc could still lead us to hit this case. In this case we'll actually
have an open libpq connection around that we'll leak.

The attached patch fixes both issues.

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? 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().

Greetings,

Andres Freund