Re: Serverside SNI support in libpq

> On 17 Mar 2026, at 07:22, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:
>
>> I'm a bit surprised that the .dat
>> file processing doesn't error out keys that aren't part of the DSL for defining
>> GUCs but clearly it doesn't. Fixed.
>
> This also surprised me, I wrote a patch to improve this [1].

That's great, thank you!

> I only have a few mostly stylistic comments, otherwise the patch looks good.

Thanks for reviewing!

> The start path checks ssl_passphrase_cmd for null, the reload doesn't.

The ssl_passphrase_reload can only be true if ssl_passphrase_cmd is non-null
since it otherwise won't be parsed. That being said, it's an implementation
detail bleeding through and the null check is cheap enough so added.

> + if (openssl_tls_init_hook != default_openssl_tls_init)
> + {
> + ereport(WARNING,
> + errcode(ERRCODE_CONFIG_FILE_ERROR),
> + errmsg("SNI is enabled; installed TLS init hook will be ignored"),
>
> Won't this spam the log with one warning per hosts line? It might be
> okay/acceptable, but there isn't anything line specific in this
> warning.

It will, and I've been waffling a bit on if that's good or bad. The WARNING
does apply to all lines in pg_hosts but it is as you say also not reporting any
specific context. In the end I settled for reporting it only once and added a
test for it (as well as tweaked the existing test which needed to take LibreSSL
into account, which I had missed).

> + * file. The list is returned in the hosts parameter. The function will return
> + * a HostsFileLoadResult value detailing the result of the operation. When
> + * the hosts configuration failed to load, the err_msg variable may have more
> + * information in case it was passed as non-NULL.
> + */
> +int
> +load_hosts(List **hosts, char **err_msg)
>
> Comment says HostsFileLoadResult, but the return type is int.

HostsFileLoadResult is an enum and can thus be represented by an int, and it's
done like that to avoid having to include hba.h in libpq.h for the prototype.

> +typedef enum HostsFileLoad
> +{
> + HOSTSFILE_LOAD_OK = 0,
> + HOSTSFILE_LOAD_FAILED,
> + HOSTSFILE_EMPTY,
> + HOSTSFILE_MISSING,
> + HOSTSFILE_DISABLED,
> +} HostsFileLoadResult;
>
> Is the HostsFileLoad vs HostsFileLoadResult difference intentional?

It isn't, fixed.

> +#ifdef USE_ASSERT_CHECKING
> + if (res == HOSTSFILE_DISABLED)
> + Assert(ssl_sni == false);
> +#endif
>
> Do we need this ifdef?

Assert(..); will become (void)true; in production builds so technically they
shouldn't be required to avoid compiler complaints. Personally I don't like to
have code which is only useful in assert-enabled builds be present in
non-assert builds so I ifdef-wrapped the conditional.

> And I also found a typo (distncnt):

Fixed.

--
Daniel Gustafsson