Pre-existing from output.rs, but worth fixing now that this is shared code: file.keep() promotes the temp file to a permanent path that is never cleaned up. Every call to make_tls_connector (including reconnects in the output connector) leaks one file on disk.

A cleaner alternative that avoids the file entirely — parse the PEM in memory and add to the cert store directly:

let ca_cert = X509::from_pem(pem.as_bytes())
    .context("failed to parse CA certificate")?;
builder.cert_store_mut()
    .add_cert(ca_cert)
    .context("failed to add CA certificate to store")?;

More symmetric with how client certs are handled below (builder_set_client_from_pem) and eliminates the temp file lifetime concern entirely.

is this a problem we can fix?

I tried something like this when I first implemented TLS support but there was some issue with it that required pulling this trick, although I don't really remember what the problem was right now.

we do something similar here, maybe it helps https://github.com/feldera/feldera/blob/main/crates/pipeline-manager/src/config.rs#L311

This map_err closure is byte-for-byte identical to the one in the TLS branch above. Extract it before the if let to eliminate the duplication:

let make_conn_err = |e: tokio_postgres::Error| {
    ControllerError::invalid_transport_configuration(
        &endpoint_name,
        &format!(
            "Unable to connect to postgres instance '{}': {e}",
            self.config.uri
        ),
    )
};

Both branches then use .map_err(make_conn_err)?.

I agree with this

The tokio::spawn block here is identical to the one in the TLS branch. Because Connection<T> differs between TLS and NoTls variants the client assignment itself is hard to unify, but the spawn logic can be extracted into a small helper:

fn spawn_pg_connection_monitor(
    name: String,
    conn: impl Future<Output = Result<(), tokio_postgres::Error>> + Send + 'static,
) {
    tokio::spawn(async move {
        if let Err(e) = conn.await {
            error!("postgres {name}: connection error: {e}");
        }
    });
}

Eliminates the literal duplication and gives the pattern a name.

I agree with this too

has_tls() only checks for a CA cert. If a user sets ssl_client_pem or ssl_client_key but forgets ssl_ca_pem/ssl_ca_location, they silently get a plaintext connection with no indication anything is wrong. Silent fallback to plaintext when the user clearly intended TLS is the kind of misconfiguration that takes a while to diagnose.

Suggest adding a warning in make_tls_connector:

if !tls.has_tls()
    && (tls.ssl_client_pem.is_some()
        || tls.ssl_client_location.is_some()
        || tls.ssl_client_key.is_some()
        || tls.ssl_client_key_location.is_some())
{
    tracing::warn!(
        "postgres: TLS client certificate fields are set but no CA certificate \
         was provided; connecting without TLS. Set `ssl_ca_pem` or \
         `ssl_ca_location` to enable TLS."
    );
}

I agree with this too