Re: Serverside SNI support in libpq - Mailing list pgsql-hackers

Hi Daniel,

I just reviewed the patch and got a few comments:

> On Nov 11, 2025, at 06:32, Daniel Gustafsson <daniel@yesql.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
cmdas “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
earlierfree 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/