Re: Serverside SNI support in libpq

Hi Daniel,

I just reviewed the patch and got a few comments:

> On Nov 11, 2025, at 06:32, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> Attached is a cleaned up rebase with improved memory handling, additional code
> documentation, removed passphrase test (sent as a separate thread), and some
> general cleanup and additional testing.
>
> --
> Daniel Gustafsson
>
> <v9-0001-Serverside-SNI-support-for-libpq.patch>

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?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/