Re: pg_stat_ssl additions

On Tue, Jan 29, 2019 at 12:18:29PM +0900, Kyotaro HORIGUCHI wrote:
> At Mon, 28 Jan 2019 14:53:43 +0100, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <24783370-5acd-e0f3-8eb7-7f42ff2a026d(at)2ndquadrant(dot)com>
>> This is strange. The tests work for me, and also on the cfbot. The
>
> Agreed. It seemed so also to me.

The tests look sane to me for what it's worth.

> When initializing SSL context, it picks up the root certificate
> from my home directory, not in test installation and I had one
> there. It is not based on $HOME but pwent so it is unchangeable
> (and it is the right design for the purpose).

Oops. I agree that the tests ought to be as much isolated as
possible, without optionally fetching things depending on the user
environment.

> +# ssl-related properties may defautly set to the files in the users'
> +# environment. Explicitly provide them a value so that they don't
> +# point a valid file accidentially. Some other common properties are
> +# set here together.
> +# Attach this at the head of $common_connstr.
> +my $def_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid sslkey=invalid sslrootcert=invalid sslcrl=invalid ";
> +

Maybe it is better to just put that in test_connect_ok and
test_connect_fails for only the SSL parameters? I don't see much
point in enforcing dbname and user.

> # The server should not accept non-SSL connections.
> test_connect_fails(
> @@ -185,7 +192,7 @@ test_connect_ok(
> # Check that connecting with verify-full fails, when the hostname doesn't
> # match the hostname in the server's certificate.
> $common_connstr =
> - "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
> + $def_connstr."sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";

I think this is bad, inconsistent style. Adding the variable within
the quoted section is fine, as much as moving SERVERHOSTADDR out. But
mixing styles is not.
--
Michael