Hi Daniel,
None of my comment on v9 are addressed in v10:
>
> 1 - commit message
> ```
> Experimental support for serverside SNI support in libpq, a new config
> file $datadir/pg_hosts.conf is used for configuring which certicate and
> ```
>
> Typo: certicate -> certificate
>
> 2 - be-secure-common.c
> ```
> +run_ssl_passphrase_command(const char *prompt, bool is_server_start, char
> *buf, int size, void *userdata)
> {
> int loglevel = is_server_start ? ERROR : LOG;
> char *command;
> FILE *fh;
> int pclose_rc;
> size_t len = 0;
> + char *cmd = (char *) userdata;
> ```
>
> Cmd is only passed to replace_percent_placeholders(), and the function take a
> "const char *” argument, so we can define cmd as “const char *”.
>
> 2 - be-secure-common.c
> ```
> + tokenize_auth_file(HostsFileName, file, &hosts_lines, LOG, 0);
> +
> + foreach(line, hosts_lines)
> + {
> + TokenizedAuthLine *tok_line = (TokenizedAuthLine *) lfirst(line);
> +
> + if (tok_line->err_msg != NULL)
> + {
> + ok = false;
> + continue;
> + }
> +
> + if ((newline = parse_hosts_line(tok_line, LOG)) == NULL)
> + {
> + ok = false;
> + continue;
> + }
> +
> + parsed_lines = lappend(parsed_lines, newline);
> + }
> +
> + free_auth_file(file, 0);
> ```
>
> When I read this function, I thought to raise a comment for freeing
> hosts_lines. However, then I read be-secure-openssl.c, I saw the load_hosts()
> is called within a transient hostctx, so it doesn’t have to free memory
> pieces. Can we explain that in the function comment? Otherwise other
> reviewers and future code readers may have the same confusion.
>
> 3 - be-secure-openssl.c
> ```
> int
> @@ -759,6 +933,9 @@ be_tls_close(Port *port)
> pfree(port->peer_dn);
> port->peer_dn = NULL;
> }
> +
> + Host_context = NULL;
> + SSL_context = NULL;
> }
> ```
>
> Do we need to free_contexts() here? When be_tls_init() is called again,
> contexts will anyway be freed, so I guess earlier free might be better?
>
> 4 - guc_parameters.dat
> ```
> +{ name => 'hosts_file', type => 'string', context => 'PGC_POSTMASTER', group
> => 'FILE_LOCATIONS',
> + short_desc => 'Sets the server\'s "hosts" configuration file.',
> + flags => 'GUC_SUPERUSER_ONLY',
> + variable => 'HostsFileName',
> + boot_val => 'NULL',
> +},
>
> +{ name => 'ssl_snimode', type => 'enum', context => 'PGC_SIGHUP', group =>
> 'CONN_AUTH_SSL',
> + short_desc => 'Sets the SNI mode to use for the server.',
> + flags => 'GUC_SUPERUSER_ONLY',
> + variable => 'ssl_snimode',
> + boot_val => 'SSL_SNIMODE_DEFAULT',
> + options => 'ssl_snimode_options',
> +},
> ```
>
> If ssl_snimode is PGC_SIGHUP that allows to reload without a server reset,
> why hosts_file cannot?
Comment 4 can be ignored as Jacob has answered.
> On Nov 24, 2025, at 22:53, Daniel Gustafsson <[email protected]> wrote:
>
>> On 12 Nov 2025, at 23:44, Jacob Champion <[email protected]>
>> wrote:
>
>> Did you have any thoughts on my earlier review [2]? The test patch
>> attached there still fails on my machine with v9.
>
> The attached incorporates your tests, fixes them to make them pass. The
> culprit seemed to be a combination of a bug in the code (the verify callback
> need to be defined in the default context even if there is no CA for it to be
> called in an SNI setting because OpenSSL), and that the tests were matching
> backend errors against frontend messages.
>
> The other comments from your review are also addressed, as well as additional
> cleanup and improved error handling.
>
> --
> Daniel Gustafsson
>
> <v10-0001-Serverside-SNI-support-for-libpq.patch>
I reviewed v10 again, and got some a few more comments:
5 - runtime.sgml
```
+ in <filename>pg_hba.conf</filename>. <replaceable>hostname</replaceable>
+ is matched against the hostname TLS extension in the SSL handshake.
```
In the patch code, default context uses hostname “*”, should we explain “*”
here in the doc?
6 - runtime.sgml
```
+ <filename>pg_hosts.conf</filename>, which is stored in the clusters
+ data directory. The hosts configuration file contains lines of the general
```
Typo: clusters => cluster’s
7 - runtime.sgml
```
+ will only be used to for the handshake until the hostname is inspected, it
```
“Used to for” => “used for"
8 - Cluster.pm
```
+matching the specified pattern. If the pattern matches agsinst the logfile a
```
Typo: agsinst => against
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
