Hi Daniel, I just reviewed the patch and got a few comments:
> On Nov 11, 2025, at 06:32, Daniel Gustafsson <[email protected]> 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/
