> On 24 Sep 2026, at 22:16, Jacob Champion <[email protected]> > wrote: > > On Tue, Sep 22, 2026 at 2:56 PM Daniel Gustafsson <[email protected]> wrote: >> In the v3 the ssl_sni value isn't reverted at all, which albeit confusing is >> in >> line with how we treat (and document) SSL configuration so I think thats the >> better option. Flipping it in existing sessions would require a lot more >> infrastructure for little gain. > >> + # Grant pg_read_all_settings to ssltestuser so that relevant GUCs can be >> + # examined during tests >> + $node->psql('postgres', "GRANT pg_read_all_settings TO ssltestuser"); > > (This patch didn't introduce it, but $node->psql() can fail silently. > Something for later, maybe.)
Fair point. Fixed in 0002.
>> +my $log =
>> + PostgreSQL::Test::Utils::slurp_file($node->logfile, $node_loglocation);
>> +like(
>> + $log,
>> + qr/SNI is still on/,
>> + 'SSL reload triggered WARNING on ssl_sni state');
>
> Should be equivalent to
>
> $node->log_check('SSL reload triggered WARNING on ssl_sni state',
> $node_loglocation, log_like => [qr/SNI is still on/]);
Fixed.
>> - if (ssl_sni)
>> + if (new_hosts->sni_enabled)
>
>> - if (!ssl_sni)
>> + if ((SSL_hosts && !SSL_hosts->sni_enabled) || !ssl_sni)
>
> This maybe overlaps with Zsolt's review, but these diffs don't feel
> right. (In the second, I think SSL_hosts refers to the prior config?)
Right, I had it backwards in my head (again). Fixed.
> While I was reviewing this, I noticed separately that we leave
> SSL_hosts and its memory context around after be_tls_destroy(). I
> don't think it's leaked (if you turn ssl back on, the previous hosts
> should be cleared out), but it is unused. I can't remember, did we do
> that on purpose?
I can't remember it being on purpose, and I didn't see anything in the thread
suggesting it either after a quick skim. 0003 fixes this by clearing SSL_hosts
on TLS destroy.
Zsolt also reported two small issues off-list which are solved in 0004 and
0005. On a failed reload of non-SNI configuration we could be left with
dangling pointers to the previous config GUC values. This is easily solved by
pstrduping the GUC values on config init. A smaller leak was that the SSL_CTX
wasn't being cleaned up on duplicate hosts in pg_hosts parsing.
--
Daniel Gustafsson
v5-0001-Keep-current-ssl_sni-setting-on-SSL-config-reload.patch
Description: Binary data
v5-0002-Use-safe_psql-in-SSL-test-setup-to-avoid-silent-f.patch
Description: Binary data
v5-0003-Clear-SSL_hosts-when-destroying-TLS.patch
Description: Binary data
v5-0004-Copy-SSL-GUCs-into-the-hosts-context.patch
Description: Binary data
v5-0005-Delay-creation-of-SSL_CTX-structure-to-allow-clea.patch
Description: Binary data
