> On 24 Sep 2026, at 22:16, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
>
> On Tue, Sep 22, 2026 at 2:56 PM Daniel Gustafsson <daniel@yesql.se> 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
